Emily Shaffer <nasamuffin@xxxxxxxxxx> writes: > 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@xxxxxxxxxx> >> --- >> 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). >> +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. >> + } >> } >> } >> >> @@ -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. >> +{ >> + 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
