Keep uploaded shapefile names and projections out of the shell and SQL - #64
Merged
Conversation
…nd 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 GHSA-jfrj-r728-qcc9 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the privately reported vulnerability tracked in GHSA-jfrj-r728-qcc9, reported by @saidM. Releases as 3.11.2.
Importers::Shapefilebuilt shell command strings forgdalsrsinfoandogr2ogrout of the extracted.shppath, andUnzip.extractwrites 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 read from the archive's.prjinto both theogr2ogrcommand and theST_Transformstatement, 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.Those values are now passed to
gdalsrsinfoandogr2ogras an argv list so no shell parses them, and the projection is quoted withconnection.quotebefore it reaches the statement.Importers::ESRIGeoJSONgets the same treatment.Unzip.extractadditionally resolves each entry's destination against the real path of the directory it extracts into and skips entries that land outside it, along with symlink entries.Note the affected range is
<= 3.11.1, not<= 3.11.0as originally filed — 3.11.1 is published and carries the same code.Verification
Three regression specs, each confirmed to fail against the unpatched code before passing against the fix. Every guard in
unzip.rband every quoting and argv change was mutation-tested by removing it and confirming a spec goes red — nine in total, all covered.Full suite green on both CI legs locally: 279 examples, 0 failures on Ruby 3.2 / Rails 7 and on Ruby 4.0 / Rails 8.
🤖 Generated with Claude Code
https://claude.ai/code/session_017uHh49RG66T3qNFNRnPY6s