fix: pass converter commands as argv lists instead of via a shell - #114
Open
dustenhubbard wants to merge 1 commit into
Open
fix: pass converter commands as argv lists instead of via a shell#114dustenhubbard wants to merge 1 commit into
dustenhubbard wants to merge 1 commit into
Conversation
The scaled-zarr converter and the neuroglancer export built their commands by f-string-quoting fields taken from the series file (series.src_dir, series.jser_fp, and the --output argument) and ran them with subprocess.Popen(..., shell=True) on non-Windows platforms. Double quotes do not neutralize a shell: $(...), backticks, and a closing quote followed by ; or && are all interpreted. A series whose src_dir contained shell metacharacters would have those characters executed, so opening a shared .jser and then running the converter or export could run arbitrary commands with the user's privileges, with no visible sign. Pass the command as an argv list on every platform, with each path as a single literal element, and drop shell=True and the manual quoting. The Windows branch already passed a list; both platforms now share the same argv. Legitimate paths, including paths with spaces, are handled correctly by construction. Covers both launch sites: convert_zarr and create_ng_zarr.
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
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.
Passes the two converter commands as argv lists instead of joining them into a string and running them through a shell, which closes an arbitrary-command-execution path from a shared series file.
Root cause
The scaled-zarr converter and the neuroglancer export built their commands by f-string-quoting fields taken from the series (
series.src_dir,series.jser_fp, and the--outputargument) and ran them withsubprocess.Popen(..., shell=True)on non-Windows platforms:src_diris read verbatim fromseries_data["src_dir"]in the.jser. Double quotes do not neutralize a shell:$(...), backticks, and a closing quote followed by;or&&all execute. Opening a series prepared by someone else and then running a conversion, the ordinary next step, runs whatever the field contains, with the user's privileges and no visible sign.Change
Pass
convert_cmdas a list tosubprocess.Popenon every platform, with each path a single literal element, and dropshell=Trueand the manual quoting. The Windows branch already passed a list, so both platforms now share the same argv. Legitimate paths, including paths with spaces, are handled correctly by construction, which is what the quoting was reaching for.Covers both launch sites:
convert_zarrandcreate_ng_zarr.Notes
Detail and a reproduction are in the linked issue.
Closes #113