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

[Glen Choo <chooglen@google.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 10, 2022 at 10:57:34PM +0000, Glen Choo via GitGitGadget wrote:
>
>> +/* Redact headers in info */
>> +static void redact_sensitive_info_header(struct strbuf *header)
>> +{
>> +	const char *sensitive_header;
>> +
>> +	/*
>> +	 * curl's h2h3 prints headers in info, e.g.:
>> +	 *   h2h3 [<header-name>: <header-val>]
>> +	 */
>> +	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);
>
> This will misbehave if fed the string "h2h3 [", because that strlen()
> becomes 0, and the subtraction underflows.
>
> Unlikely, since we are being fed by curl, but possibly worth asserting
> (though see below for an alternative which drops this line).
>
>> +		if (redact_sensitive_header(&inner)) {
>> +			strbuf_setlen(header, strlen("h2h3 ["));
>
> This strlen may be better spelled as:
>
>   sensitive_header - header->buf
>
> which IMHO makes it more clear that our intent is to truncate based on
> the pointer we computed by skipping (and has no chance of getting out of
> sync with the earlier copy of the string).
>
> It's also a little more robust, in that it doesn't depend on "h2h3"
> being at the beginning of the string (though in practice it must be,
> because that's where skip_iprefix() is checking). See below on that.
>
>> +			strbuf_addbuf(header, &inner);
>> +			strbuf_addch(header, ']');
>> +		}
>> +
>> +		strbuf_release(&inner);
>
> This will do a new allocation/free for each info line, even if it's not
> redacted. It's probably premature optimization to worry about it, but
> you could do it all in the original strbuf, if we inform
> redact_sensitive_header() of the offset at which it should look for the
> header (and because it uses "sensitive_header - header->buf" for the
> truncation, it handles the extra "h2h3" at the beginning just fine).
> Something like:
>
> diff --git a/http.c b/http.c
> index 8135fac283..8a5ba3f477 100644
> --- a/http.c
> +++ b/http.c
> @@ -561,14 +561,14 @@ static void set_curl_keepalive(CURL *c)
>  #endif
>  
>  /* Return 1 if redactions have been made, 0 otherwise. */
> -static int redact_sensitive_header(struct strbuf *header)
> +static int redact_sensitive_header(struct strbuf *header, size_t offset)
>  {
>  	int ret = 0;
>  	const char *sensitive_header;
>  
>  	if (trace_curl_redact &&
> -	    (skip_iprefix(header->buf, "Authorization:", &sensitive_header) ||
> -	     skip_iprefix(header->buf, "Proxy-Authorization:", &sensitive_header))) {
> +	    (skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) ||
> +	     skip_iprefix(header->buf + offset, "Proxy-Authorization:", &sensitive_header))) {
>  		/* The first token is the type, which is OK to log */
>  		while (isspace(*sensitive_header))
>  			sensitive_header++;
> @@ -579,7 +579,7 @@ static int redact_sensitive_header(struct strbuf *header)
>  		strbuf_addstr(header, " <redacted>");
>  		ret = 1;
>  	} else if (trace_curl_redact &&
> -		   skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
> +		   skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) {
>  		struct strbuf redacted_header = STRBUF_INIT;
>  		const char *cookie;
>  
> @@ -631,17 +631,10 @@ static void redact_sensitive_info_header(struct strbuf *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);
> +		if (redact_sensitive_header(header, sensitive_header - header->buf)) {
> +			/* redaction ate our closing bracket */
>  			strbuf_addch(header, ']');
>  		}
> -
> -		strbuf_release(&inner);
>  	}
>  }
>  
> @@ -659,7 +652,7 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,
>  
>  	for (header = headers; *header; header++) {
>  		if (hide_sensitive_header)
> -			redact_sensitive_header(*header);
> +			redact_sensitive_header(*header, 0);
>  		strbuf_insertstr((*header), 0, text);
>  		strbuf_insertstr((*header), strlen(text), ": ");
>  		strbuf_rtrim((*header));

As someone who's still trying to wrap my head around pointer
manipulations, these suggestions are very welcome, thanks!

I'll take these suggestions along with the HTTP2 one.