Preparing for a Git 2.26.3 release

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/

On Mon, Apr 27, 2020 at 10:55 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

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

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

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.

Junio C Hamano wrote:

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:

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

Thanks, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Hi Junio,

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

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

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

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.