[PATCH 0/3] Sanitizing sideband output

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

The topic has been waiting for an action but haven't seen any.  Here
is a follow-up message in an attempt to unblock.

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

... among which I already retracted one.

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

The patch [1/3] in this series addresses this issue.  This is to be
applied on top of Dscho's [5/6].

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

This is what I already retracted.  The structure of the posted
series to start extra tight and then tighten the default with the
last patch is actually good for experiment.  We can merge all the
steps without the "Loosen until Git 3.0 and then tighten Git 3.0
with WITH_BREAKING_CHANGES option" patch to 'next' and see who
screams.  Once it proves to be not too bad, we can then merge that
last step also to 'next' and make sure the loosened state still
works as expected.

So, the plan is to merge the early part of the series, INCLUDING the
"do not introduce the value 'default' that will change its meaning
in the configuration file" step, to 'next', while holding the last
"Loosen until Git 3.0" step back.  The last step in Dscho's series
[6/6] has to be adjusted because of minor textual conflicts with the
"do not introduce the value 'default'" step, so a rebased version of
it appears as [2/3] in this series.

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

And I cannot do this part myself, as the posted series does not shed
any light what uncertainty we fear in the bytestream coming from the
other side.  If we know what malicious data we want to protect
against, we can surgically and specifically protect against them,
but there is no hint in the posted series X-<.


Summary of patches in this series:

 * [1/3] sideband: drop 'default' configuration

   Remove the value 'default' from the allowed values for the
   sideband.allowControlCharacters configuration variable.  Applies
   on top of Dscho's [5/6].

 * [2/3] sideband: delay sanitizing by default to Git v3.0

   Dscho's [6/6] rebased on top of [1/3] of this series.

 * [3/3] sideband: conditional documentation fix

   Documentation mark-up update to help translators with the
   conditional documentation added in [2/3].


 Documentation/config/sideband.adoc  | 13 ++++++++++---
 sideband.c                          | 10 ++++++----
 t/t5409-colorize-remote-messages.sh | 18 +++++++++++++-----
 3 files changed, 29 insertions(+), 12 deletions(-)

-- 
2.53.0-549-g863838a955