Preparing for a Git 2.26.3 release

Preparing for a Git 2.26.3 release

[Jonathan Nieder]

Hi,

Last week and the week before Git released 2.26.1 and 2.26.2 in quick
succession, to address some security issues (CVE-2020-5260 and
CVE-2020-11008).

Since then we've heard about a few related (non-security) regressions.
I'd like to avoid giving people an excuse not to upgrade, so this
morning[1] I promised a discussion of what I'd like to see in a 2.26.3
release to help with that.

credential_from_url fallout
---------------------------
The credential_from_url hardening affected http.c in the way we'd
like, but it also affected credential-store[2].  We may want to relax
that.

(only affects 2.25.y and earlier) The credential_from_url hardening
also affected credential.<urlpattern>.* parsing[3].  Depending on what
semantics we decide on for those earlier releases, we are likely to
want 2.26.y to behave the same way[4].

2.25.y -> 2.26.0 regressions?
-----------------------------
The main major changes from 2.25.y to 2.26.y were the change of
default rebase backend and protocol.version defaulting to 2.

I don't believe there are any major outstanding issues with the
change in rebase backend, but I'm cc-ing Elijah Newren to get a chance
to be corrected :).

The protocol version change was painful for users that fetch in the
same repo from linux-next and other linux remotes[5].  The problem has
been isolated and fixed, so we could either apply the revert or apply
the fixes[6].

credential helper hardening
---------------------------
Lastly, after seeing a v2.26.1 security release and v2.26.2 security
release in quick succession, I'm looking at what it would take to make
Git more robust so we don't end up need .3, .4, .5 security releases
soon after.  Perhaps we can make the credential system a bit more
robust to prevent future similar accidents.  For example:

- teach in-tree credential helpers to reject repeated fields in their
  input (Just In Case some variant of newline injection that uses \r
  or something pops up)

- teach in-tree credential helpers to require the host and protocol
  fields to be set (e.g., [7])

- update git-credential(1) to document the newly tightened protocol

That might go against the goal of getting rid of excuses not to
upgrade, though.  Where we see regression potential, we can be patient
and wait for 2.27, but if we have some examples with particularly low
regression potential then they might be okay 2.26.3 fodder as well.

Thoughts?

Thanks,
Jonathan

[1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2020-04-27#l65
[2] https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/
[3] https://lore.kernel.org/git/pull.615.git.1587588665.gitgitgadget@gmail.com/
[4] https://lore.kernel.org/git/pull.620.git.1587767749.gitgitgadget@gmail.com/#t
[5] https://lore.kernel.org/git/20200422155047.GB91734@google.com/
[6] https://lore.kernel.org/git/cover.1588031728.git.jonathantanmy@google.com/
[7] https://lore.kernel.org/git/20200420224310.9989-1-carenas@gmail.com/

Re: Preparing for a Git 2.26.3 release

[Elijah Newren]

On Mon, Apr 27, 2020 at 10:55 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Last week and the week before Git released 2.26.1 and 2.26.2 in quick
> succession, to address some security issues (CVE-2020-5260 and
> CVE-2020-11008).
>
> Since then we've heard about a few related (non-security) regressions.
> I'd like to avoid giving people an excuse not to upgrade, so this
> morning[1] I promised a discussion of what I'd like to see in a 2.26.3
> release to help with that.
>
> credential_from_url fallout
> ---------------------------
> The credential_from_url hardening affected http.c in the way we'd
> like, but it also affected credential-store[2].  We may want to relax
> that.
>
> (only affects 2.25.y and earlier) The credential_from_url hardening
> also affected credential.<urlpattern>.* parsing[3].  Depending on what
> semantics we decide on for those earlier releases, we are likely to
> want 2.26.y to behave the same way[4].
>
> 2.25.y -> 2.26.0 regressions?
> -----------------------------
> The main major changes from 2.25.y to 2.26.y were the change of
> default rebase backend and protocol.version defaulting to 2.
>
> I don't believe there are any major outstanding issues with the
> change in rebase backend, but I'm cc-ing Elijah Newren to get a chance
> to be corrected :).

I'm not aware of any outstanding (by which I think you mean
"unsolved") issues, but there are four short topics fixing regressions
in master since v2.26.x (namely:
ag/rebase-merge-allow-ff-under-abbrev-command,
en/pull-do-not-rebase-after-fast-forwarding,
en/sequencer-reflog-action, and en/rebase-no-keep-empty).  Some or all
of those topics might make sense to include in a stable release if
you're considering fixes other than security updates.

