On Wed, Nov 09, 2022 at 12:52:31AM +0000, Glen Choo via GitGitGadget wrote:

From: Glen Choo <redacted>

With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like
"Authorization" and "Cookie" get redacted. However, since [1], curl's
h2h3 module also prints headers in its "info", which don't get redacted.
For example,

  echo 'github.com	TRUE	/	FALSE	1698960413304	o	foo=bar' >cookiefile &&
  GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 git \
    -c 'http.cookiefile=cookiefile' \
    -c 'http.version=' \
    ls-remote https://github.com/git/git refs/heads/main 2>output &&
  grep 'cookie' output

produces output like:

  23:04:16.920495 http.c:678              == Info: h2h3 [cookie: o=foo=bar]
  23:04:16.920562 http.c:637              => Send header: cookie: o=<redacted>

Oops. Thanks for sharing this patch, I agree that the general approach
makes sense.

    I initially sent this to the security list, but the general impression
    is that this isn't sensitive enough for an embargoed fix, so this is
    better discussed in the open instead.

Indeed, this only really matters if the would-be victim is convinced to
somehow share the contents of their GIT_CURL_VEROBSE or GIT_TRACE_CURL
output. So I don't think there are any security implications here,
though I appreciate your caution in double checking with the
git-security list first.

     * How could we set up end-to-end tests to ensure that we're testing
       this against affected versions of curl? To avoid regressions, I'd
       also prefer to test against future versions of curl too.

Does that necessarily matter? We want to make sure that we don't see
sensitive headers from the h2h3 module with any version of cURL, no?

+/* Redact headers in info */
+static void redact_sensitive_info_header(struct strbuf *header)
+{
+	const char *sensitive_header;
+
+	if (trace_curl_redact &&
+	    skip_iprefix(header->buf, "h2h3 [", &sensitive_header)) {
+		struct strbuf inner = STRBUF_INIT;
+
+		/* Drop the trailing "]" */
+		strbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);

Makes sense. If the incoming header contains the "h2h3 [...]" marker,
and we are redacting sensitive headers, and there are header to redact,
redact them.

+		if (!redact_sensitive_header(&inner)) {
+			strbuf_setlen(header, strlen("h2h3 ["));
+			strbuf_addbuf(header, &inner);

This leaks inner.buf, no?

+			strbuf_addch(header, ']');
+		}
 	}
 }

The rest looks good.

Thanks,
Taylor