From: "brian m. carlson" <sandals@crustytoothpaste•net>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, Johannes Schindelin <johannes.schindelin@gmx•de>
Subject: Re: [PATCH 0/3] Sanitize sideband channel messages
Date: Tue, 14 Jan 2025 22:50:25 +0000 [thread overview]
Message-ID: <Z4bqMYKRP7Gva5St@tapette.crustytoothpaste.net> (raw)
In-Reply-To: <pull.1853.git.1736878772.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5777 bytes --]
On 2025-01-14 at 18:19:29, Johannes Schindelin via GitGitGadget wrote:
> When a clone fails, users naturally turn to the output of the git
> clone command. To assist in such scenarios, the output includes the messages
> from the remote git pack-objects process, delivered via what Git calls the
> "sideband channel."
>
> Given that the remote server is, by nature, remote, there is no guarantee
> that it runs an unmodified Git version. This exposes Git to ANSI escape
> sequence injection (see
> CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> terminal state, hide information, and even insert characters into the input
> buffer (as if the user had typed those characters).
I could certainly be mistaken, but I believe the report feature (e.g.,
title report), which is disabled for security reasons on all major
terminal emulators, is the only feature that can be used to adjust the
input buffer. If there are others, then those would definitely be
vulnerability in the terminal emulator, which is the place they should be
fixed.
> This patch series addresses this vulnerability by sanitizing the sideband
> channel.
>
> It is important to note that the lack of sanitization in the sideband
> channel is already "exploited" by the Git user community, albeit in
> well-intentioned ways. For instance, certain server-side hooks use ANSI
> color sequences in error messages to make them more noticeable during
> intentional failed fetches, e.g. as seen at
> https://github.com/kikeonline/githook-explode and
> https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
>
> To accommodate such use cases, Git will allow ANSI color sequences to pass
> through by default, while presenting all other ASCII control characters in a
> common form (e.g., presenting the ESC character as ^[).
>
> This vulnerability was reported to the Git security mailing list in early
> November, along with these fixes, as part of an iteration of the patches
> that led to the coordinated security release on Tuesday, January 14th, 2025.
I think there is some disagreement as to whether this constitutes a
vulnerability. I personally don't agree with that characterization, and
a CWE is a type of weakness, not a vulnerability.
Note that all of these problems could also occur by SSHing into an
untrusted server, running `curl` without redirecting output, or running
`cat` on a specially crafted file at the command line. It is
specifically expected that people use SSH to log into untrusted or
partially-trusted machines, so this is not just a thought exercise.
None of those cases would be addressed by this series.
> While Git for Windows included these fixes in v2.47.1(2), the consensus,
> apart from one reviewer, was not to include them in Git's embargoed
> versions. The risk was considered too high to disrupt existing scenarios
> that depend on control characters received via the sideband channel being
> sent verbatim to the user's terminal emulator.
>
> Several reviewers suggested advising terminal emulator writers about these
> "quality of implementation issues" instead. I was quite surprised by this
> approach, as it seems overly optimistic to assume that terminal emulators
> could distinguish between control characters intentionally sent by Git and
> those unintentionally relayed from the remote server.
I've done some analysis of this approach after discussion on the
security list and I don't think we should adopt it, as I mentioned
there.
Where pre-receive hooks are available, people frequently run various
commands to test and analyze code in them, including build or static
analysis tools, such as Rust's Cargo. Cargo is capable of printing a
wide variety of escape sequences in its output, including `\e[K`, which
overwrites text to the right (e.g., for progress bars and status output
much like Git produces), and sequences for hyperlinks. Stripping these
sequences would break the output in ways that would be confusing to the
user (since they work fine in a regular terminal) and hard to
reproduce or fix.
There are a variety of other terminal sequences that I have also seen
practically used here which would also be broken. Other sequences that
could usefully be sent (but I have not seen practically implemented)
include sixel codes (which are a type of image format) that could be
used to display QR codes for purposes such as tracking CI jobs or
providing a "receipt" of code pushed.
I agree that this would have been a nice feature to add at the beginning
of the development of the sideband feature, but I fear that it is too
late to make an incompatible change now.
I realize that you've provided an escape hatch, but as we've seen with
other defense-in-depth measures, that doesn't avoid the inconvenience
and hassle of dealing with those changes and the costs of deploying
fixes everywhere. We need to consider the costs and impact of these
patches on our users, including the burden of dealing with incompatible
changes, and given the fact that this problem can occur in a wide
variety of other contexts which you are not solving here and which would
be better solved more generally in terminal emulators themselves, I
don't think the benefits of this approach outweigh the downsides.
I do agree that there are terminal emulators which have some surprising
and probably insecure behaviour, as we've discussed in the past, but
because I believe those issues are more general and could be a problem
for any terminal-using program, I continue to believe that those issues
are best addressed in the terminal emulator itself.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
next prev parent reply other threads:[~2025-01-14 22:50 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-14 18:19 [PATCH 0/3] Sanitize sideband channel messages Johannes Schindelin via GitGitGadget
2025-01-14 18:19 ` [PATCH 1/3] sideband: mask control characters Johannes Schindelin via GitGitGadget
2025-01-15 14:49 ` Phillip Wood
2025-12-02 15:43 ` Johannes Schindelin
2025-01-15 15:17 ` Andreas Schwab
2025-01-15 16:24 ` Junio C Hamano
2025-01-14 18:19 ` [PATCH 2/3] sideband: introduce an "escape hatch" to allow " Johannes Schindelin via GitGitGadget
2025-01-14 18:19 ` [PATCH 3/3] sideband: do allow ANSI color sequences by default Johannes Schindelin via GitGitGadget
2025-01-14 22:50 ` brian m. carlson [this message]
2025-01-16 6:45 ` [PATCH 0/3] Sanitize sideband channel messages Junio C Hamano
2025-01-28 16:03 ` Ondrej Pohorelsky
2025-01-31 17:55 ` Junio C Hamano
2025-12-02 14:11 ` Johannes Schindelin
2025-12-03 0:47 ` brian m. carlson
2025-12-03 8:04 ` Johannes Schindelin
2025-01-15 14:49 ` Phillip Wood
2025-12-02 14:56 ` Johannes Schindelin
2025-12-17 14:23 ` [PATCH v2 0/4] " Johannes Schindelin via GitGitGadget
2025-12-17 14:23 ` [PATCH v2 1/4] sideband: mask control characters Johannes Schindelin via GitGitGadget
2026-01-09 12:38 ` Patrick Steinhardt
2026-01-16 19:29 ` Johannes Schindelin
2025-12-17 14:23 ` [PATCH v2 2/4] sideband: introduce an "escape hatch" to allow " Johannes Schindelin via GitGitGadget
2025-12-18 2:22 ` Junio C Hamano
2025-12-18 17:59 ` Johannes Schindelin
2025-12-19 13:33 ` Junio C Hamano
2026-01-16 19:25 ` Johannes Schindelin
2026-01-09 12:38 ` Patrick Steinhardt
2025-12-17 14:23 ` [PATCH v2 3/4] sideband: do allow ANSI color sequences by default Johannes Schindelin via GitGitGadget
2026-01-09 12:38 ` Patrick Steinhardt
2026-01-16 19:38 ` Johannes Schindelin
2025-12-17 14:23 ` [PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through Johannes Schindelin via GitGitGadget
2026-01-09 12:38 ` Patrick Steinhardt
2026-01-10 17:26 ` brian m. carlson
2026-01-15 21:14 ` Jeff King
2026-01-15 21:36 ` Junio C Hamano
2026-01-15 23:12 ` Johannes Schindelin
2026-01-16 6:45 ` Patrick Steinhardt
2026-01-16 12:12 ` Ondrej Pohorelsky
2026-01-16 15:21 ` Junio C Hamano
2026-01-16 18:46 ` Johannes Schindelin
2026-01-16 19:24 ` Junio C Hamano
2026-01-19 7:20 ` Patrick Steinhardt
2026-01-19 22:16 ` brian m. carlson
2026-01-20 2:41 ` D. Ben Knoble
2026-01-20 17:05 ` Junio C Hamano
2026-01-20 19:31 ` Jeff King
2026-01-20 20:11 ` Junio C Hamano
2026-01-21 7:39 ` Patrick Steinhardt
2026-01-22 12:29 ` Johannes Schindelin
2026-01-22 17:58 ` Junio C Hamano
2026-01-15 23:10 ` brian m. carlson
2026-02-03 1:11 ` Junio C Hamano
2026-02-03 7:12 ` Johannes Schindelin
2026-02-03 19:00 ` Junio C Hamano
2026-02-04 19:35 ` Junio C Hamano
2026-01-16 19:47 ` Johannes Schindelin
2026-01-16 22:26 ` [PATCH v3 0/5] Sanitize sideband channel messages Johannes Schindelin via GitGitGadget
2026-01-16 22:26 ` [PATCH v3 1/5] sideband: mask control characters Johannes Schindelin via GitGitGadget
2026-01-16 22:26 ` [PATCH v3 2/5] sideband: introduce an "escape hatch" to allow " Johannes Schindelin via GitGitGadget
2026-01-16 22:26 ` [PATCH v3 3/5] sideband: do allow ANSI color sequences by default Johannes Schindelin via GitGitGadget
2026-01-16 22:26 ` [PATCH v3 4/5] sideband: add options to allow more control sequences to be passed through Johannes Schindelin via GitGitGadget
2026-01-16 22:26 ` [PATCH v3 5/5] sideband: offer to configure sanitizing on a per-URL basis Johannes Schindelin via GitGitGadget
2026-01-16 22:32 ` [PATCH v3 0/5] Sanitize sideband channel messages Johannes Schindelin
2026-02-03 10:17 ` [PATCH v4 0/6] " Johannes Schindelin via GitGitGadget
2026-02-03 10:17 ` [PATCH v4 1/6] sideband: mask control characters Johannes Schindelin via GitGitGadget
2026-02-03 10:17 ` [PATCH v4 2/6] sideband: introduce an "escape hatch" to allow " Johannes Schindelin via GitGitGadget
2026-02-03 10:17 ` [PATCH v4 3/6] sideband: do allow ANSI color sequences by default Johannes Schindelin via GitGitGadget
2026-02-03 10:18 ` [PATCH v4 4/6] sideband: add options to allow more control sequences to be passed through Johannes Schindelin via GitGitGadget
2026-02-03 10:18 ` [PATCH v4 5/6] sideband: offer to configure sanitizing on a per-URL basis Johannes Schindelin via GitGitGadget
2026-02-03 10:18 ` [PATCH v4 6/6] sideband: delay sanitizing by default to Git v3.0 Johannes Schindelin via GitGitGadget
2026-02-04 19:26 ` [PATCH v4 0/6] Sanitize sideband channel messages Junio C Hamano
2026-02-05 14:48 ` Junio C Hamano
2026-02-13 23:50 ` Junio C Hamano
2026-03-02 18:11 ` [PATCH 0/3] Sanitizing sideband output Junio C Hamano
2026-03-02 18:11 ` [PATCH 1/3] sideband: drop 'default' configuration Junio C Hamano
2026-03-02 18:11 ` [PATCH 2/3] sideband: delay sanitizing by default to Git v3.0 Junio C Hamano
2026-03-02 18:11 ` [PATCH 3/3] sideband: conditional documentation fix Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 0/7] Sanitizing sideband output Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 1/7] sideband: mask control characters Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 2/7] sideband: introduce an "escape hatch" to allow " Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 3/7] sideband: do allow ANSI color sequences by default Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 4/7] sideband: add options to allow more control sequences to be passed through Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 5/7] sideband: offer to configure sanitizing on a per-URL basis Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 6/7] sideband: drop 'default' configuration Junio C Hamano
2026-03-05 23:34 ` [PATCH v5 7/7] sideband: delay sanitizing by default to Git v3.0 Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z4bqMYKRP7Gva5St@tapette.crustytoothpaste.net \
--to=sandals@crustytoothpaste$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox