Skip to content

Commit 67f1301

Browse files
committed
Review comments
1 parent fbca9ea commit 67f1301

6 files changed

Lines changed: 169 additions & 31 deletions

File tree

CHANGELOG.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ Release 0.14.0 (unreleased)
1515
* Fix ``dfetch add`` crashing with a ``ValueError`` when the remote URL has a trailing slash (#1137)
1616
* Fix unhelpful error message when a metadata file is malformed (#1145)
1717
* Fix arbitrary file write via malicious tar/zip symlink (#1152)
18-
* Prevent ssh command injection (#1152)
18+
* Prevent SSH command injection (#1152)
1919

2020
Release 0.13.0 (released 2026-03-30)
2121
====================================

dfetch/util/ssh.py

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
"""SSH command allowlist validation."""
2+
3+
import os
4+
import re
5+
import shlex
6+
7+
_SAFE_SSH_OPTION_KEYS = frozenset(
8+
{"StrictHostKeyChecking", "BatchMode", "UserKnownHostsFile"}
9+
)
10+
11+
_PORT_RE = re.compile(r"^[0-9]{1,5}$")
12+
13+
14+
class InvalidSshCommandError(Exception):
15+
"""Raised when an SSH command string does not pass the allowlist."""
16+
17+
18+
def _validate_value_flag(tokens: list[str], i: int, flag: str) -> None:
19+
"""Raise InvalidSshCommandError if a -i or -p flag or its value is invalid."""
20+
if i + 1 >= len(tokens):
21+
raise InvalidSshCommandError(f"flag {flag!r} requires a value")
22+
if flag == "-p" and not _PORT_RE.match(tokens[i + 1]):
23+
raise InvalidSshCommandError("-p requires a numeric port")
24+
25+
26+
def _validate_option_flag(tokens: list[str], i: int) -> None:
27+
"""Raise InvalidSshCommandError if a -o flag or its value is invalid."""
28+
if i + 1 >= len(tokens):
29+
raise InvalidSshCommandError("-o requires a value")
30+
key = tokens[i + 1].split("=", 1)[0]
31+
if key not in _SAFE_SSH_OPTION_KEYS:
32+
raise InvalidSshCommandError(
33+
f"SSH option {tokens[i + 1]!r} is not in the allowed list"
34+
)
35+
36+
37+
def _validate_ssh_flags(tokens: list[str]) -> None:
38+
"""Raise InvalidSshCommandError if any flag token after the binary is invalid."""
39+
i = 1
40+
while i < len(tokens):
41+
flag = tokens[i]
42+
if flag in ("-i", "-p"):
43+
_validate_value_flag(tokens, i, flag)
44+
i += 2
45+
elif flag == "-o":
46+
_validate_option_flag(tokens, i)
47+
i += 2
48+
else:
49+
raise InvalidSshCommandError(f"flag {flag!r} is not allowed")
50+
51+
52+
def sanitize_ssh_cmd(ssh_cmd: str) -> str:
53+
"""Return *ssh_cmd* if it passes the allowlist; raise :exc:`InvalidSshCommandError` otherwise.
54+
55+
Accepts:
56+
* A bare absolute path to an ssh binary (no arguments).
57+
* ``ssh`` with a restricted set of flags: ``-i <file>``, ``-p <port>``,
58+
``-o`` with option key in :data:`_SAFE_SSH_OPTION_KEYS`.
59+
"""
60+
try:
61+
tokens = shlex.split(ssh_cmd)
62+
except ValueError as exc:
63+
raise InvalidSshCommandError("cannot parse ssh command") from exc
64+
65+
if not tokens:
66+
raise InvalidSshCommandError("empty ssh command")
67+
68+
binary = tokens[0]
69+
70+
if os.path.isabs(binary):
71+
if len(tokens) == 1:
72+
return ssh_cmd
73+
raise InvalidSshCommandError("absolute-path ssh binary may not carry arguments")
74+
75+
if binary != "ssh":
76+
raise InvalidSshCommandError("ssh command must be 'ssh' or an absolute path")
77+
78+
_validate_ssh_flags(tokens)
79+
return ssh_cmd

dfetch/vcs/archive.py

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -510,23 +510,21 @@ def _check_no_member_traverses_symlink(members: list[tarfile.TarInfo]) -> None:
510510
and provides defence-in-depth on newer ones.
511511
512512
Raises:
513-
RuntimeError: When a non-symlink member's path passes through a
514-
symlink member name.
513+
RuntimeError: When a member's path passes through a symlink member
514+
name, including symlink-through-symlink chains.
515515
"""
516-
symlink_names = {m.name for m in members if m.issym()}
517-
if not symlink_names:
518-
return
516+
symlink_names: set[str] = set()
519517
for member in members:
520-
if member.issym():
521-
continue
522518
parts = pathlib.PurePosixPath(member.name).parts
523-
for depth in range(1, len(parts)):
519+
for depth in range(1, len(parts) + 1):
524520
parent = str(pathlib.PurePosixPath(*parts[:depth]))
525521
if parent in symlink_names:
526522
raise RuntimeError(
527523
f"Archive member {member.name!r} would be written "
528524
f"through symlink {parent!r}"
529525
)
526+
if member.issym():
527+
symlink_names.add(str(pathlib.PurePosixPath(member.name)))
530528

531529
@staticmethod
532530
def _check_tar_members(tf: tarfile.TarFile) -> None:

dfetch/vcs/git.py

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
from dfetch.log import get_logger
1414
from dfetch.util.cmdline import SubprocessCommandError, run_on_cmdline
1515
from dfetch.util.license import is_license_file
16+
from dfetch.util.ssh import InvalidSshCommandError, sanitize_ssh_cmd
1617
from dfetch.util.util import (
1718
glob_within_root,
1819
in_directory,
@@ -29,40 +30,31 @@
2930
logger = get_logger(__name__)
3031

3132

32-
_SHELL_METACHAR_RE = re.compile(r"[;&|`$(){}<>\n\r!]")
33-
34-
35-
def _sanitize_ssh_cmd(ssh_cmd: str, source: str) -> str | None:
36-
"""Return *ssh_cmd* if it is safe to use, otherwise log a warning and return None."""
37-
if _SHELL_METACHAR_RE.search(ssh_cmd):
38-
logger.warning(
39-
"Ignoring %s: contains unsafe shell characters, falling back to 'ssh'",
40-
source,
41-
)
33+
def _try_sanitize(source: str, raw: str | None) -> str | None:
34+
if not raw:
35+
return None
36+
try:
37+
return sanitize_ssh_cmd(raw)
38+
except InvalidSshCommandError as exc:
39+
logger.warning("Ignoring %s: %s, falling back to 'ssh'", source, exc)
4240
return None
43-
return ssh_cmd
4441

4542

4643
def _build_git_ssh_command() -> str:
4744
"""Returns a safe SSH command string for Git that enforces non-interactive mode.
4845
4946
Respects existing GIT_SSH_COMMAND and git core.sshCommand.
5047
"""
51-
raw = os.environ.get("GIT_SSH_COMMAND")
52-
ssh_cmd = _sanitize_ssh_cmd(raw, "GIT_SSH_COMMAND") if raw else None
48+
ssh_cmd = _try_sanitize("GIT_SSH_COMMAND", os.environ.get("GIT_SSH_COMMAND"))
5349

5450
if not ssh_cmd:
55-
5651
try:
5752
result = run_on_cmdline(
58-
logger,
59-
["git", "config", "--get", "core.sshCommand"],
53+
logger, ["git", "config", "--get", "core.sshCommand"]
6054
)
61-
raw_config = result.stdout.decode().strip()
62-
ssh_cmd = (
63-
_sanitize_ssh_cmd(raw_config, "core.sshCommand") if raw_config else None
55+
ssh_cmd = _try_sanitize(
56+
"core.sshCommand", result.stdout.decode().strip() or None
6457
)
65-
6658
except SubprocessCommandError:
6759
ssh_cmd = None
6860

tests/test_archive.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,52 @@ def _setup(tf: tarfile.TarFile) -> None:
358358
_check_tar_members(tf)
359359

360360

361+
def test_check_tar_members_rejects_two_step_symlink_attack_dotprefix():
362+
"""Two-step tar-slip via a ./ prefixed symlink name must be rejected."""
363+
364+
def _setup(tf: tarfile.TarFile) -> None:
365+
_add_symlink(tf, "./project/link", "../../outside")
366+
content = b"escaped"
367+
info = tarfile.TarInfo("project/link/payload.txt")
368+
info.size = len(content)
369+
tf.addfile(info, io.BytesIO(content))
370+
371+
tf = _make_tar_with_member(_setup)
372+
with pytest.raises(RuntimeError, match="symlink"):
373+
_check_tar_members(tf)
374+
375+
376+
def test_check_tar_members_rejects_symlink_chain():
377+
"""A second symlink whose path passes through an earlier symlink must be rejected."""
378+
379+
def _setup(tf: tarfile.TarFile) -> None:
380+
_add_symlink(tf, "project/link", "../../outside")
381+
_add_symlink(tf, "project/link/evil", "../payload")
382+
383+
tf = _make_tar_with_member(_setup)
384+
with pytest.raises(RuntimeError, match="symlink"):
385+
_check_tar_members(tf)
386+
387+
388+
def test_check_tar_members_rejects_symlink_exact_path_overwrite():
389+
"""A non-symlink member with the same path as an earlier symlink must be rejected.
390+
391+
Python's tarfile.extract follows a symlink when writing a file over it, so
392+
allowing this would write content to the symlink's (potentially external) target.
393+
"""
394+
395+
def _setup(tf: tarfile.TarFile) -> None:
396+
_add_symlink(tf, "project/link", "../../outside")
397+
content = b"escaped"
398+
info = tarfile.TarInfo("project/link")
399+
info.size = len(content)
400+
tf.addfile(info, io.BytesIO(content))
401+
402+
tf = _make_tar_with_member(_setup)
403+
with pytest.raises(RuntimeError, match="symlink"):
404+
_check_tar_members(tf)
405+
406+
361407
def test_check_tar_members_allows_file_next_to_symlink():
362408
"""A file sibling to a symlink (not written through it) must be accepted."""
363409

tests/test_git_vcs.py

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,10 +213,10 @@ def test_ls_remote():
213213
"ssh -i keyfile -o BatchMode=yes",
214214
),
215215
(
216-
"git config",
216+
"git config with unsupported flag",
217217
None,
218218
"ssh -F configfile",
219-
"ssh -F configfile -o BatchMode=yes",
219+
"ssh -o BatchMode=yes",
220220
),
221221
("no env or git config", None, None, "ssh -o BatchMode=yes"),
222222
(
@@ -249,6 +249,24 @@ def test_ls_remote():
249249
None,
250250
"ssh -o BatchMode=yes",
251251
),
252+
(
253+
"injection via ProxyCommand option",
254+
"ssh -o ProxyCommand=evil",
255+
None,
256+
"ssh -o BatchMode=yes",
257+
),
258+
(
259+
"absolute path ssh binary accepted",
260+
"/usr/bin/ssh",
261+
None,
262+
"/usr/bin/ssh -o BatchMode=yes",
263+
),
264+
(
265+
"absolute path with arguments rejected",
266+
"/usr/bin/ssh -F config",
267+
None,
268+
"ssh -o BatchMode=yes",
269+
),
252270
],
253271
)
254272
def test_build_git_ssh_command(name, env_ssh, git_config_ssh, expected):
@@ -271,3 +289,8 @@ def test_build_git_ssh_command(name, env_ssh, git_config_ssh, expected):
271289
mock_logger.debug.assert_called_once()
272290
else:
273291
mock_logger.debug.assert_not_called()
292+
293+
if "injection" in name or "unsupported" in name or "rejected" in name:
294+
mock_logger.warning.assert_called()
295+
else:
296+
mock_logger.warning.assert_not_called()

0 commit comments

Comments
 (0)