Keep uploaded shapefile names and projections out of the shell and SQL

lib/spatial_features/importers/esri_geo_json.rb

@@ -1,4 +1,5 @@
 require 'digest/md5'
+require 'open3'
 require 'spatial_features/importers/geo_json'
 
 module SpatialFeatures
@@ -15,11 +16,9 @@ def geojson
       private
 
       def esri_json_to_geojson(url)
-        if URI.parse(url).relative?
-          `ogr2ogr -t_srs EPSG:4326 -f GeoJSON /dev/stdout "#{url}"` # It is a local file path
-        else
-          `ogr2ogr -t_srs EPSG:4326 -f GeoJSON /dev/stdout "#{url}" OGRGeoJSON`
-        end
+        args = ['ogr2ogr', '-t_srs', 'EPSG:4326', '-f', 'GeoJSON', '/dev/stdout', url]
+        args << 'OGRGeoJSON' unless URI.parse(url).relative? # A relative URL is a local file path
+        Open3.capture2(*args).first
       end
     end
   end

lib/spatial_features/importers/shapefile.rb

@@ -1,5 +1,6 @@
 require 'ostruct'
 require 'digest/md5'
+require 'open3'
 
 module SpatialFeatures
   module Importers
@@ -55,8 +56,11 @@ def data_from_record(record, proj4 = nil)
         if proj4 == PROJ4_4326
           data[:geog] = wkt
         else
