[PATCH 0/1] http: Fix crash when passing malformed URL

[PATCH 0/1] http: Fix crash when passing malformed URL

[Anton Wuerfel]

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

[PATCH 1/1] http: Fix crash when passing malformed URL

[Anton Wuerfel]

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

Re: [PATCH 0/1] http: Fix crash when passing malformed URL

[Jeff King]

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

Re: [PATCH 1/1] http: Fix crash when passing malformed URL

[Jeff King]

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