fix(security): drop shell=True from subprocess invocations#1340
fix(security): drop shell=True from subprocess invocations#1340reddraconi wants to merge 1 commit intoTagStudioDev:mainfrom
Conversation
Removes `shell=True` from the three Windows-platform subprocess calls
in qt/utils/file_opener.py and the ripgrep invocation in
core/library/refresh.py.
Each call now passes its arguments as a list and goes straight to
execvp / CreateProcess, so filenames containing shell metacharacters
(&, ^, %, (), $(...), backticks) are treated as plain data instead of
being parsed by cmd.exe or /bin/sh.
- "Show in Explorer" passes ["explorer", f"/select,{path}"].
- The Windows-start fallback passes ["cmd", "/c", "start", "", path].
- The default Windows open uses `os.startfile`.
- ripgrep is invoked with its arguments as a list.
Behavior should be unchanged for ordinary filenames.
We passed 231 of 231 tests and ruff and mypy came back clean.
Refs TagStudioDev#1339
| [command_name] + command_args, | ||
| shell=True, |
There was a problem hiding this comment.
I'm not on windows so I can't check, but it seems to me like removing shell = True and then calling the shell manually instead would still cause shell meta chars to be parsed (as it would with bash -c "<cmd>". (Although the syntax of /c is different from bash so maybe not)
There was a problem hiding this comment.
Craaaaaap. You're right. I think it'll still expand stuff like %VAR%, too after /c.
If we have to keep the edge case and still use cmd, we could wrap the whole path in double quotes and parse the guts of the string for internal double quotes and percents, then toss them? Or just reject the string. It wouldn't be perfect, but it's better than sending raw stuff to cmd.
Another option is PowerShell, but that could run into ExecutionPolicy issues, depending on how the host is configured. I know I lock down ExecutionPolicy for my users via group policy to make sure they don't get up to shenanigans.
Something like this with -LiteralPath could work since it won't expand into anything weird:
def _open_with_powershell(normpath: str) -> None:
escaped = normpath.replace("'", "''")
ps_script = f"Start-Process -FilePath '{escaped}'"
# -EncodedCommand wants base64 of UTF-16LE.
encoded = base64.b64encode(ps_script.encode("utf-16-le")).decode("ascii")
silent_popen(
["powershell", "-NoProfile", "-NonInteractive", "-EncodedCommand", encoded],
close_fds=True,
creationflags=subprocess.CREATE_NEW_PROCESS_GROUP
| subprocess.CREATE_BREAKAWAY_FROM_JOB,
)
if windows_start_command:
_open_with_powershell(normpath)
else:
os.startfile(normpath)Otherwise, if we don't need the edge case, os.startfile path would still be the way to do it.
Thoughts?
2 participants
Removes
shell=Truefrom the three Windows-platform subprocess calls in qt/utils/file_opener.py and the ripgrep invocation in core/library/refresh.py.Each call now passes its arguments as a list and goes straight to execvp / CreateProcess, so filenames with shell metacharacters (&, ^, %, (), $(...), backticks) aren't parsed by cmd.exe or /bin/sh.
Behavior should be unchanged for ordinary filenames.
We passed 231 of 231 tests. ruff and mypy came back clean.
Refs #1339
Summary
#1339 contains the vulnerability description, the Windows and POSIX reproduction steps, and the standalone reproducer script.
This PR removes
shell=Truefrom the four subprocess invocations noted in #1339 and replaces each with the equivalent list-argument form to bypass the shell entirely:src/tagstudio/qt/utils/file_opener.py["explorer", f"/select,{path}"]directly.["cmd", "/c", "start", "", path].os.startfile.src/tagstudio/core/library/refresh.pyNo new dependencies. No user-visible change for valid filenames.
Tasks Completed
ruff checkcleanmypy --config-file pyproject.toml .clean