{"thread":{"id":"51231","subject":"[PATCH 2/2] url: do not allow %00 to represent NULL in URLs","startedAt":"2019-06-03T21:44:28Z","lastAt":"2019-06-04T17:38:23Z","messageCount":9,"participants":["Matthew DeVore","brian m. carlson","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"376617","messageId":"20190603204526.7723-3-matvore@google.com","threadId":"51231","inReplyTo":"20190603204526.7723-1-matvore@google.com","subject":"[PATCH 2/2] url: do not allow %00 to represent NULL in URLs","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-03T20:45:26Z","receivedAt":"2019-06-03T21:44: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\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n url.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/url.c b/url.c\nindex c0bb4e23c3..cf791cb139 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 >= 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.17.1\n\n"},{"id":"376618","messageId":"20190603204526.7723-1-matvore@google.com","threadId":"51231","inReplyTo":null,"subject":"[PATCH 0/2] Harden url.c URL-decoding logic","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-03T20:45:24Z","receivedAt":"2019-06-03T21:48:42Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Fixing two minor issues related to string-handling corner cases in url.c\n\nThanks,\n\nMatthew DeVore (2):\n  url: do not read past end of buffer\n  url: do not allow %00 to represent NULL in URLs\n\n url.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"376624","messageId":"20190603204526.7723-2-matvore@google.com","threadId":"51231","inReplyTo":"20190603204526.7723-1-matvore@google.com","subject":"[PATCH 1/2] url: do not read past end of buffer","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-06-03T20:45:25Z","receivedAt":"2019-06-03T22:22:49Z","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\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n url.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/url.c b/url.c\nindex 25576c390b..c0bb4e23c3 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 >= 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.17.1\n\n"},{"id":"376631","messageId":"20190604010243.GR8616@genre.crustytoothpaste.net","threadId":"51231","inReplyTo":"20190603204526.7723-3-matvore@google.com","subject":"Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-06-04T01:02:43Z","receivedAt":"2019-06-04T01:02:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-06-03 at 20:45:26, Matthew DeVore wrote:\n> There is no reason to allow %00 to terminate a string, so do not allow it.\n> Otherwise, we end up returning arbitrary content in the string (that which is\n> after the %00) which is effectively hidden from callers and can escape sanity\n> checks and validation, and possible be used in tandem with a security\n> vulnerability to introduce a payload.\n\nSo I think the reason you've stated is good and I agree that we\nshouldn't decode data we're not going to use. However, I'm also\ninterested in the cases in which we decode data and don't want to allow\nNULs, because we should, in general, allow bizarre URLs as long as\nthey're URL-encoded.\n\nIt looks like several of the places we do this are in the credential\nmanager code, and I think I can agree that usernames and passwords\nshould not contain NUL characters (for Basic auth, RFC 7617 prohibits\nit). It also seems that the credential code decodes the path parameter\nbefore passing it on, which is unfortunate, but can't be changed for\nbackward compatibility reasons.\n\nAnd then the other instances are a file: URL in remote-testsvn.c and\nquery parameters that have no reason to contain NULs in http-backend.c.\n\nSo I think overall this is fine, although we probably want to change the\ncommit summary to say \"NUL\" instead of \"NULL\".\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"376643","messageId":"18ab56e2-3b25-8cfd-438d-72f49145adc4@web.de","threadId":"51231","inReplyTo":"20190603204526.7723-2-matvore@google.com","subject":"Re: [PATCH 1/2] url: do not read past end of buffer","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-06-04T05:00:34Z","receivedAt":"2019-06-04T05:01:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.19 um 22:45 schrieb Matthew DeVore:\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> Signed-off-by: Matthew DeVore <matvore@google.com>\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..c0bb4e23c3 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 >= 3) {\n\nTricky.  hex2chr() makes sure to not run over the end of NUL-terminated\nstrings, but url_decode_internal() is supposed to honor the parameter\nlen as well.  Your change disables %-decoding for the two callers that\npass -1 as len, though.  So perhaps like this?\n\n\t\tif (c == '%' && (len < 0 || len >= 3)) {\n\nIn any case: Good find!\n\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>\n\n"},{"id":"376644","messageId":"ca09cb2f-e376-1491-102d-0b06e49530a4@web.de","threadId":"51231","inReplyTo":"20190603204526.7723-3-matvore@google.com","subject":"Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-06-04T05:01:01Z","receivedAt":"2019-06-04T05:01:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.19 um 22:45 schrieb Matthew DeVore:\n> There is no reason to allow %00 to terminate a string, so do not allow it.\n> Otherwise, we end up returning arbitrary content in the string (that which is\n> after the %00) which is effectively hidden from callers and can escape sanity\n> checks and validation, and possible be used in tandem with a security\n> vulnerability to introduce a payload.\n\nIt's a bit hard to see with the (extended, but still) limited context,\nbut url_decode_internal() effectively returns a NUL-terminated string,\neven though it does use a strbuf parameter named \"out\" for temporary\nstorage.  So callers really have no use for decoded NULs, and this\nchange thus makes sense to me.\n\n>\n> Signed-off-by: Matthew DeVore <matvore@google.com>\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 c0bb4e23c3..cf791cb139 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 >= 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>\n\n"},{"id":"376675","messageId":"20190604172238.GJ4641@comcast.net","threadId":"51231","inReplyTo":"18ab56e2-3b25-8cfd-438d-72f49145adc4@web.de","subject":"Re: [PATCH 1/2] url: do not read past end of buffer","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T17:22:38Z","receivedAt":"2019-06-04T17:22:43Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 07:00:34AM +0200, René Scharfe wrote:\n> Am 03.06.19 um 22:45 schrieb Matthew DeVore:\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> > Signed-off-by: Matthew DeVore <matvore@google.com>\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..c0bb4e23c3 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 >= 3) {\n> \n> Tricky.  hex2chr() makes sure to not run over the end of NUL-terminated\n> strings, but url_decode_internal() is supposed to honor the parameter\n> len as well.  Your change disables %-decoding for the two callers that\n> pass -1 as len, though.  So perhaps like this?\n> \n> \t\tif (c == '%' && (len < 0 || len >= 3)) {\n\nI've applied this and will include it in the next roll-up. Thank you for\ncatching it. (I'm disappointed that I missed it and that there were no tests to\ncatch the mistake.)\n"},{"id":"376677","messageId":"20190604172352.GK4641@comcast.net","threadId":"51231","inReplyTo":"ca09cb2f-e376-1491-102d-0b06e49530a4@web.de","subject":"Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T17:23:52Z","receivedAt":"2019-06-04T17:23:56Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 07:01:01AM +0200, René Scharfe wrote:\n> It's a bit hard to see with the (extended, but still) limited context,\n> but url_decode_internal() effectively returns a NUL-terminated string,\n> even though it does use a strbuf parameter named \"out\" for temporary\n> storage.  So callers really have no use for decoded NULs, and this\n> change thus makes sense to me.\n> \n\nThat was more or less my train of thought as well. Thank you for taking a look.\n"},{"id":"376679","messageId":"20190604173818.GL4641@comcast.net","threadId":"51231","inReplyTo":"20190604010243.GR8616@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T17:38:18Z","receivedAt":"2019-06-04T17:38:23Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 01:02:43AM +0000, brian m. carlson wrote:\n> It looks like several of the places we do this are in the credential\n> manager code, and I think I can agree that usernames and passwords\n> should not contain NUL characters (for Basic auth, RFC 7617 prohibits\n> it). It also seems that the credential code decodes the path parameter\n> before passing it on, which is unfortunate, but can't be changed for\n> backward compatibility reasons.\n> \n> And then the other instances are a file: URL in remote-testsvn.c and\n> query parameters that have no reason to contain NULs in http-backend.c.\n\nOK. Good to know that there is no justification to support %00 in URLs.\n\n> So I think overall this is fine, although we probably want to change the\n> commit summary to say \"NUL\" instead of \"NULL\".\n\nApplied for the next roll-up. Thank you for taking a look.\n"}]}