Re: [PATCH 0/3] Sanitize sideband channel messages

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 3, 2025, 08:04 UTC
Message-ID: <416327f5-0c12-350a-ad8f-67b402a303d6@gmx.de>
In-Reply-To: <aS-D5lD2Kk6BHNIl@fruit.crustytoothpaste.net>

Hi brian,

On Wed, 3 Dec 2025, brian m. carlson wrote:

Show 13 quoted lines

> On 2025-12-02 at 14:11:54, Johannes Schindelin wrote:
> > So you haven't come across `OSC P 1 0 ; ? ST` (see e.g.
> > https://www.xfree86.org/current/ctlseqs.html#:~:text=OSC%20P%20s%20;%20P%20t%20ST
> > for this control sequence, as well as others that elicit responses from
> > terminal emulators, from current cursor position to terminal
> > capabilities)? I use this Escape sequence myself in my `tmux` sessions to
> > toggle the colors between bright-on-dark and dark-on-bright.
> 
> So let's talk about this class of escape sequences with your patches for
> a moment.  I compiled the patches in this series on my system and
> changed the default PATH to use that client-side git binary (the
> server-side is unchanged).  I have not changed any configuration related
> to your patches, so the behaviour is the patch default.

Thank you for testing this patch series; I'm really happy that we can cross this long-standing item off the list. It opens the door for us to work together, and I am eager to keep the momentum going.

Show 42 quoted lines

> I have a server called castro (after the San Francisco neighbourhood)
> and I added the following script called `~/bin/fake-git-upload-pack`,
> which should let us simulate a malicious server:
> 
> ----
> #!/bin/sh
> 
> printf '\033]10;rgb:ffff/ffff/ffff\007Hello, world!\n' >&2
> 
> exec git-upload-pack "$@"
> ----
> 
> This basically uses this class of escape sequences to change the
> foreground colour to bright white.
> 
> I then ran a clone command, like so:
> 
> ----
> % git clone -u fake-git-upload-pack castro:~/git/css.git
> Cloning into 'css'...
> Hello, world!
> remote: Enumerating objects: 663, done.
> remote: Counting objects: 100% (4/4), done.
> remote: Compressing objects: 100% (3/3), done.
> remote: Total 663 (delta 0), reused 0 (delta 0), pack-reused 659 (from 1)
> Receiving objects: 100% (663/663), 114.83 KiB | 38.28 MiB/s, done.
> Resolving deltas: 100% (329/329), done.
> ----
> 
> Despite my patched Git binary, the escape sequence was executed and my
> foreground colour was changed.  So I don't think these patches are
> sufficient to actually fix the issue and I somewhat doubt that it's even
> possible at all to defend against a malicious SSH server which would
> like to send arbitrary escape sequences in general.
> 
> I don't think we can just close stderr or not wire it up to the TTY
> because OpenSSH needs the TTY to prompt and doing so also breaks things
> on Windows.[0]  There are also cases where the remote side sends
> messages over the Banner portion of the protocol that are required for
> auth ($DAYJOB sends a unique URL for 2FA, for instance) and redirecting
> stderr to `/dev/null` would mean that people couldn't log into those
> machines.

Just to clarify: the patches here are specifically about the sideband behavior in HTTPS, where the attack scenario that I am trying to defend against involves a malicious server posing as a trusted one, e.g. via a domain that looks highly similar to "github.com". In that context, SSH isn’t really applicable, victims would be immediately suspicious if asked for SSH credentials.

So while your SSH tests are interesting, they’re outside the scope of this patch series. If you’d like to explore equivalent fixes on the SSH side, that would be a great complementary effort, but the focus here is ensuring sideband over HTTPS is handled securely.

> If it's the case that we effectively can't fix this for SSH, I don't see
> the advantage to trying to patch this for HTTPS, since it would give a
> false sense of security and many people use both in their daily work (I
> certainly do).

Thanks for sharing your opinion.

I would frame it a bit differently, though: security is rarely all‑or‑nothing, it’s a game of layers.

Even if SSH-based Git operations have weaknesses that seem to be unlikely to be fully fixable right now, that doesn’t mean we should leave HTTPS-based operations exposed when we can strengthen them. Each improvement reduces the attack surface, and together they add up to meaningful protection. So rather than a false sense of security, I see this patch series as one necessary step in a layered approach. If complementary work on SSH becomes possible, that would be great, but in the meantime it seems rational to secure what we can.

Show 14 quoted lines

> > It is true that many terminal emulators started disabling support for such
> > Escape sequences. But that's not because the terminal emulators' features
> > were buggy. That's because some console programs are buggy, allowing
> > payload originating from outside the user's trust boundary to be passed
> > through to the terminal without proper sanitizing. That's what the entire
> > CWE-150 weakness class (https://cwe.mitre.org/data/definitions/150.html)
> > is all about.
> 
> It is in general very difficult to eliminate all sources of untrusted
> input in the terminal because people run `cat` and a variety of other
> tools on untrusted files all the time.  It would certainly be convenient
> if we did not need to deal with that case, but we do nonetheless.
> That's why we've tended to patch terminal emulators when escape
> sequences execute code.

You’re right that users sometimes do things that are hard or impossible to protect against. There is nothing we can do in Git's source code to prevent a user from `cat`ing a malicous file, for example.

But what we _can_ do is to ensure that Git, at least, is as secure by default as we can make it. In this context: to sanitize control sequences originating from outside Git's (or the Git user's) control. That’s exactly why CWE‑150 exists: the responsibility lies with programs to sanitize what they pass through, rather than expecting terminal emulators to defend against every possible misuse.

