Jeff King <peff@xxxxxxxx> 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.
