fix(sbom): reject argument-injection in --from-repo --ref (git clone) - #252
fix(sbom): reject argument-injection in --from-repo --ref (git clone)#252nikhilpatidar wants to merge 2 commits into
Conversation
The git-clone path reachable from `nuguard sbom generate --from-repo --ref <ref>` (and from the public `AiSbomExtractor.extract_from_repo` API) passed the ref and url to `git clone` as positional arguments. A hostile ref value starting with `-` could be interpreted by git as a flag (e.g. `--upload-pack=<cmd>`), a known argument-injection vector documented in CVE-2017-1000117 and related advisories. This change: * rejects refs that begin with `-`, contain whitespace, or are empty before subprocess.run is invoked (no partial execution possible); * rejects urls that lack an http(s)/ssh scheme; * adds a `--` separator before the url and destination in the `git clone` argv so any future regression in the regex cannot re-introduce the same injection class; * adds 21 regression tests (refs/urls rejected, well-formed inputs accepted, subprocess argv structure pins `--` placement). Thorough manual review of the threat model: the user has to opt into `--from-repo` and supply a ref from an external source. A malicious README or copy-pasteable command is the realistic delivery vector for this class, which is why a security tool should refuse the input rather than depend on git's own heuristics.
KanishkThamman
left a comment
KanishkThamman
left a comment
There was a problem hiding this comment.
Confirmed this closes a real argument-injection RCE via git clone --upload-pack=... — reproduced it locally, the -- separator + scheme check fixes it. Two small nits inline, not blockers.
| # Accepting such a ref would let a hostile ref string (e.g. one pasted from | ||
| # a malicious README) trick ``git clone`` into invoking other git options | ||
| # such as ``--upload-pack=<command>`` — a known argument-injection vector. | ||
| _SAFE_REF_RE = re.compile(r"^[^-,\s\x00][^,\s\x00]*$") |
There was a problem hiding this comment.
Nit: $ matches before a trailing newline in Python regex, so "main\n" slips past this (verified). Use \Z or re.fullmatch to close it.
There was a problem hiding this comment.
Fixed in 241cb91. Switched ^[^-,\s\x00][^,\s\x00]*$ to ^[^-,\s\x00][^,\s\x00]*\Z so a trailing newline in the ref can no longer bypass the regex. The 'main\n slips past' failure mode is closed. Added a 'main\n' regression case to the parametrize set.
| # upstream, ``extract_from_repo`` is a public API callable from any | ||
| # embedding, so we re-validate here to avoid the same injection class. | ||
| # Only http(s)/ssh transports are accepted at the clone boundary. | ||
| _SAFE_URL_RE = re.compile(r"^(https?|ssh)://[^\s\x00]*$") |
There was a problem hiding this comment.
Nit: same $ vs \Z issue as the ref regex above. Also, this rejects scp-style SSH URLs (git@host:path), which were previously accepted by extract_from_repo — a behavior regression for existing callers using that form.
There was a problem hiding this comment.
Fixed in 241cb91. Extended _SAFE_URL_RE to accept scp-style SSH (git@host:path) — the form used by every GitHub/GitLab/Bitbucket SSH clone and historically accepted by extract_from_repo. The path sub-pattern still forbids -, ,, :, whitespace, and null at the start, so a hostile provider cannot smuggle git@host:--upload-pack=evil` through.
Added three well-formed scp URLs to test_clone_repo_accepts_well_formed_urls and a new test_clone_repo_rejects_hostile_scp_urls parametrize set for the security boundary.
Two review nits on the _clone_repo argument-injection hardening: 1. The ref and URL regexes used `^...$` which Python interprets as 'no newline followed by end-of-string OR a single trailing newline'. A ref like `"main\n"` therefore slipped past _SAFE_REF_RE and reached git clone as a positional argument. Switch to `\Z`. 2. _SAFE_URL_RE only accepted http(s)/ssh:// — but extract_from_repo historically accepted scp-style SSH URLs (`git@host:path`), used by every GitHub/GitLab/Bitbucket SSH clone. Accept the scp form while still rejecting any path that starts with a flag character so a hostile provider cannot smuggle `git@host:--upload-pack=evil` through. Tests: - Add 'main\n' to the rejected-ref parametrize set. - Add three scp-style URLs to the accepted-url parametrize set. - Add a new test_clone_repo_rejects_hostile_scp_urls parametrize set for scp URLs whose path starts with `-`, `,,`, or whitespace — pinning the security boundary while preserving compatibility.
2 participants
PR Type
What
Hardens
AiSbomExtractor._clone_repoagainst argument-injection via--from-repo --refso a hostile ref string (e.g. one pasted from amalicious README) cannot be reinterpreted by
git cloneas a flag(
--upload-pack=<cmd>,--config=..., etc.).Closes #253
Why
NuGuard is a security tool whose
sbom generate --from-repoflowhands a user-supplied
--refvalue straight togit cloneas apositional argument. The same git argument-injection class has been
documented since CVE-2017-1000117 and reappears in newer advisories;
modern git rejects most leading-
-positional arguments but theprotection is not uniform across versions and a hardening check at the
NuGuard boundary costs nothing and removes the operator-machine attack
surface entirely.
The realistic delivery vector is a copy-pasted command or a README
that encourages the operator to run a specific
--ref, which is aplausible threat model for a tool that is itself invoked defensively.
Root Cause
_clone_repoassembled its argv with the ref and url as barepositional arguments:
Anything that starts with
-is parsed by git as an option.How
nuguard/sbom/extractor/core.py_SAFE_REF_RE,_SAFE_URL_RE) at class level._clone_reponow validatesrefandurland raisesValueErrorwith a clear message before any subprocess isstarted.
--separator beforeurlanddestin the argv sothat any future regression in the regex cannot re-introduce the same
injection class (defence in depth).
tests/sbom/test_clone_repo_arg_injection.py(new)21 regression tests covering:
-, embedded NUL/CR/LF/TAB,whitespace-only, empty) — each verifies
subprocess.runis notinvoked.
-, missing scheme,file://,git://, empty/whitespace).https,http,ssh) reachsubprocess.runwith the expected argv shape.
--separator placement in the argv so thedefence-in-depth check cannot silently regress.
The fix does not change any existing API behaviour for legitimate
inputs — every previously-valid ref and url still produces an identical
argv (plus the new
--separator, which git treats as an end-of-optionsmarker that affects nothing when subsequent args are non-flags).
Tests
Validation
The source-file edit is intentionally NOT auto-formatted:
ruff formatwould rewrite unrelated lines in this 3000-line module,inflating the diff and obscuring the security fix. The diff against
mainis +43/-1 in the one function plus a new test file.