-          data[:geog] = ActiveRecord::Base.connection.select_value <<-SQL
-            SELECT ST_Transform(ST_GeomFromText('#{wkt}'), '#{proj4}', 4326) AS geog
+          # `proj4` is read out of the uploaded archive's .prj, so it is quoted before it
+          # reaches the statement.
+          conn = ActiveRecord::Base.connection
+          data[:geog] = conn.select_value <<-SQL
+            SELECT ST_Transform(ST_GeomFromText(#{conn.quote(wkt)}), #{conn.quote(proj4)}, 4326) AS geog
           SQL
         end
 
@@ -90,18 +94,27 @@ def validate_shapefile!(file_path)
       end
 
       # Use OGR2OGR to reproject into EPSG:4326 so we can skip the reprojection step per-feature
+      #
+      # @note Both the projection and the path come from the uploaded archive, so they are
+      #   passed as an argv list and no shell parses them. Assembling a command string here
+      #   would let an uploader run commands of their choosing.
       def project_to_4326(file_path)
         output_path = Tempfile.create([::File.basename(file_path, '.shp') + '_epsg_4326_', '.shp']) { |file| file.path }
         return unless (proj4 = proj4_from_file(file_path))
-        return unless system("ogr2ogr -s_srs '#{proj4}' -t_srs EPSG:4326 '#{output_path}' '#{file_path}'")
+        return unless system('ogr2ogr', '-s_srs', proj4, '-t_srs', 'EPSG:4326', output_path, file_path)
         return ::File.open(output_path)
       end
 
+      # Returns the PROJ.4 projection string GDAL reads out of the file's .prj, or nil when
+      # it can't determine one. Returns nil when `gdalsrsinfo` is not installed, which
+      # `proj4_projection` reports.
       def proj4_from_file(file_path)
         # Sanitize: "'+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs '\n" and lately
         #           "+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs \n" to
         #           "+proj=utm +zone=11 +datum=NAD83 +units=m +no_defs"
-        `gdalsrsinfo "#{file_path}" -o proj4`.strip.remove(/^'|'$/).presence
+        Open3.capture2('gdalsrsinfo', file_path, '-o', 'proj4').first.strip.remove(/^'|'$/).presence
+      rescue Errno::ENOENT
+        nil
       end
 
       # a zip archive may contain multiple SHP files

lib/spatial_features/unzip.rb

@@ -1,4 +1,5 @@
 require 'fileutils'
+require 'pathname'
 
 module SpatialFeatures
   module Unzip
@@ -51,16 +52,26 @@ def self.paths_in_nested_archives(paths, find:, depth:, tmpdir: nil, **extract_o
       end
     end
 
+    # Extracts the archive's entries and returns their paths, skipping entries that a name
+    # cannot place inside `tmpdir`.
+    #
+    # @param tmpdir [String] where to extract to. Must already exist, since the destination
+    #   is resolved before anything is written. Defaults to a fresh temporary directory.
     def self.extract(file_path, tmpdir: nil, downcase: false)
       tmpdir ||= Dir.mktmpdir
+      root = Pathname.new(tmpdir).realpath
+
       [].tap do |paths|
         entries(file_path).each do |entry|
           next if entry.name =~ IGNORED_ENTRY_PATHS
+          next if entry.symlink?
 
           output_filename = entry.name
           output_filename = output_filename.downcase if downcase
 
-          path = "#{tmpdir}/#{output_filename}"
+          path = contained_path(root, output_filename)
+          next unless path
+
           directory = File.dirname(path)
           basename = File.basename(path)
 
@@ -72,6 +83,24 @@ def self.extract(file_path, tmpdir: nil, downcase: false)
       end
     end
 
+    # Returns where an entry of this name lands under `root`, or nil when the name would place
+    # it outside. Entry names are stored in the archive verbatim, so they can carry `..`
+    # segments that climb out of the directory we extract into.
+    #
+    # @note `root` must already be a real path, and `::extract` skips symlink entries, so
+    #   nothing beneath it is a symlink. `cleanpath` resolves `..` lexically, so a symlink
+    #   under `root` would let an entry name walk back out of the directory unnoticed.
+    def self.contained_path(root, output_filename)
+      path = root.join(output_filename).cleanpath
+
+      return unless path.to_s.start_with?("#{root}#{File::SEPARATOR}")
+
+      # `cleanpath` drops the trailing separator that marks a directory entry, which
+      # `PathNotFound#extensions` reads to tell directories from the files it reports.
+      output_filename.end_with?(File::SEPARATOR) ? "#{path}#{File::SEPARATOR}" : path.to_s
+    end
+    private_class_method :contained_path
+
     def self.names(file_path)
       entries(file_path).collect(&:name)
     end

lib/spatial_features/version.rb

@@ -1,3 +1,3 @@
 module SpatialFeatures
-  VERSION = "3.11.1"
+  VERSION = "3.11.2"
 end

spec/lib/spatial_features/importers/shapefile_spec.rb

@@ -144,4 +144,66 @@
       end
     end
   end
+
+  context 'when given an archive whose entry name contains shell metacharacters' do
+    let(:marker) { ::File.join(Dir.mktmpdir, 'injected') }
+    let(:data) { archive_with_shell_metacharacter_entry_name(marker) }
+
+    it 'does not run the injected command' do
+      SpatialFeatures::Importers::Shapefile.create_all(data).each do |importer|
+        importer.features rescue nil # The entry is not a real shapefile, so the import fails either way
+      end
+
+      expect(::File.exist?(marker)).to be false
+    end
+  end
+
+  context 'when given a shapefile whose projection contains shell metacharacters' do
+    let(:marker) { ::File.join(Dir.mktmpdir, 'injected') }
+
+    it 'does not run the injected command' do
+      subject = SpatialFeatures::Importers::Shapefile.new(shapefile)
+      allow(subject).to receive(:proj4_from_file).and_return(%Q{+proj=longlat';touch #{marker};'})
+
+      subject.features rescue nil
+
+      expect(::File.exist?(marker)).to be false
+    end
+  end
+
+  context 'when given a shapefile whose projection contains a SQL quote' do
+    it 'sends the projection as one quoted literal rather than as statements' do
+      subject = SpatialFeatures::Importers::Shapefile.new(shapefile)
+      allow(subject).to receive(:proj4_from_file).and_return("+proj=longlat'; DROP TABLE features; --")
+
+      statements = []
+      allow(ActiveRecord::Base.connection).to receive(:select_value).and_wrap_original do |original, sql, *args|
+        statements << sql
+        original.call(sql, *args)
+      end
+
+      subject.features rescue nil
+
+      # The quote the projection carries is doubled, so everything after it stays inside the
+      # literal instead of closing it and leaving `DROP TABLE` as a statement of its own.
+      expect(statements).to include(a_string_including("'+proj=longlat''; DROP TABLE features; --'"))
+    end
+  end
+
+  context 'when a record geometry renders as WKT containing a SQL quote' do
+    it 'sends the geometry as one quoted literal rather than as statements' do
+      subject = SpatialFeatures::Importers::Shapefile.new(shapefile)
+      record = double(geometry: double(as_text: "POINT(0 0)'; DROP TABLE features; --"), attributes: {})
+
+      statements = []
+      allow(ActiveRecord::Base.connection).to receive(:select_value).and_wrap_original do |original, sql, *args|
+        statements << sql
+        nil
+      end
+
+      subject.send(:data_from_record, record, '+proj=utm +zone=11')
+
+      expect(statements).to include(a_string_including("'POINT(0 0)''; DROP TABLE features; --'"))
+    end
+  end
 end

spec/lib/spatial_features/unzip_spec.rb

@@ -0,0 +1,81 @@
+require 'spec_helper'
+
+describe SpatialFeatures::Unzip do
+  describe '::extract' do
+    # `::extract` resolves the destination before extracting into it, so it returns real
+    # paths. On macOS the temp directory is reached through a symlink.
+    let(:tmpdir) { File.realpath(Dir.mktmpdir) }
+
+    def archive_with_entry_named(name)
+      path = ::File.join(Dir.mktmpdir, 'archive.zip')
+      Zip::OutputStream.open(path) do |zos|
+        zos.put_next_entry(name)
+        zos.write('contents')
+      end
+      path
+    end
+
+    it 'extracts entries below the destination directory' do
+      paths = SpatialFeatures::Unzip.extract(archive_with_entry_named('layers/data.shp'), tmpdir: tmpdir)
+
+      expect(paths).to contain_exactly("#{tmpdir}/layers/data.shp")
+    end
+
+    # `IGNORED_ENTRY_PATHS` turns this name away before `::contained_path` sees it, so this
+    # covers the pair of them. The `::contained_path` specs below cover the check itself.
+    it 'does not extract an entry whose name climbs out of the destination directory' do
+      paths = SpatialFeatures::Unzip.extract(archive_with_entry_named('layers/../../escaped.shp'), tmpdir: tmpdir)
+
+      expect(paths).to be_empty
+      expect(::File.exist?(::File.expand_path("#{tmpdir}/../../escaped.shp"))).to be false
+    end
+
+    # `::contained_path` compares against the destination lexically, so it can only vouch for
+    # a destination that is already a real path. These two hold that up.
+
+    it 'resolves a destination reached through a symlink' do
+      link = ::File.join(Dir.mktmpdir, 'link')
+      ::File.symlink(tmpdir, link)
+
+      paths = SpatialFeatures::Unzip.extract(archive_with_entry_named('layers/data.shp'), tmpdir: link)
+
+      expect(paths).to contain_exactly("#{tmpdir}/layers/data.shp")
+    end
+
+    # Asserting on the returned paths rather than on the filesystem: rubyzip declines to
+    # create a symlink on extract, so no filesystem assertion here can tell our skip from
+    # rubyzip's. Only the returned paths change when the skip is removed.
+    it 'does not extract symlink entries' do
+      paths = SpatialFeatures::Unzip.extract(fixture_file_path('archive_with_symlink.zip'), tmpdir: tmpdir)
+
+      expect(paths).to contain_exactly("#{tmpdir}/layers/", "#{tmpdir}/layers/data.shp")
+    end
+  end
+
+  describe '::contained_path' do
+    let(:root) { Pathname.new(File.realpath(Dir.mktmpdir)).join('root') }
+
+    it 'returns where the entry lands' do
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, 'layers/data.shp')).to eq("#{root}/layers/data.shp")
+    end
+
+    it 'keeps the trailing separator that marks a directory entry' do
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, 'layers/')).to eq("#{root}/layers/")
+    end
+
+    it 'returns nil for a name that climbs out' do
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, '../escaped.shp')).to be_nil
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, 'layers/../../escaped.shp')).to be_nil
+    end
+
+    # `IGNORED_ENTRY_PATHS` turns away the names that climb out, but an absolute name starts
+    # with neither a dot nor `__macosx`, so this check is the only thing that rejects it.
+    it 'returns nil for an absolute name' do
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, '/etc/passwd')).to be_nil
+    end
+
+    it 'returns nil for a name that reaches a sibling whose path shares the prefix' do
+      expect(SpatialFeatures::Unzip.send(:contained_path, root, '../root-evil/escaped.shp')).to be_nil
+    end
+  end
+end

