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

From: Jeff King <peff@peff.net>
Date: Mar 16, 2016, 21:42 UTC
Message-ID: <20160316214200.GB4441@sigill.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:

Show 11 quoted lines

> 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