| Message ID | 20240204185427.39664-3-ypsah@devyard.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix gitlab's token-based authentication w/ kerberos | expand |
On 2024-02-04 at 18:54:27, 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 actually contains credentials itself. I don't think your proposed change is safe. Credentials are supposed to be tied to a certain site and may even be tied to a specific repository, and if there's a redirect, then we need to re-fetch credentials or we could leak credentials to the wrong site by reusing them. Your change would therefore introduce a security vulnerability. I should also point out that in general we are trying to make it less easy and less convenient for people to use credentials in the URL because that always necessitates insecure storage. There have in fact been proposals to remove that functionality entirely.
Quentin Bouget <ypsah@devyard.org> writes: > 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 actually contains credentials itself. "This commit makes it so" -> "Make it so" > + char *username = NULL, *password = NULL; > + > + if (http_auth.username) > + username = xstrdup(http_auth.username); > + if (http_auth.password) > + password = xstrdup(http_auth.password); Not a huge deal, but we have xstrdup_or_null() helper function exactly for a use case like this. > credential_from_url(&http_auth, options->base_url->buf); > + > + 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; This is an interesting change. I wonder what breaks if we completely ignored such credential materials forced by the remote via a redirect? > url = options->effective_url->buf; > } > } Thanks.
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 >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); >+ >+ 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
On Sun Feb 4, 2024 at 11:36 PM CET, brian m. carlson wrote: > On 2024-02-04 at 18:54:27, 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 actually contains credentials itself. > > I don't think your proposed change is safe. Credentials are supposed to > be tied to a certain site and may even be tied to a specific repository, > and if there's a redirect, then we need to re-fetch credentials or we > could leak credentials to the wrong site by reusing them. Your change > would therefore introduce a security vulnerability. Good point, I had not considered the security implications. I can see libcurl only reuses credentials after a redirect if the hostname has not changed: [1] By default, libcurl only sends credentials and Authentication headers to the initial hostname as given in the original URL, to avoid leaking username + password to other sites. Does it sound OK if I use the credentials provided by the redirect when there are any (out of consistency with the current implementation), and only allow reusing the current credentials when the redirect and the original URLs share the same hostname? If so, is there a helper to extract the hostname out of a URL in git? I can see credential.c does this itself, otherwise, libcurl provides its own helpers for this (curl_url(), curl_url_set(), curl_url_get()). [2] > I should also point out that in general we are trying to make it less > easy and less convenient for people to use credentials in the URL > because that always necessitates insecure storage. There have in fact > been proposals to remove that functionality entirely. Apologies, I feel like I may have given the impression I wanted to configure credentials in git's configuration files, which is not the case. My use case is to `git push` a tag from a CI/CD pipeline to trigger a release, similar to how I do it here. [3] Or is this the kind of use case you are trying to discourage? [1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html [2] https://curl.se/libcurl/c/curl_url_get.html [3] https://gitlab.com/ypsah/simple-git-versioning/-/blob/main/.gitlab-ci.yml?ref_type=heads#L110
On Sun Feb 4, 2024 at 11:51 PM CET, Junio C Hamano wrote: > Quentin Bouget <ypsah@devyard.org> writes: > > > 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 actually contains credentials itself. > > "This commit makes it so" -> "Make it so" Will change. > > + char *username = NULL, *password = NULL; > > + > > + if (http_auth.username) > > + username = xstrdup(http_auth.username); > > + if (http_auth.password) > > + password = xstrdup(http_auth.password); > > Not a huge deal, but we have xstrdup_or_null() helper function > exactly for a use case like this. Thanks, will change. > > credential_from_url(&http_auth, options->base_url->buf); > > + > > + 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; > > This is an interesting change. I wonder what breaks if we > completely ignored such credential materials forced by the remote > via a redirect? Me too. Maybe the original author would know. Is it OK to Cc them in this case? > > url = options->effective_url->buf; > > } > > } Thanks, Quentin
On Mon Feb 5, 2024 at 12:01 AM CET, wrote: > 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 > > 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); > > + > > + 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. I guess this can be controlled by setting http.followRedirects to false. I am not sure there is generally more danger in following a redirect URL with or without the provided credentials. Note that this is also how git currently behaves and while I am not aware of any concrete use case myself, I am also not confident there isn't any. That being said, Brian M. Carlson pointed out that reusing credentials from the initial URL for the redirect URL is quite dangerous. In my reply, I have suggested to switch to implementing the same behaviour as libcurl when it comes to reusing credentials: if the hostname of the redirect is the same as the original URL, reuse the credentials, otherwise drop them. [1] I would still retain the (seemingly strange) current behaviour of favoring credentials in the redirect URL over anything else. Thanks, Quentin [1] https://curl.se/libcurl/c/CURLOPT_UNRESTRICTED_AUTH.html
Hi Quentin,
> I have suggested to switch to implementing the same behaviour as libcurl when it comes to reusing credentials: if the hostname of the redirect is the same as the original URL, reuse the credentials, otherwise drop them.
The protocol & port number also need to match, so it doesn't end up
the same as CVE-2022-27774 [1]
Rob :)
[1] https://curl.se/docs/CVE-2022-27774.html
On 2024-02-05 at 03:01:17, Quentin Bouget wrote: > Good point, I had not considered the security implications. > > I can see libcurl only reuses credentials after a redirect if the > hostname has not changed: [1] > > By default, libcurl only sends credentials and Authentication > headers to the initial hostname as given in the original URL, to > avoid leaking username + password to other sites. > > Does it sound OK if I use the credentials provided by the redirect when > there are any (out of consistency with the current implementation), and > only allow reusing the current credentials when the redirect and the > original URLs share the same hostname? I don't think we can actually rely on that functionality because `credential.usehttppath` could actually have been set, in which case we'd need a different credential. For example, I know some forges issue certain types of tokens that are tied to a specific URL and wouldn't validate for a redirect, even if it were actually the same repo. If there are credentials in the URL provided by the redirect, I think it should be safe to use them; otherwise, we'd need to rely on filling them with the credential protocol. > Apologies, I feel like I may have given the impression I wanted to > configure credentials in git's configuration files, which is not the > case. > > My use case is to `git push` a tag from a CI/CD pipeline to trigger a > release, similar to how I do it here. [3] > > Or is this the kind of use case you are trying to discourage? We're trying to discourage all use of credentials in the URL at the command line and in remote names/configuration files. If you want to pass in credentials from the environment, the Git FAQ explains how to do that[0], and that technique can be used in such a situation. [0] https://git-scm.com/docs/gitfaq#http-credentials-environment
On Monday, February 5, 2024 5:18 PM, brian m. carlson wrote: >On 2024-02-05 at 03:01:17, Quentin Bouget wrote: >> Good point, I had not considered the security implications. >> >> I can see libcurl only reuses credentials after a redirect if the >> hostname has not changed: [1] >> >> By default, libcurl only sends credentials and Authentication >> headers to the initial hostname as given in the original URL, to >> avoid leaking username + password to other sites. >> >> Does it sound OK if I use the credentials provided by the redirect >> when there are any (out of consistency with the current >> implementation), and only allow reusing the current credentials when >> the redirect and the original URLs share the same hostname? > >I don't think we can actually rely on that functionality because >`credential.usehttppath` could actually have been set, in which case we'd need a >different credential. For example, I know some forges issue certain types of tokens >that are tied to a specific URL and wouldn't validate for a redirect, even if it were >actually the same repo. > >If there are credentials in the URL provided by the redirect, I think it should be safe >to use them; otherwise, we'd need to rely on filling them with the credential >protocol. > >> Apologies, I feel like I may have given the impression I wanted to >> configure credentials in git's configuration files, which is not the >> case. >> >> My use case is to `git push` a tag from a CI/CD pipeline to trigger a >> release, similar to how I do it here. [3] >> >> Or is this the kind of use case you are trying to discourage? > >We're trying to discourage all use of credentials in the URL at the command line and >in remote names/configuration files. If you want to pass in credentials from the >environment, the Git FAQ explains how to do that[0], and that technique can be >used in such a situation. > >[0] https://git-scm.com/docs/gitfaq#http-credentials-environment A common side-use case (not directly in git) for this situation is to attempt to use curl (or libcurl) to create a Pull Request via the GitHub (or other enterprise git server) CLI or POST. This is most often done via REST rather than supplying via the URL. It does remove the need to pass some credentials (a.k.a. the API token) via the URL as the API token gets injected into the JSON content - this may have been the original motivation as many of the servers do redirects. However, they do not reprocess or inject different credentials. I am wonder about the specific use case is for this situation and why a redirect injects a credential change, which I cannot see is a good thing. --Randall
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); + + 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; } }
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 actually contains credentials itself. Signed-off-by: Quentin Bouget <ypsah@devyard.org> --- http.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)