From: "brian m. carlson" <sandals@crustytoothpaste•net>
To: Johannes Schindelin <Johannes.Schindelin@gmx•de>
Cc: Junio C Hamano <gitster@pobox•com>,
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH 0/3] Sanitize sideband channel messages
Date: Wed, 3 Dec 2025 00:47:16 +0000 [thread overview]
Message-ID: <aS-D5lD2Kk6BHNIl@fruit.crustytoothpaste.net> (raw)
In-Reply-To: <f4a0cf5a-fe35-e038-a78e-e87caef03780@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 7735 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
next prev parent reply other threads:[~2025-12-03 0:47 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 ` [PATCH 0/3] Sanitize sideband channel messages brian m. carlson
2025-01-16 6:45 ` 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 [this message]
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=aS-D5lD2Kk6BHNIl@fruit.crustytoothpaste.net \
--to=sandals@crustytoothpaste$(echo .)net \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
/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