From: Anupam Mediratta <mediratta@gmail.com>

applyCommit() builds a `git diff-tree ... | git apply ...` pipeline as a shell command string, interpolating the commit id and running it via os.system()/system(shell=True). The id usually comes from `git rev-list` output (safe, plain SHA-1s), but it can also come verbatim from the user-supplied `--commit` option, which is never validated (git-p4.py:2620-2631). A value such as `$(some-command)` passed to `--commit` is executed by the shell during command substitution, even though the value is wrapped in double quotes.

Replace the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, matching the pattern already used throughout this file (read_pipe, read_pipe_lines, p4_system). This removes the shell entirely, rather than relying on quoting the interpolated value.

Add a regression test exercising `git p4 submit --commit` with a shell metacharacter payload, verifying it is never interpreted.

Signed-off-by: Anupam Mediratta <mediratta@gmail.com>
---
    git-p4: avoid shell interpretation of commit ids in applyCommit
    
    P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a
    shell command string and runs it with os.system()/system(shell=True).
    The commit id it interpolates is usually a plain SHA-1 from git
    rev-list, but it can also come straight from the unvalidated --commit
    command-line option, so a value such as $(some-command) passed to
    --commit gets executed by the shell during command substitution.
    
    This replaces the shell pipeline with two argument-vector subprocess
    calls connected directly through a pipe, the same pattern already used
    everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so
    there's no shell left to escape correctly. It also adds a regression
    test in t9803 that submits a commit id crafted with shell metacharacters
    and checks they're never executed.
    
    I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm
    this contribution complies with it.
    
    Changes since v1: corrected the description of the affected data (it's
    the unvalidated --commit argument, not Perforce server data, that was
    ever exploitable here), removed the shell entirely instead of quoting
    the interpolated value, replaced a test that never reached the
    vulnerable code with one that drives it through git p4 submit --commit,
    and fixed the commit message format (subsystem prefix, rationale,
    sign-off).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v1
Pull-Request: https://github.com/git/git/pull/2411

 git-p4.py                         | 31 +++++++++++++++++++++----------
 t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

"Anupam Mediratta via GitGitGadget" <gitgitgadget@gmail.com> writes:

Both of these check the result of running diff|apply pipeline and react to a failure.

It is a bit hard to discover, but the original code catches a failed "diff|apply" pipeline invocation, because the "system()" used here is what git-p4.py defines for itself. When the pipeline fails, this system() raises subprocess.CalledProcessError().

The new one ignores the exit status from the pipeline, so even after a failure to apply the change, the program continues.

Which may not be what you want to see.

From: Anupam Mediratta <mediratta@gmail.com>

applyCommit() builds a `git diff-tree ... | git apply ...` pipeline as a shell command string, interpolating the commit id and running it via os.system()/system(shell=True). The id usually comes from `git rev-list` output (safe, plain SHA-1s), but it can also come verbatim from the user-supplied `--commit` option, which is never validated (git-p4.py:2620-2631). A value such as `$(some-command)` passed to `--commit` is executed by the shell during command substitution, even though the value is wrapped in double quotes.

Replace the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, matching the pattern already used throughout this file (read_pipe, read_pipe_lines, p4_system). This removes the shell entirely, rather than relying on quoting the interpolated value.

The shell-based call that applied the patch for real went through git-p4.py's own system() helper, which raises CalledProcessError on a non-zero exit status, so a failed apply aborted the submit. Keep that behaviour by giving the new helper the same ignore_error contract system() uses: it raises unless the caller asks for the status, and the two callers that test the status for themselves ask for it.

Add a regression test exercising `git p4 submit --commit` with a shell metacharacter payload, verifying it is never interpreted.

