patchhttp: Fix crash when passing malformed URL

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(+)

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(+)

On Wed, Mar 16, 2016 at 11:54:07AM +0100, Anton Wuerfel wrote:

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

On Wed, Mar 16, 2016 at 11:54:06AM +0100, Anton Wuerfel wrote:

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