{"thread":{"id":"51240","subject":"[PATCH v2 0/2] Harden url.c URL-decoding logic","startedAt":"2019-06-04T17:57:23Z","lastAt":"2019-06-04T21:13:48Z","messageCount":5,"participants":["Matthew DeVore","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"376680","messageId":"cover.1559670300.git.matvore@google.com","threadId":"51240","inReplyTo":null,"subject":"[PATCH v2 0/2] Harden url.c URL-decoding logic","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-04T17:57:03Z","receivedAt":"2019-06-04T17:57:23Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"This roll-up includes simple but important fixes from Brian Carlson and René\nScharfe.\n\n - fix typo of \"NUL\" in commit heading\n - re-enable %-decoding in non-NULL-terminated strings\n\nMatthew DeVore (2):\n url: do not read past end of buffer\n url: do not allow %00 to represent NUL in URLs\n\n url.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"376681","messageId":"9628f0bfeda578a1c7d157d61b87f5c430567d74.1559670300.git.matvore@google.com","threadId":"51240","inReplyTo":"cover.1559670300.git.matvore@google.com","subject":"[PATCH v2 1/2] url: do not read past end of buffer","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-04T17:57:04Z","receivedAt":"2019-06-04T17:57:25Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"url_decode_internal could have been tricked into reading past the length\nof the **query buffer if there are fewer than 2 characters after a % (in\na null-terminated string, % would have to be the last character).\nPrevent this from happening by checking len before decoding the %\nsequence.\n\nHelped-by: René Scharfe \nSigned-off-by: Matthew DeVore \n---\n url.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/url.c b/url.c\nindex 25576c390b..9ea9d5611b 100644\n--- a/url.c\n+++ b/url.c\n@@ -39,21 +39,21 @@ static char *url_decode_internal(const char **query, int len,\n \t\tunsigned char c = *q;\n \n \t\tif (!c)\n \t\t\tbreak;\n \t\tif (stop_at && strchr(stop_at, c)) {\n \t\t\tq++;\n \t\t\tlen--;\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (c == '%') {\n+\t\tif (c == '%' && (len < 0 || len >= 3)) {\n \t\t\tint val = hex2chr(q + 1);\n \t\t\tif (0 <= val) {\n \t\t\t\tstrbuf_addch(out, val);\n \t\t\t\tq += 3;\n \t\t\t\tlen -= 3;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \n \t\tif (decode_plus && c == '+')\n-- \n2.21.0\n\n"},{"id":"376682","messageId":"d3d6316a2aa4691c630b1bb2db6c3ac706aaaa31.1559670300.git.matvore@google.com","threadId":"51240","inReplyTo":"cover.1559670300.git.matvore@google.com","subject":"[PATCH v2 2/2] url: do not allow %00 to represent NUL in URLs","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-04T17:57:05Z","receivedAt":"2019-06-04T17:57:28Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"There is no reason to allow %00 to terminate a string, so do not allow it.\nOtherwise, we end up returning arbitrary content in the string (that which is\nafter the %00) which is effectively hidden from callers and can escape sanity\nchecks and validation, and possible be used in tandem with a security\nvulnerability to introduce a payload.\n\nHelped-by: brian m. carlson \nSigned-off-by: Matthew DeVore \n---\n url.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/url.c b/url.c\nindex 9ea9d5611b..1b8ef78cea 100644\n--- a/url.c\n+++ b/url.c\n@@ -41,21 +41,21 @@ static char *url_decode_internal(const char **query, int len,\n \t\tif (!c)\n \t\t\tbreak;\n \t\tif (stop_at && strchr(stop_at, c)) {\n \t\t\tq++;\n \t\t\tlen--;\n \t\t\tbreak;\n \t\t}\n \n \t\tif (c == '%' && (len < 0 || len >= 3)) {\n \t\t\tint val = hex2chr(q + 1);\n-\t\t\tif (0 <= val) {\n+\t\t\tif (0 < val) {\n \t\t\t\tstrbuf_addch(out, val);\n \t\t\t\tq += 3;\n \t\t\t\tlen -= 3;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \n \t\tif (decode_plus && c == '+')\n \t\t\tstrbuf_addch(out, ' ');\n \t\telse\n-- \n2.21.0\n\n"},{"id":"376690","messageId":"xmqq4l55t7t5.fsf@gitster-ct.c.googlers.com","threadId":"51240","inReplyTo":"9628f0bfeda578a1c7d157d61b87f5c430567d74.1559670300.git.matvore@google.com","subject":"Re: [PATCH v2 1/2] url: do not read past end of buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-04T20:27:34Z","receivedAt":"2019-06-04T20:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> url_decode_internal could have been tricked into reading past the length\n> of the **query buffer if there are fewer than 2 characters after a % (in\n> a null-terminated string, % would have to be the last character).\n> Prevent this from happening by checking len before decoding the %\n> sequence.\n>\n> Helped-by: René Scharfe \n> Signed-off-by: Matthew DeVore \n> ---\n> url.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/url.c b/url.c\n> index 25576c390b..9ea9d5611b 100644\n> --- a/url.c\n> +++ b/url.c\n> @@ -39,21 +39,21 @@ static char *url_decode_internal(const char **query, int len,\n> \t\tunsigned char c = *q;\n> \n> \t\tif (!c)\n> \t\t\tbreak;\n> \t\tif (stop_at && strchr(stop_at, c)) {\n> \t\t\tq++;\n> \t\t\tlen--;\n> \t\t\tbreak;\n> \t\t}\n> \n> -\t\tif (c == '%') {\n> +\t\tif (c == '%' && (len < 0 || len >= 3)) {\n> \t\t\tint val = hex2chr(q + 1);\n\nThis made me wonder what happens when the caller sent -1 in len, but\nhex2chr() stops on such a string with % plus one hexadecimal at the\nend of the string, and we'd end up copying these two bytes one at a\ntime, which is what we want, so it is OK. And the rejection of %00\ndone in 2/2 follows the same codeflow here, which is quite straight\nforward.\n\nNice.\n\n\n> \t\t\tif (0 <= val) {\n> \t\t\t\tstrbuf_addch(out, val);\n> \t\t\t\tq += 3;\n> \t\t\t\tlen -= 3;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> \n> \t\tif (decode_plus && c == '+')\n"},{"id":"376695","messageId":"20190604211344.GM4641@comcast.net","threadId":"51240","inReplyTo":"cover.1559670300.git.matvore@google.com","subject":"Re: [PATCH v2 0/2] Harden url.c URL-decoding logic","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T21:13:44Z","receivedAt":"2019-06-04T21:13:48Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"First message had incorrect recipient list. Re-sending with non-typo'd Jeff's\ne-mail address.\n\nSomeone also politely reminded me off-band that I should make subsequent\nversions of patchsets be respond-to on the cover-letter of v1 of that patchset.\nI will do that from next time.\n"}]}