Signed-off-by: Anupam Mediratta <mediratta@gmail.com>
---
    git-p4: avoid shell interpretation of commit ids in applyCommit
    
    P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a
    shell command string and runs it with os.system()/system(shell=True).
    The commit id it interpolates is usually a plain SHA-1 from git
    rev-list, but it can also come straight from the unvalidated --commit
    command-line option, so a value such as $(some-command) passed to
    --commit gets executed by the shell during command substitution.
    
    This replaces the shell pipeline with two argument-vector subprocess
    calls connected directly through a pipe, the same pattern already used
    everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so
    there's no shell left to escape correctly. It also adds a regression
    test in t9803 that submits a commit id crafted with shell metacharacters
    and checks they're never executed.
    
    I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm
    this contribution complies with it.
    
    Changes since v1: as Junio pointed out, the call that applies the patch
    for real used to go through git-p4.py's own system() helper, which
    raises CalledProcessError on a non-zero exit status, so a failed git
    apply aborted the submit; v1 dropped that and silently carried on. The
    new helper now takes the same ignore_error argument system() does and
    raises by default, and the two callers that inspect the exit status
    themselves pass ignore_error=True.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v2
Pull-Request: https://github.com/git/git/pull/2411

Range-diff vs v1:

 1:  391e429a2a ! 1:  6fcacc71b8 git-p4: avoid shell interpretation of commit ids in applyCommit
     @@ Commit message
          removes the shell entirely, rather than relying on quoting the
          interpolated value.
      
     +    The shell-based call that applied the patch for real went through
     +    git-p4.py's own system() helper, which raises CalledProcessError on a
     +    non-zero exit status, so a failed apply aborted the submit. Keep that
     +    behaviour by giving the new helper the same ignore_error contract
     +    system() uses: it raises unless the caller asks for the status, and the
     +    two callers that test the status for themselves ask for it.
     +
          Add a regression test exercising `git p4 submit --commit` with a shell
          metacharacter payload, verifying it is never interpreted.
      
     @@ git-p4.py: def p4_system(cmd, *k, **kw):
               raise subprocess.CalledProcessError(retcode, real_cmd)
       
       
     -+def diffTreeApply(id, applyArgs):
     ++def diffTreeApply(id, applyArgs, ignore_error=False):
      +    """Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`
      +    without a shell, so id can never be interpreted as shell syntax. Returns
     -+    the exit status of git apply."""
     ++    the exit status of git apply, raising CalledProcessError on a non-zero
     ++    status unless ignore_error is set."""
      +    diffArgv = ["git", "diff-tree", "--full-index", "-p", id]
      +    applyArgv = ["git", "apply"] + applyArgs
      +    if verbose:
     @@ git-p4.py: def p4_system(cmd, *k, **kw):
      +    diffProc.stdout.close()
      +    applyProc.wait()
      +    diffProc.wait()
     -+    return applyProc.returncode
     ++    retcode = applyProc.returncode
     ++    if retcode and not ignore_error:
     ++        raise subprocess.CalledProcessError(retcode, applyArgv)
     ++    return retcode
      +
      +
       def die_bad_access(s):
     @@ git-p4.py: class P4Submit(Command, P4UserMap):
      -            print("TryPatch: %s" % tryPatchCmd)
      -
      -        if os.system(tryPatchCmd) != 0:
     -+        if diffTreeApply(id, tryPatchArgs) != 0:
     ++        if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:
                   fixed_rcs_keywords = False
                   patch_succeeded = False
                   print("Unfortunately applying the change failed!")
     @@ git-p4.py: class P4Submit(Command, P4UserMap):
                   if fixed_rcs_keywords:
                       print("Retrying the patch with RCS keywords cleaned up")
      -                if os.system(tryPatchCmd) == 0:
     -+                if diffTreeApply(id, tryPatchArgs) == 0:
     ++                if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:
                           patch_succeeded = True
                           print("Patch succeesed this time with RCS keywords cleaned")
       

 git-p4.py                         | 35 ++++++++++++++++++++++---------
 t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++
 2 files changed, 41 insertions(+), 10 deletions(-)