RE: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Date: Feb 4, 2024, 23:01 UTC
Message-ID: <017a01da57be$28748aa0$795d9fe0$@nexbridge.com>
In-Reply-To: <20240204185427.39664-3-ypsah@devyard.org>

On Sunday, February 4, 2024 1:54 PM, Quentin Bouget wrote:

>During a re-authentication (second attempt at authenticating with a remote,

e.g.

>after a failed GSSAPI attempt), git allows the remote to provide credential

overrides

>in the redirect URL and unconditionnaly drops the current HTTP credentials

in favors

>of those, even when there aren't any.
>
>This commit makes it so HTTP credentials are only overridden when the

redirect URL

Show 23 quoted lines

>actually contains credentials itself.
>
>Signed-off-by: Quentin Bouget <ypsah@devyard.org>
>---
> http.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/http.c b/http.c
>index ccea19ac47..caba9cac1e 100644
>--- a/http.c
>+++ b/http.c
>@@ -2160,7 +2160,25 @@ static int http_request_reauth(const char *url,
> 	if (options && options->effective_url && options->base_url) {
> 		if (update_url_from_redirect(options->base_url,
> 					     url, options->effective_url)) {
>+			char *username = NULL, *password = NULL;
>+
>+			if (http_auth.username)
>+				username = xstrdup(http_auth.username);
>+			if (http_auth.password)
>+				password = xstrdup(http_auth.password);
>+
> 			credential_from_url(&http_auth,

options->base_url->buf);

Show 14 quoted lines

>+
>+			if (http_auth.username)
>+				free(username);
>+			else if (username)
>+				http_auth.username = username;
>+
>+			if (http_auth.password)
>+				free(password);
>+			else if (password)
>+				http_auth.password = password;
>+
> 			url = options->effective_url->buf;
> 		}
> 	}

I am wondering whether this is a good idea. Having credentials in a redirect seems like it might be a vector for going somewhere other than what you want to do, with credentials you do not necessarily want. Others might no better than I on this, but would potentially lead to a CVE? I would prefer to see credentials in a redirect rejected rather than used. --Randall