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

[Jeff King <peff@peff.net>]

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