From 87820709cae9ea5cbe194897aaebe4a769927769 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Sun, 16 Aug 2026 09:05:53 -0700 Subject: [PATCH] fix: Keep uploaded shapefile names and projections out of the shell and SQL `Importers::Shapefile` built shell command strings for `gdalsrsinfo` and `ogr2ogr` out of the extracted `.shp` path, and `Unzip.extract` writes each archive entry under whatever name that entry carries, so a ZIP whose entry name held shell metacharacters ran as a command. The same class interpolated the PROJ.4 string `gdalsrsinfo` reads from the archive's `.prj` into both the `ogr2ogr` command and the `ST_Transform` statement in `data_from_record`, so a crafted projection reached the shell and the database as well. All of it runs on the ordinary import path before the shapefile is validated, and a host app cannot defend against it because the gem owns the unzip-then-shell-out step. Instead of interpolating those values into a command string, we pass them to `gdalsrsinfo` and `ogr2ogr` as an argv list so no shell parses them, and quote the projection with `connection.quote` before it reaches the statement. `Importers::ESRIGeoJSON` gets the same argv treatment for its own `ogr2ogr` call. `proj4_from_file` rescues `Errno::ENOENT` so a missing `gdalsrsinfo` still surfaces as `IndeterminateShapefileProjection`, which is what the string form gave us by returning an empty string. `Unzip.extract` now resolves each entry's destination against the real path of the directory it extracts into, skipping entries that land outside it along with symlink entries. Neither escape was reachable before, since `IGNORED_ENTRY_PATHS` turns away every `..` name and rubyzip declines to create symlinks on extract, so this makes containment a property of the method itself rather than of a pattern written to hide dotfiles and of a dependency's behaviour. Reported by @saidM. Refs https://github.com/culturecode/spatial_features/security/advisories/GHSA-jfrj-r728-qcc9 Co-Authored-By: Claude Opus 5 (1M context) --- .../importers/esri_geo_json.rb | 9 +- lib/spatial_features/importers/shapefile.rb | 21 ++++- lib/spatial_features/unzip.rb | 31 ++++++- lib/spatial_features/version.rb | 2 +- spec/fixtures/archive_with_symlink.zip | Bin 0 -> 485 bytes .../importers/shapefile_spec.rb | 62 ++++++++++++++ spec/lib/spatial_features/unzip_spec.rb | 81 ++++++++++++++++++ spec/support/fixtures.rb | 11 +++ tasks/fixtures.rake | 16 ++++ 9 files changed, 222 insertions(+), 11 deletions(-) create mode 100644 spec/fixtures/archive_with_symlink.zip create mode 100644 spec/lib/spatial_features/unzip_spec.rb 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 0000000000000000000000000000000000000000..7d113827b6774cd108e7eb3737f10de144c21f75 GIT binary patch literal 485 zcmWIWW@h1H00E`|fmp7c@|xU0HVCsY$S~w&=4FS5a56AYdDEB$!lf1542&#a89~xO zpr2ZjtY45=TwI*$4N@7W(UU5bN zK7)!g5(`q(GILTDk}6A5i@5^48JXmmad}b#=1hjS3qeeXmsufRM)NMZNpP(|20H`H xBmsuEj-DWsV16dfFjOBy3gkcE(<1&<$4XB!d83^A1>Bk@r0|3u;Ti*Zx literal 0 HcmV?d00001 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.