diff --git a/lib/spatial_features/importers/esri_geo_json.rb b/lib/spatial_features/importers/esri_geo_json.rb index a734f11c..35dac882 100644 --- a/lib/spatial_features/importers/esri_geo_json.rb +++ b/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 diff --git a/lib/spatial_features/importers/shapefile.rb b/lib/spatial_features/importers/shapefile.rb index 28c2bf41..fb62d22f 100644 --- a/lib/spatial_features/importers/shapefile.rb +++ b/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 diff --git a/lib/spatial_features/unzip.rb b/lib/spatial_features/unzip.rb index c694576d..65f5887f 100644 --- a/lib/spatial_features/unzip.rb +++ b/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 diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index 0d9b80a1..fbfc61f6 100644 --- a/lib/spatial_features/version.rb +++ b/lib/spatial_features/version.rb @@ -1,3 +1,3 @@ module SpatialFeatures - VERSION = "3.11.1" + VERSION = "3.11.2" end diff --git a/spec/fixtures/archive_with_symlink.zip b/spec/fixtures/archive_with_symlink.zip new file mode 100644 index 00000000..7d113827 Binary files /dev/null and b/spec/fixtures/archive_with_symlink.zip differ diff --git a/spec/lib/spatial_features/importers/shapefile_spec.rb b/spec/lib/spatial_features/importers/shapefile_spec.rb index fbe386b4..1e41af64 100644 --- a/spec/lib/spatial_features/importers/shapefile_spec.rb +++ b/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 diff --git a/spec/lib/spatial_features/unzip_spec.rb b/spec/lib/spatial_features/unzip_spec.rb new file mode 100644 index 00000000..121b1bfd --- /dev/null +++ b/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 diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index 4bc583a5..a127ca69 100644 --- a/spec/support/fixtures.rb +++ b/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 diff --git a/tasks/fixtures.rake b/tasks/fixtures.rake index a230f0aa..73d6857e 100644 --- a/tasks/fixtures.rake +++ b/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.