git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] http: redact curl h2h3 headers in info

From
Glen Choo <chooglen@google.com>
Date
Nov 10, 2022, 22:14 UTC
Message-ID
<kl6lmt8y31ak.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<Y21zz1HYXzyyfwqy@google.com>
Emily Shaffer <nasamuffin@google.com> writes:
Show 74 quoted lines
> On Wed, Nov 09, 2022 at 12:52:31AM +0000, Glen Choo via GitGitGadget wrote:
>> 
>> 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>
>> 
>> Teach http.c to check for h2h3 headers in info and redact them using the
>> existing header redaction logic.
>> 
>> [1] https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77
>> 
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>     http: redact curl h2h3 headers in info
>>     
>>     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.
>>     
>>     Since this comes from curl's HTTP2.0/3.0 module, this can be mitigated
>>     by setting http.version to 1.X, e.g. "git -c http.version=HTTP/1.1".
>>     
>>     According to [1], the susceptible curl versions appear to be 7.86.0,
>>     7.85.0, 7.84.0, 7.83.1, 7.83.0, 7.82.0, but I'm not sure which platforms
>>     are vulnerable.
>>     
>>     This patch fixes the issue on my machine running curl 7.85.0, so I think
>>     it is okay to merge as-is. That said, I would strongly prefer to add
>>     tests, but I haven't figured out how. In particular:
>>     
>>      * Do we have a way of using HTTP/2.0 in our tests? A cursory glance at
>>        our httpd config suggests that we only use HTTP/1.1.
>>      * 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.
>>     
>>     [1]
>>     https://github.com/curl/curl/commit/f8c3724aa90472c0e617ddbbc420aa199971eb77
>> 
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1377%2Fchooglen%2Fhttp%2Fredact-h2h3-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1377/chooglen/http/redact-h2h3-v1
>> Pull-Request: https://github.com/git/git/pull/1377
>> 
>>  http.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>> 
>> diff --git a/http.c b/http.c
>> index 5d0502f51fd..cbcc7c3f5b6 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)
>>  }
>>  #endif
>>  
>> -static void redact_sensitive_header(struct strbuf *header)
>> +/* Return 0 if redactions been made, 1 otherwise. */
>
> Does it make sense to reverse the retval here?
>
> `if (!redact_sensitive_header())` sounds like "if not redacted, ..." -
> but here it means the opposite, right?

I struggled with this for a bit since I wasn't sure what the convention is here. Enumerating some off the top of my head, we have:

- For 'booleans', we "0" for false and "1" for true (e.g.
  starts_with()).
- For functions that may fail with error, we have "0" for success and
  nonzero to signal the failure type (e.g. strbuf_getdelim()).
- For functions that don't fail we have "0" for "nothing was done" and
  "1" for something was done (e.g. skip_prefix()).

(Tangent: from a readability perspective, this is pretty poor. I need to know beforehand whether or not the function may fail with error before I know what the return value means?)

This probably falls into the last category, so for consistency, I think this should return "1" for "redactions have happened" (as you suggested).

Show 42 quoted lines
>> +static int redact_sensitive_header(struct strbuf *header)
>>  {
>> +	int ret = 1;
>>  	const char *sensitive_header;
>>  
>>  	if (trace_curl_redact &&
>> @@ -575,6 +577,7 @@ static void redact_sensitive_header(struct strbuf *header)
>>  		/* Everything else is opaque and possibly sensitive */
>>  		strbuf_setlen(header,  sensitive_header - header->buf);
>>  		strbuf_addstr(header, " <redacted>");
>> +		ret = 0;
>>  	} else if (trace_curl_redact &&
>>  		   skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
>>  		struct strbuf redacted_header = STRBUF_INIT;
>> @@ -612,6 +615,27 @@ static void redact_sensitive_header(struct strbuf *header)
>>  
>>  		strbuf_setlen(header, sensitive_header - header->buf);
>>  		strbuf_addbuf(header, &redacted_header);
>> +		ret = 0;
>> +	}
>> +	return ret;
>> +}
>> +
>> +/* 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);
>> +		if (!redact_sensitive_header(&inner)) {
>> +			strbuf_setlen(header, strlen("h2h3 ["));
>> +			strbuf_addbuf(header, &inner);
>> +			strbuf_addch(header, ']');
>
> I'd really like some more comments in this function - even just one
> describing the string we're trying to redact, or showing a sample line.
> Navigating string parsing is always a bit difficult.
Ah yes, I should include a description of the string.
Show 12 quoted lines
>> +		}
>>  	}
>>  }
>>  
>> @@ -668,6 +692,18 @@ static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)
>>  	strbuf_release(&out);
>>  }
>>  
>> +static void curl_print_info(char *data, size_t size)
>
> Nit: Every other helper in this file calls it _dump_, so should this
> also say _dump_ instead of _print_?
Sure, I have no opinion here, so I'll do that.
Show 36 quoted lines
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +
>> +	strbuf_add(&buf, data, size);
>> +
>> +	redact_sensitive_info_header(&buf);
>> +	trace_printf_key(&trace_curl, "== Info: %s", buf.buf);
>> +
>> +	strbuf_release(&buf);
>> +}
>> +
>>  static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)
>>  {
>>  	const char *text;
>> @@ -675,7 +711,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
>>  
>>  	switch (type) {
>>  	case CURLINFO_TEXT:
>> -		trace_printf_key(&trace_curl, "== Info: %s", data);
>> +		curl_print_info(data, size);
>>  		break;
>>  	case CURLINFO_HEADER_OUT:
>>  		text = "=> Send header";
>> 
>> base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193
>
> Otherwise functionally it seems fine to me. case CURLINFO_TEXT is the
> one case that's not already using a curl_dump_* helper, so we're adding
> one, and to that helper we're adding a call out to
> redact_sensitive_header().
>
> Thanks.
>  - Emily
>
>> -- 
>> gitgitgadget
Previous: Emily ShafferNext: Taylor Blau
Message 13 of 24 in “http: redact curl h2h3 headers in info”
  1. http: redact curl h2h3 headers in infoGlen Choo via GitGitGadget, Nov 9, 2022
  2. Taylor BlauNov 10, 2022
  3. Glen ChooNov 10, 2022
  4. Jeff KingNov 10, 2022
  5. Glen ChooNov 10, 2022
  6. Jeff KingNov 11, 2022
  7. Taylor BlauNov 11, 2022
  8. t: run t5551 tests with both HTTP and HTTP/2Jeff King, Nov 11, 2022
  9. Ævar Arnfjörð BjarmasonNov 11, 2022
  10. Jeff KingNov 11, 2022
  11. Jeff KingNov 11, 2022
  12. Emily ShafferNov 10, 2022
  13. Glen ChooNov 10, 2022
  14. Taylor BlauNov 11, 2022
  15. http: redact curl h2h3 headers in infoGlen Choo via GitGitGadget, Nov 10, 2022
  16. Taylor BlauNov 11, 2022
  17. Jeff KingNov 11, 2022
  18. Taylor BlauNov 11, 2022
  19. Glen ChooNov 11, 2022
  20. 0/2 http: redact curl h2h3 headers in infoGlen Choo via GitGitGadget, Nov 11, 2022
  21. 1/2 t: run t5551 tests with both HTTP and HTTP/2Jeff King via GitGitGadget, Nov 11, 2022
  22. 2/2 http: redact curl h2h3 headers in infoGlen Choo via GitGitGadget, Nov 11, 2022
  23. Jeff KingNov 14, 2022
  24. Taylor BlauNov 14, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.