# [PATCH 0/1] http: Fix crash when passing malformed URL 4 messages from 2016-03-16 to 2016-03-16. Participants: Anton Wuerfel, Jeff King. Thread: https://gitlist.dev/t/41703 ## Anton Wuerfel, 2016-03-16 10:54 Subject: [PATCH 0/1] http: Fix crash when passing malformed URL Message-ID: <1458125647-32707-1-git-send-email-anton.wuerfel@fau.de> URL: https://gitlist.dev/e/1458125647-32707-1-git-send-email-anton.wuerfel%40fau.de ``` When passing a malformed URL to http_init() in http.c, git dies from a null pointer dereference. An example for a malformed URL is http:/git-scm.com (note the single slash after the protocol). I could not reproduce this error within any functions of git - I just noticed it during development of a git extension together with Phillip Raffeck. Anton Wuerfel (1): http: Fix crash when passing malformed URL http.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.8.0.rc1.108.g7827469 ``` ## Anton Wuerfel, 2016-03-16 10:54 Subject: [PATCH 1/1] http: Fix crash when passing malformed URL Message-ID: <1458125647-32707-2-git-send-email-anton.wuerfel@fau.de> URL: https://gitlist.dev/e/1458125647-32707-2-git-send-email-anton.wuerfel%40fau.de In-Reply-To: <1458125647-32707-1-git-send-email-anton.wuerfel@fau.de> ``` When passing a malformed URL to http_init() in http.c, git dies from a null pointer dereference. An example for a malformed URL is http:/git-scm.com (note the single slash after the protocol). This patch adds simple error handling as git notices the malformed URL already, but never checks the error value. When passing a malformed URL, credential_from_url(struct credential *c, const char *url) initializes *c with null values. When the existence of `://` in url is checked, the function returns without further change of *c. The null pointer dereference occurs in get_curl_handle () at http.c:593, when the `protocol` field of struct credential is strcmp'ed: Program received signal SIGSEGV, Segmentation fault. 0x0000000000405efd in get_curl_handle () at http.c:593 593 if (!strcmp(http_auth.protocol, "https")) { --- http.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/http.c b/http.c index 69da445..80cf752 100644 --- a/http.c +++ b/http.c @@ -660,6 +660,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) http_is_verbose = 0; normalized_url = url_normalize(url, &config.url); + + if (config.url.err) + die(_("libcurl: %s, URL: %s"), config.url.err, url); git_config(urlmatch_config_entry, &config); free(normalized_url); -- 2.8.0.rc1.108.g7827469 ``` ## Jeff King, 2016-03-16 21:29 Subject: Re: [PATCH 0/1] http: Fix crash when passing malformed URL Message-ID: <20160316212938.GA4441@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160316212938.GA4441%40sigill.intra.peff.net In-Reply-To: <1458125647-32707-1-git-send-email-anton.wuerfel@fau.de> ``` On Wed, Mar 16, 2016 at 11:54:06AM +0100, Anton Wuerfel wrote: > When passing a malformed URL to http_init() in http.c, git dies from a null > pointer dereference. An example for a malformed URL is http:/git-scm.com (note > the single slash after the protocol). > > I could not reproduce this error within any functions of git - I just noticed it > during development of a git extension together with Phillip Raffeck. You can reproduce it with: git clone https::bogus Normally we recognize "https://" as the signal to send the URL to the git-remote-https helper, but you can override the helper by specifying "whatever::", and then the rest of the string will be passed to it. -Peff ``` ## Jeff King, 2016-03-16 21:42 Subject: Re: [PATCH 1/1] http: Fix crash when passing malformed URL Message-ID: <20160316214200.GB4441@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160316214200.GB4441%40sigill.intra.peff.net In-Reply-To: <1458125647-32707-2-git-send-email-anton.wuerfel@fau.de> ``` On Wed, Mar 16, 2016 at 11:54:07AM +0100, Anton Wuerfel wrote: > When passing a malformed URL to http_init() in http.c, git dies from a null > pointer dereference. An example for a malformed URL is http:/git-scm.com (note > the single slash after the protocol). > This patch adds simple error handling as git notices the malformed URL already, > but never checks the error value. > > When passing a malformed URL, credential_from_url(struct credential *c, const char *url) > initializes *c with null values. When the existence of `://` in url is checked, > the function returns without further change of *c. > The null pointer dereference occurs in get_curl_handle () at http.c:593, when > the `protocol` field of struct credential is strcmp'ed: So I think the most direct bug here is that line 593 assumes that http_auth.protocol is not NULL, when it might very well be (if we could not parse the protocol). Your solution is to detect early that we don't have a URL that curl can parse, and bail. I think that's probably a reasonable thing to do in general. But it doesn't make me certain that there's a case that curl might parse that our credential url-parser might not. And the code in question does not even care about credentials at all! It's just piggy-backing on the earlier parse done by the credential code. I think it would make much more sense for it to rely on the normalized url we produce. IOW, to do something like: if (starts_with(normalized_url, "https://")) /* https stuff */ else if (starts_with(normalized_url, "http://")) /* http stuff */ else /* other stuff */ Note that the current code doesn't actually check for "http" (versus other protocols; despite the name http_init(), this code gets run for the probably-never-used-these-days git-over-ftp protocol). I suspect we are respecting http_proxy for ftp connections, which is silly. Note that normalized_url is freed before this point, so we may have to hold onto it longer. Or it may be possible to use the broken-down representation from config.url; I didn't look. -Peff ```