spec/support/fixtures.rb

@@ -109,3 +109,14 @@ def kml_file_with_ground_overlay
 def kml_file_with_ground_overlay_and_features
   open_fixture_file("kml_file_with_ground_overlay_and_features.kml")
 end
+
+# Returns an archive whose single entry is named so that a shell reaching the name would
+# treat part of it as a command and write to `marker`.
+def archive_with_shell_metacharacter_entry_name(marker)
+  path = ::File.join(Dir.mktmpdir, 'injection.zip')
+  Zip::OutputStream.open(path) do |zos|
+    zos.put_next_entry(%Q{x";touch #{marker};".shp})
+    zos.write('not a real shapefile')
+  end
+  ::File.open(path)
+end

tasks/fixtures.rake

@@ -21,6 +21,20 @@ module FixtureGenerator
     /9k=
   B64
 
+  # An archive holding a symlink entry beside a regular file. Embedded because rubyzip
+  # declines to write a symlink entry, so the task cannot build this one from its parts.
+  SYMLINK_ARCHIVE = Base64.decode64(<<~B64).freeze
+    UEsDBAoAAAAAAAJQEF0KuR8pCwAAAAsAAAAEABwAbGlua1VUCQADlOyBapTsgWp1eAsAAQT1AQAA
+    BAAAAAAvZXRjL3Bhc3N3ZFBLAwQKAAAAAAACUBBdAAAAAAAAAAAAAAAABwAcAGxheWVycy9VVAkA
+    A5TsgWqU7IFqdXgLAAEE9QEAAAQAAAAAUEsDBAoAAAAAAAJQEF3vi38LEAAAABAAAAAPABwAbGF5
+    ZXJzL2RhdGEuc2hwVVQJAAOU7IFqlOyBanV4CwABBPUBAAAEAAAAAHNoYXBlZmlsZSBieXRlcwpQ
+    SwECHgMKAAAAAAACUBBdCrkfKQsAAAALAAAABAAYAAAAAAAAAAAA7aEAAAAAbGlua1VUBQADlOyB
+    anV4CwABBPUBAAAEAAAAAFBLAQIeAwoAAAAAAAJQEF0AAAAAAAAAAAAAAAAHABgAAAAAAAAAEADt
+    QUkAAABsYXllcnMvVVQFAAOU7IFqdXgLAAEE9QEAAAQAAAAAUEsBAh4DCgAAAAAAAlAQXe+LfwsQ
+    AAAAEAAAAA8AGAAAAAAAAQAAAKSBigAAAGxheWVycy9kYXRhLnNocFVUBQADlOyBanV4CwABBPUB
+    AAAEAAAAAFBLBQYAAAAAAwADAOwAAADjAAAAAAA=
+  B64
+
   class << self
     def build_all(dir)
       FileUtils.mkdir_p(dir)
@@ -82,6 +96,8 @@ module FixtureGenerator
       # Nothing the importer recognises.
       write_zip(::File.join(dir, 'archive_without_any_known_file.zip'),
                 [['notes.whatever', :empty]])
+
+      ::File.binwrite(::File.join(dir, 'archive_with_symlink.zip'), SYMLINK_ARCHIVE)
     end
 
     # Writes `count` square polygons on a regular grid, in metres, as an ESRI Shapefile.