Harden url.c URL-decoding logic
v2 cover Harden url.c URL-decoding logic
This roll-up includes simple but important fixes from Brian Carlson and René Scharfe.
- fix typo of "NUL" in commit heading
- re-enable %-decoding in non-NULL-terminated strings
Matthew DeVore (2):
url: do not read past end of buffer
url: do not allow %00 to represent NUL in URLs
url.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Signature · 1 line
2.21.0
PATCH v2 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 <0 || 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.21.0
Re: [PATCH v2 1/2] url: do not read past end of buffer
Quoted 20 lines · Matthew DeVore
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.Helped-by René Scharfe <[email protected]>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;if (!c)
url.c static char *url_decode_internal(const char **query, int len, · line 50
Quoted 10 lines
url.c +1
-1break ;if (stop_at &&strchr (stop_at, c)) { q++; len--;break ; } -if (c =='%' ) { +if (c =='%' && (len <0 || len >=3 )) {int val =hex2chr (q +1 );
This made me wonder what happens when the caller sent -1 in len, but hex2chr() stops on such a string with % plus one hexadecimal at the end of the string, and we'd end up copying these two bytes one at a time, which is what we want, so it is OK. And the rejection of %00 done in 2/2 follows the same codeflow here, which is quite straight forward.
Nice.
Quoted 9 lines
url.c +0
-0if (0 <= val) {strbuf_addch (out, val); q +=3 ; len -=3 ;continue ; } }if (decode_plus && c =='+' )
PATCH v2 2/2 url: do not allow %00 to represent NUL 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 <0 || 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.21.0
First message had incorrect recipient list. Re-sending with non-typo'd Jeff's e-mail address.
Someone also politely reminded me off-band that I should make subsequent versions of patchsets be respond-to on the cover-letter of v1 of that patchset. I will do that from next time.