> The protocol version change was painful for users that fetch in the
> same repo from linux-next and other linux remotes[5].  The problem has
> been isolated and fixed, so we could either apply the revert or apply
> the fixes[6].
>
> credential helper hardening
> ---------------------------
> Lastly, after seeing a v2.26.1 security release and v2.26.2 security
> release in quick succession, I'm looking at what it would take to make
> Git more robust so we don't end up need .3, .4, .5 security releases
> soon after.  Perhaps we can make the credential system a bit more
> robust to prevent future similar accidents.  For example:
>
> - teach in-tree credential helpers to reject repeated fields in their
>   input (Just In Case some variant of newline injection that uses \r
>   or something pops up)
>
> - teach in-tree credential helpers to require the host and protocol
>   fields to be set (e.g., [7])
>
> - update git-credential(1) to document the newly tightened protocol
>
> That might go against the goal of getting rid of excuses not to
> upgrade, though.  Where we see regression potential, we can be patient
> and wait for 2.27, but if we have some examples with particularly low
> regression potential then they might be okay 2.26.3 fodder as well.
>
> Thoughts?
>
> Thanks,
> Jonathan
>
> [1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2020-04-27#l65
> [2] https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/
> [3] https://lore.kernel.org/git/pull.615.git.1587588665.gitgitgadget@gmail.com/
> [4] https://lore.kernel.org/git/pull.620.git.1587767749.gitgitgadget@gmail.com/#t
> [5] https://lore.kernel.org/git/20200422155047.GB91734@google.com/
> [6] https://lore.kernel.org/git/cover.1588031728.git.jonathantanmy@google.com/
> [7] https://lore.kernel.org/git/20200420224310.9989-1-carenas@gmail.com/

proto v2 fixes for maint (was Re: Preparing for a Git 2.26.3 release)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Since then we've heard about a few related (non-security) regressions.
> I'd like to avoid giving people an excuse not to upgrade, so this
> morning[1] I promised a discussion of what I'd like to see in a 2.26.3
> release to help with that.

Thanks for starting this.  

I'll have chances to comment on other areas you listed, but since
I've answered on v2-proto stuff to somebody else already...

> The protocol version change was painful for users that fetch in the
> same repo from linux-next and other linux remotes[5].  The problem has
> been isolated and fixed, so we could either apply the revert or apply
> the fixes[6].

The demote patch hasn't even hit 'master'.  

My preference is to merge the demotion down to 'master' and 'maint'
while merging down this fix to 'next' and to 'master'.

And immediately revert the demotion on 'master', which will make the
tip of 'master' with v2 as the default, with "this" fix.

That way, those who want to help us polish the code further for the
next release would use v2 as default with the proposed fix for this
breakage and can hunt for other breakages in v2, while those on the
maintenance track (and v2.26.3 JNeider wants to see happen soon)
would revert to the original protocol as default.

In short, my preference is to ship 2.26.3 with the "demote v2 from
default", and hopefully try 2.27 with "v2 with negotiation fix" and
hope people won't find any other remaining glitches in 2.27.  After
that, we may want to merge the negotiation fix down to 2.26.x track
but I am not comfortable merging it in a release on the maintenance
track with the timeframe we seem to be talking about (i.e. a few
weeks, presumably).

Re: proto v2 fixes for maint (was Re: Preparing for a Git 2.26.3 release)

[Michal Suchánek]

On Tue, 28 Apr 2020 10:25:09 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Since then we've heard about a few related (non-security) regressions.
> > I'd like to avoid giving people an excuse not to upgrade, so this
> > morning[1] I promised a discussion of what I'd like to see in a 2.26.3
> > release to help with that.  
> 
> Thanks for starting this.  
> 
> I'll have chances to comment on other areas you listed, but since
> I've answered on v2-proto stuff to somebody else already...
> 
> > The protocol version change was painful for users that fetch in the
> > same repo from linux-next and other linux remotes[5].  The problem has
> > been isolated and fixed, so we could either apply the revert or apply
> > the fixes[6].  
> 
> The demote patch hasn't even hit 'master'.  
> 
> My preference is to merge the demotion down to 'master' and 'maint'
> while merging down this fix to 'next' and to 'master'.

I understand that this issue puts v2 protocol into question and you
want to go back to v0 as default. Why not merge the fixes for v2,
though? Even if it is not the default keeping it broken in maint does
not sound great.

Thanks

Michal

Re: proto v2 fixes for maint (was Re: Preparing for a Git 2.26.3 release)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In short, my preference is to ship 2.26.3 with the "demote v2 from
> default", and hopefully try 2.27 with "v2 with negotiation fix" and
> hope people won't find any other remaining glitches in 2.27.  After
> that, we may want to merge the negotiation fix down to 2.26.x track
> but I am not comfortable merging it in a release on the maintenance
> track with the timeframe we seem to be talking about (i.e. a few
> weeks, presumably).

The last part needs clarification.  What I am hesitant is to merge
only the "negotiation fix" in 2.26.3 while keeping v2 the default.
As long as v2 is not the default with the "demote" patch, I am OK
to have the "negotiation fix" in there, too.

Re: proto v2 fixes for maint (was Re: Preparing for a Git 2.26.3 release)

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> The protocol version change was painful for users that fetch in the
>> same repo from linux-next and other linux remotes[5].  The problem has
>> been isolated and fixed, so we could either apply the revert or apply
>> the fixes[6].
>
> The demote patch hasn't even hit 'master'.
>
> My preference is to merge the demotion down to 'master' and 'maint'
> while merging down this fix to 'next' and to 'master'.
>
> And immediately revert the demotion on 'master', which will make the
> tip of 'master' with v2 as the default, with "this" fix.

Yes, sounds good to me.

By the way, the diagnosis in the demotion patch

    Users fetching from linux-next and other kernel remotes are reporting
    that the limited ref advertisement causes negotiation to reach
    MAX_IN_VAIN, resulting in too-large fetches.

turned out to be false, as Peff noticed.  It wasn't due to the ref
advertisement but was due to the protocol v2 code's reimplementation
of negotiation.  It's probably not worth amending the commit message,
given that it's already in "next"; we can correct the record in the
revert for 'master', which I should probably write during the day when
I am less likely to make more errors.

Michal Suchánek wrote:

>                                   Why not merge the fixes for v2,
> though? Even if it is not the default keeping it broken in maint does
> not sound great.

Yes, I like the "demote and fix" approach you're recommending.

Thanks,
Jonathan

Re: Preparing for a Git 2.26.3 release

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Since then we've heard about a few related (non-security) regressions.
> I'd like to avoid giving people an excuse not to upgrade, so this
> morning[1] I promised a discussion of what I'd like to see in a 2.26.3
> release to help with that.

I will be pushing out an update to 'master' shortly, which merges
more than a handful of features that could go to 2.26.x track.  We
obviously won't be merging all of them in 2.26.3, so let me try to
give an excerpt from the draft release notes on the 'master' front.

I did not limit the following list to the ones that are related to
the recent CVEs, but I excluded many of them to limit the size of
2.26.3.

Please help to further shrink the scope of 2.26.3 by nominating ones
that should be omitted.  The absolute minimum would be just the ones
related to the credential, I guess.

 * Those fetching over protocol v2 from linux-next and other kernel
   repositories are reporting that v2 often fetches way too much than
   needed.
   (merge 11c7f2a30b jn/demote-proto2-from-default later to maint).

 * The upload-pack protocol v2 gave up too early before finding a
   common ancestor, resulting in a wasteful fetch from a fork of a
   project.  This has been corrected to match the behaviour of v0
   protocol.
   (merge 2f0a093dd6 jt/v2-fetch-nego-fix later to maint).

 * The server-end of the v2 protocol to serve "git clone" and "git
   fetch" was not prepared to see a delim packets at unexpected
   places, which led to a crash.
   (merge cacae4329f jk/harden-protocol-v2-delim-handling later to maint).

 * The more aggressive updates to remote-tracking branches we had for
   the past 7 years or so were not reflected in the documentation,
   which has been corrected.
   (merge a44088435c pb/pull-fetch-doc later to maint).

 * We've left the command line parsing of "git log :/a/b/" broken for
   about a full year without anybody noticing, which has been
   corrected.
   (merge 0220461071 jc/missing-ref-store-fix later to maint).

 * Misc fixes for Windows.
   (merge 3efc128cd5 js/mingw-fixes later to maint).

 * Parsing the host part out of URL for the credential helper has been corrected.
   (merge 4c5971e18a jk/credential-parsing-end-of-host-in-URL later to maint).

 * Validation of push certificate has been made more robust against
   timing attacks.
   (merge 719483e547 bc/constant-memequal later to maint).

 * Raise the minimum required version of docbook-xsl package to 1.74,
   as 1.74.0 was from late 2008, which is more than 10 years old, and
   drop compatibility cruft from our documentation suite.
   (merge 3c255ad660 ma/doc-discard-docbook-xsl-1.73 later to maint).

 * The build procedure did not use the libcurl library and its include
   files correctly for a custom-built installation.
   (merge 0573831950 jk/build-with-right-curl later to maint).

 * Recent update to Homebrew used by macOS folks breaks build by
   moving gettext library and necessary headers.
   (merge a0b3108618 ds/build-homebrew-gettext-fix later to maint).

 * Error and verbose trace messages from "git push" did not redact
   credential material embedded in URLs.
   (merge d192fa5006 js/anonymise-push-url-in-errors later to maint).

 * Update the parser used for credential.<URL>.<variable>
   configuration, to handle <URL>s with '/' in them correctly.
   (merge b44d0118ac bc/wildcard-credential later to maint).

 * Recent updates broke parsing of "credential.<url>.<key>" where
   <url> is not a full URL (e.g. [credential "https://"] helper = ...)
   stopped working, which has been corrected.
   (merge 9a121b0d22 js/partial-urlmatch-2.17 later to maint).
   (merge cd93e6c029 js/partial-urlmatch later to maint).

 * With the recent tightening of the code that is used to parse
   various parts of a URL for use in the credential subsystem, a
   hand-edited credential-store file causes the credential helper to
   die, which is a bit too harsh to the users.  Demote the error
   behaviour to just ignore and keep using well-formed lines instead.
   (merge c03859a665 cb/credential-store-ignore-bogus-lines later to maint).

 * The samples in the credential documentation has been updated to
   make it clear that we depict what would appear in the .git/config
   file, by adding appropriate quotes as needed..
   (merge 177681a07e jk/credential-sample-update later to maint).

 * The <stdlib.h> header on NetBSD brings in its own definition of
   hmac() function (eek), which conflicts with our own and unrelated
   function with the same name.  Our function has been renamed to work
   around the issue.
   (merge 3013118eb8 cb/avoid-colliding-with-netbsd-hmac later to maint).

Re: Preparing for a Git 2.26.3 release

[Johannes Schindelin]

Hi Junio,

On Fri, 8 May 2020, Junio C Hamano wrote:

>  * Recent updates broke parsing of "credential.<url>.<key>" where
>    <url> is not a full URL (e.g. [credential "https://"] helper = ...)
>    stopped working, which has been corrected.
>    (merge 9a121b0d22 js/partial-urlmatch-2.17 later to maint).
>    (merge cd93e6c029 js/partial-urlmatch later to maint).

Are you planning on also releasing, say, v2.23.4, with this fix? I ask
because GitHub Desktop currently bundles Git v2.23.x, and while we fixed
the breakages on Windows, they are still present on macOS and Linux (for
details, see https://github.com/desktop/desktop/issues/9597).

Thanks,
Dscho

Re: Preparing for a Git 2.26.3 release

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 8 May 2020, Junio C Hamano wrote:
>
>>  * Recent updates broke parsing of "credential.<url>.<key>" where
>>    <url> is not a full URL (e.g. [credential "https://"] helper = ...)
>>    stopped working, which has been corrected.
>>    (merge 9a121b0d22 js/partial-urlmatch-2.17 later to maint).
>>    (merge cd93e6c029 js/partial-urlmatch later to maint).
>
> Are you planning on also releasing, say, v2.23.4, with this fix?

Yes, some subset of the changes we will apply to 2.26.3 probably can
be backported down to older maintenance releases.  

Our usual policy is not to backport beyond two or three maintenance
tracks unless it is a security fix, and because the current maint
track is 2.26.x, which means 2.25.x and possibly 2.24.x series are
the oldest candidates.

If you think it makes sense to start the discussion to choose that
absolute minimum set, I am fine with that approach, too.  The
partial-urlmatch topic was designed to be usable with tracks as old
as 2.17.x series, so I am OK to go a bit further back but if we are
to do so, how far back should we as the upstream go back?  I am OK
to say we make the 2.23 series as the oldest for this round.

And then if we were to draw the line there, what other changes in
the set do we want to include the 2.{23,24,25,26}.x maintenance
releases?  That would certainly be a narrower subset than the list 
in the message you are responding to.

Thanks.