Re: [PATCH v4 0/6] Sanitize sideband channel messages

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Git's sideband channel passes server output directly to the client terminal
> without sanitizing it, creating an ANSI escape sequence injection
> vulnerability (CWE-150 [https://cwe.mitre.org/data/definitions/150.html]). A
> malicious or compromised server can corrupt terminal state, obscure
> information, or inject characters into the terminal's input buffer (which
> the terminal will interpret as if the user had typed them).
>
> Git users have no mechanism to distinguish between Git's legitimate output
> and content displayed (or hidden) via attack sequences.
>
> This series aims to fix the vulnerability by sanitizing control characters
> in the sideband output. To address concerns about existing hacks that
> exploit Git's lack of sanitizing, this security fix will only kick in with
> Git v3.0, where the default will change to pass ANSI color sequences (SGR
> codes) 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 will be rendered in caret notation (e.g., ESC
> becomes ^[).
>
> Users who need different behavior get configuration options:
> sideband.allowControlCharacters provides an escape hatch for environments
> that require raw passthrough. The defaults in Git v3.0 will be secure.
>
> This series applies cleanly on v2.53.0.

Thanks for an updated series.  They applied cleanly and merged
cleanly to 'seen'.

I have three problems with this round, some minor, some fundamental.

 * There is a value "default" that the user can use to configure it,
   which changes its behaviour across Git 3.0 version boundary.  I
   think this is a horrible design.  Those who do not configure may
   be showing their acceptance "I can go with whatever the tool's
   designers choose with the option, and if that changes in the
   middle, I can adapt", but for those who do, the configuration is
   a mechanism, an escape hatch, to express their preference and
   avoid getting disrupted by future change of behaviour.

   Fixing it is easy here; just remove "default".  Those who want to
   live in the fiture earlycan set it to "color" and it will stay
   valid across Git 3.0 version boundary.  Those who want to make
   sure their control sequences won't be broken can set it to "pass
   everything" and it will stay valid across Git 3.0 version
   boundary.

 * I would have preferred to see the early parts of the series all
   being opt-in, and that subset of the series be able to graduate
   earlier.  Way earlier than the default flip to prove that they do
   not hurt when unconfigured (they are theoretically no-op while
   being opt-in, but we want to make sure), and that they do help
   when configured.  And then once we are satisfied, the default
   flip can be discussed and applied.

 * The overall approach is "we know better than our users what
   control sequences we want to pass, so we will write code to
   recognize these small number of control sequences and allow
   them", which I am worried that would put more users at risk.

   But the thing is that our code may not know better than the users
   what control sequences, which have little security implications,
   users want to use in the payload between their servers and their
   desktop clients.  What happens to these users who use the control
   sequences that the code does not recognize and still want to keep
   using them?  They have to either

     (1) write code to teach git to recognize their control
         sequences (perhaps they do want ISO/IEC 2022 passed) and
         tweak the allowlist mechanism to support it, or

     (2) use the allowlist mechanism to pass everything, even the
         sequences they are not interested in passing that have
         security implications.

   Most of them I suspect will do the latter, which is not what we
   want to see.

   Can't we do this the other way around?  The code may not be able
   to know what control sequences the users may want to use better
   than the users, but the code (and the author of the code) should
   know what control sequences have negative security implications
   much better than the users.  Instead of teaching the code to
   recognize control sequences to color strings [*} that have little
   security implications, can we teach the code to recognise control
   sequences that we do *not* want to pass and neuter them, without
   molesting control sequences with little security implications
   that users may want to use?

Thanks.


[Footnote]

 * or sequences to go multi-lingual over 7-bit by using ISO/IEC
   2022 ;-)