Harden url.c URL-decoding logic
cover Harden url.c URL-decoding logic
Fixing two minor issues related to string-handling corner cases in url.c
Thanks,
Matthew DeVore (2):
url: do not read past end of buffer
url: do not allow %00 to represent NULL in URLs
url.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/2 url: do not read past end of buffer
url_decode_internal could have been tricked into reading past the length of the **query buffer if there are fewer than 2 characters after a % (in a null-terminated string, % would have to be the last character). Prevent this from happening by checking len before decoding the % sequence.
--- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
url.c +1 -1
@@ -39,21 +39,21 @@ static char *url_decode_internal(const char **query, int len,unsigned char c = *q;if (!c)break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; } -if (c =='%' ) { +if (c =='%' && len >=3 ) {int val =hex2chr (q +1 );if (0 <= val) {strbuf_addch (out, val); q +=3 ; len -=3 ;continue ; } }if (decode_plus && c =='+' )
Signature · 1 line
2.17.1
Re: [PATCH 1/2] url: do not read past end of buffer
Am 03.06.19 um 22:45 schrieb Matthew DeVore:
Quoted 18 lines
url_decode_internalcould have been tricked into reading past the length of the **query buffer if there are fewer than 2 characters after a % (in a null-terminated string, % would have to be the last character). Prevent this from happening by checking len before decoding the % sequence.Signed-off-by Matthew DeVore <[email protected]>--- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)url.c +0
-0@@ -39,21 +39,21 @@ static char *url_decode_internal(const char **query, int len,unsigned char c = *q;
url.c static char *url_decode_internal(const char **query, int len, · line 49
Quoted 10 lines
url.c +1
-1if (!c)break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; } -if (c =='%' ) { +if (c =='%' && len >=3 ) {
Tricky. hex2chr() makes sure to not run over the end of NUL-terminated strings, but url_decode_internal() is supposed to honor the parameter len as well. Your change disables %-decoding for the two callers that pass -1 as len, though. So perhaps like this?
if (c == '%' && (len < 0 || len >= 3)) {In any case: Good find!
Quoted 11 lines
url.c +0
-0int val =hex2chr (q +1 );if (0 <= val) {strbuf_addch (out, val); q +=3 ; len -=3 ;continue ; } }if (decode_plus && c =='+' )
Matthew DeVore
AppliedRe: [PATCH 1/2] url: do not read past end of buffer
Quoted 35 lines · René Scharfe
Am 03.06.19 um 22:45 schrieb Matthew DeVore:
Quoted 18 lines
url_decode_internalcould have been tricked into reading past the length of the **query buffer if there are fewer than 2 characters after a % (in a null-terminated string, % would have to be the last character). Prevent this from happening by checking len before decoding the % sequence.Signed-off-by Matthew DeVore <[email protected]>--- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)url.c +0
-0@@ -39,21 +39,21 @@ static char *url_decode_internal(const char **query, int len,unsigned char c = *q;url.c static char *url_decode_internal(const char **query, int len, · line 49
Quoted 10 lines
url.c +1
-1if (!c)break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; } -if (c =='%' ) { +if (c =='%' && len >=3 ) {Tricky.
hex2chr()makes sure to not run over the end of NUL-terminated strings, buturl_decode_internal()is supposed to honor the parameter len as well. Your change disables %-decoding for the two callers that pass -1 as len, though. So perhaps like this?
Quoted 1 line
if (c == '%' && (len < 0 || len >= 3)) {I've applied this and will include it in the next roll-up. Thank you for catching it. (I'm disappointed that I missed it and that there were no tests to catch the mistake.)
PATCH 2/2 url: do not allow %00 to represent NULL in URLs
In reply to Matthew DeVoreThere is no reason to allow %00 to terminate a string, so do not allow it. Otherwise, we end up returning arbitrary content in the string (that which is after the %00) which is effectively hidden from callers and can escape sanity checks and validation, and possible be used in tandem with a security vulnerability to introduce a payload.
--- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
url.c +1 -1
@@ -41,21 +41,21 @@ static char *url_decode_internal(const char **query, int len,if (!c)break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; }if (c =='%' && len >=3 ) {int val =hex2chr (q +1 ); -if (0 <= val) { +if (0 < val) {strbuf_addch (out, val); q +=3 ; len -=3 ;continue ; } }if (decode_plus && c =='+' )strbuf_addch (out,' ' );else
Signature · 1 line
2.17.1
Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs
Quoted 5 lines · Matthew DeVore
There is no reason to allow %00 to terminate a string, so do not allow it. Otherwise, we end up returning arbitrary content in the string (that which is after the %00) which is effectively hidden from callers and can escape sanity checks and validation, and possible be used in tandem with a security vulnerability to introduce a payload.
So I think the reason you've stated is good and I agree that we shouldn't decode data we're not going to use. However, I'm also interested in the cases in which we decode data and don't want to allow NULs, because we should, in general, allow bizarre URLs as long as they're URL-encoded.
It looks like several of the places we do this are in the credential manager code, and I think I can agree that usernames and passwords should not contain NUL characters (for Basic auth, RFC 7617 prohibits it). It also seems that the credential code decodes the path parameter before passing it on, which is unfortunate, but can't be changed for backward compatibility reasons.
And then the other instances are a file: URL in remote-testsvn.c and query parameters that have no reason to contain NULs in http-backend.c.
So I think overall this is fine, although we probably want to change the commit summary to say "NUL" instead of "NULL".
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs
Quoted 9 lines · brian m. carlson
It looks like several of the places we do this are in the credential manager code, and I think I can agree that usernames and passwords should not contain NUL characters (for Basic auth, RFC 7617 prohibits it). It also seems that the credential code decodes the path parameter before passing it on, which is unfortunate, but can't be changed for backward compatibility reasons.
And then the other instances are a file: URL in
remote-testsvn.cand query parameters that have no reason to contain NULs inhttp-backend.c.
OK. Good to know that there is no justification to support %00 in URLs.
Quoted 2 lines
So I think overall this is fine, although we probably want to change the commit summary to say "NUL" instead of "NULL".
Applied for the next roll-up. Thank you for taking a look.
Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs
In reply to Matthew DeVoreAm 03.06.19 um 22:45 schrieb Matthew DeVore:
Quoted 5 lines
There is no reason to allow %00 to terminate a string, so do not allow it. Otherwise, we end up returning arbitrary content in the string (that which is after the %00) which is effectively hidden from callers and can escape sanity checks and validation, and possible be used in tandem with a security vulnerability to introduce a payload.
It's a bit hard to see with the (extended, but still) limited context, but url_decode_internal() effectively returns a NUL-terminated string, even though it does use a strbuf parameter named "out" for temporary storage. So callers really have no use for decoded NULs, and this change thus makes sense to me.
Quoted 34 lines
Signed-off-by Matthew DeVore <[email protected]>--- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)url.c +1
-1@@ -41,21 +41,21 @@ static char *url_decode_internal(const char **query, int len,if (!c)break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; }if (c =='%' && len >=3 ) {int val =hex2chr (q +1 ); -if (0 <= val) { +if (0 < val) {strbuf_addch (out, val); q +=3 ; len -=3 ;continue ; } }if (decode_plus && c =='+' )strbuf_addch (out,' ' );else
Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs
Quoted 6 lines · René Scharfe
It's a bit hard to see with the (extended, but still) limited context, but
url_decode_internal()effectively returns a NUL-terminated string, even though it does use a strbuf parameter named "out" for temporary storage. So callers really have no use for decoded NULs, and this change thus makes sense to me.
That was more or less my train of thought as well. Thank you for taking a look.