Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 10, 2024, 06:33 UTC
Message-ID: <ZZ46MrjSocJl-kpU@tanuki>
In-Reply-To: <893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> From: Victoria Dye <vdye@github.com>
> 
> Update the validation of "curl URL" submodule URLs (i.e. those that specify
> an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
> invalid URLs. The existing validation using 'credential_from_url_gently()'
> parses certain URLs incorrectly, leading to invalid submodule URLs passing
> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
> 'credential_from_url_gently()'.

Okay, so we retain the wrong behavior of `credential_from_url_gently()`, right? I wonder whether this can be abused in any way, doubly so because the function gets invoked with untrusted input from the remote server when we handle redirects in `http_request_reauth()`. But the redirect URL we end up passing to `credential_from_url_gently()` would have to contain a non-numeric port, and curl seemingly does not know to handle those either.

Other callsites include fsck (which you're fixing) and the credential store (which is entirely user-controlled). It would be great regardless to fix the underlying bug in `credential_from_url_gently()` eventually though. But I do not think that this has to be part this patch series here, which is a strict improvement.

Thanks!

Patrick