fix(security): prevent shell injection in tilde-username path expansion
Validate that the username portion of ~username paths contains only
valid characters (alphanumeric, dot, hyphen, underscore) before passing
to shell echo for expansion. Previously, paths like '~; rm -rf /'
would be passed unquoted to self._exec(f'echo {path}'), allowing
arbitrary command execution.
The approach validates the username rather than using shlex.quote(),
which would prevent tilde expansion from working at all since
echo '~user' outputs the literal string instead of expanding it.
Added tests for injection blocking and valid ~username/path expansion.
Credit to @alireza78a for reporting (PR #442, issue #442).
This commit is contained in:
parent
1151f84351
commit
5212644861
2 changed files with 29 additions and 4 deletions
|
|
@ -505,6 +505,25 @@ class TestExpandPath:
|
||||||
assert result == str(Path.home())
|
assert result == str(Path.home())
|
||||||
_assert_clean(result)
|
_assert_clean(result)
|
||||||
|
|
||||||
|
def test_tilde_injection_blocked(self, ops):
|
||||||
|
"""Paths like ~; rm -rf / must NOT execute shell commands."""
|
||||||
|
malicious = "~; echo PWNED > /tmp/_hermes_injection_test"
|
||||||
|
result = ops._expand_path(malicious)
|
||||||
|
# The invalid username (contains ";") should prevent shell expansion.
|
||||||
|
# The path should be returned as-is (no expansion).
|
||||||
|
assert result == malicious
|
||||||
|
# Verify the injected command did NOT execute
|
||||||
|
import os
|
||||||
|
assert not os.path.exists("/tmp/_hermes_injection_test")
|
||||||
|
|
||||||
|
def test_tilde_username_with_subpath(self, ops):
|
||||||
|
"""~root/file.txt should attempt expansion (valid username)."""
|
||||||
|
result = ops._expand_path("~root/file.txt")
|
||||||
|
# On most systems ~root expands to /root
|
||||||
|
if result != "~root/file.txt":
|
||||||
|
assert result.endswith("/file.txt")
|
||||||
|
assert "~" not in result
|
||||||
|
|
||||||
|
|
||||||
# ── Terminal output cleanliness ──────────────────────────────────────────
|
# ── Terminal output cleanliness ──────────────────────────────────────────
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -400,10 +400,16 @@ class ShellFileOperations(FileOperations):
|
||||||
return home
|
return home
|
||||||
elif path.startswith('~/'):
|
elif path.startswith('~/'):
|
||||||
return home + path[1:] # Replace ~ with home
|
return home + path[1:] # Replace ~ with home
|
||||||
# ~username format - let shell expand it
|
# ~username format - extract and validate username before
|
||||||
expand_result = self._exec(f"echo {path}")
|
# letting shell expand it (prevent shell injection via
|
||||||
if expand_result.exit_code == 0:
|
# paths like "~; rm -rf /").
|
||||||
return expand_result.stdout.strip()
|
rest = path[1:] # strip leading ~
|
||||||
|
slash_idx = rest.find('/')
|
||||||
|
username = rest[:slash_idx] if slash_idx >= 0 else rest
|
||||||
|
if username and re.fullmatch(r'[a-zA-Z0-9._-]+', username):
|
||||||
|
expand_result = self._exec(f"echo {path}")
|
||||||
|
if expand_result.exit_code == 0 and expand_result.stdout.strip():
|
||||||
|
return expand_result.stdout.strip()
|
||||||
|
|
||||||
return path
|
return path
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue