Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs

From: Jeff King <peff@peff.net>
Date: Jan 10, 2024, 10:23 UTC
Message-ID: <20240110102338.GA16674@coredump.intra.peff.net>
In-Reply-To: <pull.1635.git.1704822817.gitgitgadget@gmail.com>

On Tue, Jan 09, 2024 at 05:53:34PM +0000, Victoria Dye via GitGitGadget wrote:

Show 5 quoted lines

> While testing 'git fsck' checks on .gitmodules URLs, I noticed that some
> invalid URLs were passing the checks. Digging into it a bit more, the issue
> turned out to be that 'credential_from_url_gently()' parses certain URLs
> (like "http://example.com:something/deeper/path") incorrectly, in a way that
> appeared to return a valid result.

I don't think that checks was ever intended to be an overall URL-quality check. The reason we used the credential code in the fsck check is that we were checking for URLs which triggered a specific credential-related vulnerability.

I don't mind tightening things further as long as:

  1. We are not allowing any cases that the credential code would have
     forbidden (i.e., something that might let the vulnerability slip
     through, since ultimately it is the credential code which will need
     to be protected). You ported over the newline check, which is the
     main thing. It's possible that there is some difference between the
     two parsers that may allow an invalid input to create a newline for
     one but not the other, but having now looked over the code, I don't
     think so.

     And I think one could argue that the security-importance of the
     fsck check has mostly run its course. The real fix was in the
     credential code itself, and the matching fsck change was mostly
     about protecting downstream clients until they were upgraded. Now
     that it's been several years, there's not as much value there.

  2. It is not making it harder for users to work with repositories that
     may contain malformed URLs that _aren't_ vulnerabilities. It sounds
     like the specific cases you found already don't work at all with
     Git, so presumably nobody is using them. By making it an fsck
     check, though, any mistakes that are embedded in history (even if
     they are now corrected) will make it a pain to use the repository
     with sites that enable transfer.fsckObjects.

     My gut feeling is that this is probably OK in practice. If it does
     cause pain, we might consider loosening the fsck.gitmodulesUrl
     severity (under the notion from above that it is no longer a
     critical security check). But if it doesn't cause real-world pain,
     being pickier is probably better (it may save us from a
     vulnerability down the road).

-Peff