Patching terminal emulators is a last‑resort mitigation, and given the number of terminal emulators, it's once again far from an "all-or-nothing" situation. In line with your earlier argument, one terminal emulator's maintainer could claim that _another_ terminal emulator is still unpatched, so why should _they_ be required to patch theirs? You see where that leads, to finger pointing instead of security, and we all lose. I'd rather see all terminal emulator projects doing what is in their power to add layers of security, just as I am trying to do what is in my power regarding Git's security in this here mail thread.

Concretely, even if we cannot eliminate all sources of untrusted input in the terminal, in general, we should at least do our best to prevent Git from passing through untrusted input to the terminal.

Show 14 quoted lines

> > That check, whether the output is even sent to a terminal emulator or not,
> > is notably something that cannot ever be done by those `pre-receive` hooks
> > that were held up as examples to block this here patch series: They have
> > no way of knowing whether or not their output goes to a terminal, but they
> > send the control sequences anyway. Because YOLO, I guess. In that
> > respect, I think that even you two would agree that those `pre-receive`
> > hooks are broken by design.
> 
> I don't agree.  Lots of systems that are not terminals interpret
> at least some terminal escape sequences, such as GitHub Actions.  And I
> can tell you that there are a substantial number of organizations that do
> indeed have actual pre-receive hooks in production that use terminal
> escape sequences without actually knowing that the other side supports
> them because I have had to troubleshoot those pre-receive hooks.

Oh, I can imagine how cumbersome troubleshooting such `pre-receive` hooks can be. I can imagine how insistent the inventors of such hooks are on doing a legitimate thing. And because they are paying customers... they are right.

And far be I from noticing that many systems that are not terminals interpret some Escape sequences. In the extensive research I conducted in October and November last year in the course of developing this here patch series, I even stumbled across successful exploits targeting users of web-based log viewers that interpret such Escape sequences, a scenario in which I myself would have easily fallen prey to such an attack, as I would have been totally unprepared to even suspect that the log viewer shows me anything but plain text.

Having said all that, it is incorrect in general to assume that all consumers of the output of Git commands _can_ interpret Escape sequences, even if there should be a surprising number of consumers that do.

Show 7 quoted lines

> Even if we were to agree that it might not be desirable to send terminal
> escape sequences without knowing if there's a terminal, people do it,
> and even Vim does it (try `TERM=dumb vim -e`, whereupon it will send
> escape sequences, much to my annoyance).  I don't think we can say that
> everybody thinks this kind of thing is unreasonable and clearly some
> people very much want to do it and make reasonably good use of it, so
> it's a use case we should consider.

I am puzzled. Do you really want to maintain that it is rational to send Escape sequences without checking whether the receiver can interpret them as desired? By this rationale, we could simplify the logic in `color.c` rather dramatically, and always send Escape sequences. If you think this through, I am sure you will want to stop this train of thought.

Show 17 quoted lines

> > Also, it is relatively easy if you fail to protect your terminal emulator
> > to have your entire session messed up to a point where not even a `reset`
> > restores it. And corrupting the terminal session is still much better than
> > getting pranked by having all of Git's output be overwritten with a
> > picture of a snake (download the raw version of
> > https://github.com/csdvrx/sixel-testsuite/blob/master/snake.six -- after!
> > verifying that it is just a regular text file containing only a few
> > harmless escape sequences~ -- and then `cat` it to your terminal). That
> > could have been goatse, too, though. Or for that matter (as
> > https://github.com/mpv-player/mpv demonstrates, which allows you to render
> > entire Youtube videos in your current terminal window) you could be
> > Rick-rolled. And all of those are still pranks more than anything. Much
> > worse can be done with those terminal emulator capabilities.
> 
> As I mentioned, sending Sixel images can be legitimately useful to send
> things like QR codes to build outputs or for things like authentication.
> Certainly there are less savoury things one can do as well.

Right. But the presence of legitimate use-cases does not legitimize holding up bug fixes. For a humorous take on this, see https://xkcd.com/1172/.

By definition, every security bug fix is a breaking change. Just like the user of the space bar heater in that xkcd comic, the `pre-receive` hooks you cited rely on a bug that needs to be fixed.

And a bug in Git it is, it's a weakness, matching CWE-150, giving rise to vulnerabilities I have illustrated on the git-security mailing list (which I will make public once I am reasonably certain that most Git users have had a chance to upgrade to versions that no longer have those vulnerabilities).

Ciao, Johannes

Show 30 quoted lines

> > For the record, I was almost successfully gas-lit into believing that this
> > here issue is not even a vulnerability, as was claimed by some (but not
> > all) involved in the discussion on the Git security list. Fortunately I am
> > in a wonderful position that I have access to outstanding security
> > researchers, and I asked two of them, independently, to tell me whether or
> > not this is a vulnerability that needs to be fixed. Independently, both
> > agreed that my assessment "High" was too high, and it should have been
> > "Moderate" instead. At the same time, they also both agreed that it is a
> > vulnerability that should be fixed in Git.
> 
> I don't think "gas-lit" is an accurate characterization of the
> discussion.  I disagreed with you that this was a Git-specific problem
> and some others wanted more discussion about the matter.  I don't think
> anyone else had intentions of misleading or deceiving you, or making you
> doubt your memory or perceptions of reality, and I certainly did not.
> Instead, we simply disagreed on a technical matter.  Linus and I have
> clearly disagreed strongly on some matters on this list in the past and
> I don't think that "gaslighting" would be an accurate characterization
> there, either.
> 
> I will state that while I do disagree with you on this matter and it's
> clear that we don't always see eye to eye or necessarily get along
> famously, I do appreciate the work that you do for this project and Git
> for Windows and I do respect you and your contributions.
> 
> [0] I remember this from Git LFS: https://github.com/git-lfs/git-lfs/issues/1843
> -- 
> brian m. carlson (they/them)
> Toronto, Ontario, CA
>