| Message ID | pull.1853.git.1736878772.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Sanitize sideband channel messages | expand |
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.
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