feat(git): harden git commands by using --end-of-options separator

[6d7a]

Context

Snyk flags a couple of medium command injection issues that we should resolve.

What has been done

git 2.24+ supports the --end-of-options flag to separate command options from command arguments. This commit adds the separator to harden against command injections.

PR check list

• As much as possible, the changes include tests (unit and/or functional)
• If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

[codecov]

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.01%. Comparing base (73ab01d) to head (17c34ef).
⚠️ Report is 91 commits behind head on main.

Files with missing lines	Patch %	Lines
ggshield/core/git_hooks/ci/previous_commit.py	0.00%	2 Missing ⚠️
ggshield/core/git_hooks/ci/commit_range.py	0.00%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (62.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1245   +/-   ##
=======================================
  Coverage   94.01%   94.01%           
=======================================
  Files         198      198           
  Lines       11967    11967           
=======================================
  Hits        11251    11251           
  Misses        716      716           

Flag	Coverage Δ
unittests	94.01% <62.50%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[clement-tourriere]

Nice fix, the approach is right. I ran all 9 new calls on git 2.50 and they all behave the same as before, and the test suite passes.

One thing to solve before merge: --end-of-options needs git 2.24 (Nov 2019), and nothing declares that. Our deb/rpm packages depend on plain git with no version bound, and they ship their own Python, so a user on an old distro can install ggshield and then hit broken scans.

My proposal is in the first comment: detect the git version once, and drop the flag when git is too old. Old git then keeps working exactly like today.

Two smaller notes:

• get_list_commit_SHA() in git_shell.py is still not hardened, and it gets its value straight from CI env vars (CI_COMMIT_BEFORE_SHA, CIRCLE_RANGE, --commit-range). It also does .split(), so one env var can become several args. It is not a one liner because verticals/secret/repo.py:56 calls it with "--all", so maybe a follow up ticket.
• No test pins the new flag. A grep of tests/ finds no reference to these args, so this can regress silently later.

[clement-tourriere]

On git < 2.24 this fails with error: unknown option (exit 129). That is caught as CalledProcessError below, so this returns False for every ref, even HEAD. Then check_git_ref() rejects valid refs and scanning stops working.

Proposal: one helper in this file, used everywhere instead of the raw string.

GIT_MIN_VERSION = (2, 24)  # --end-of-options
_RX_GIT_VERSION = re.compile(r"git version (\d+)\.(\d+)")


@lru_cache(None)
def _supports_end_of_options() -> bool:
    try:
        match = _RX_GIT_VERSION.match(git(["--version"], log_stderr=False).strip())
    except (subprocess.CalledProcessError, GitError):
        return True
    if match is None:
        return True
    return (int(match.group(1)), int(match.group(2))) >= GIT_MIN_VERSION


def end_of_options() -> List[str]:
    return ["--end-of-options"] if _supports_end_of_options() else []

Then here:

cmd = ["cat-file", "-e", *end_of_options(), ref]

Two details I checked:

• It has to return True when the output cannot be parsed, because tests/unit/utils/test_git_shell.py:669 mocks subprocess.run with stdout=b"".
• The regex needs to accept vendor suffixes. I tested git version 2.50.1 (Apple Git-155), 2.45.1.windows.1, 2.24.0.rc1 and 1.8.3.1, all give the right answer.

[clement-tourriere]

This call site is the reason I would not just document the git 2.24 requirement.

On old git the error is caught below and only logged, so the fetch is skipped and we keep using a stale origin/<branch>. That gives a wrong commit range, which is the exact bug this function was added to prevent, and it happens with no visible error. A scanner that quietly scans less is worse than one that stops.

With end_of_options() this cannot happen.

[clement-tourriere]

Commit.from_sha runs git show with this same sha a second time, in ggshield/core/scan/commit.py:74, and that call is not hardened. Would be good to do both:

cmd = ["show", *PATCH_COMMON_ARGS, *end_of_options(), sha, "--"]

I checked this form works.