From: brian m. carlson Date: Wed, 03 Dec 2025 00:47:16 GMT Subject: Re: [PATCH 0/3] Sanitize sideband channel messages Message-ID: In-Reply-To: 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. 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. 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). > 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. > 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. 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. > 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. > 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