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

Hi Junio,

On Wed, 15 Jan 2025, Junio C Hamano 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.

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.

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.

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