[PATCH 0/2] Harden url.c URL-decoding logic

[PATCH 0/2] Harden url.c URL-decoding logic

[Matthew DeVore]

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(-)

-- 
2.17.1

[PATCH 1/2] url: do not read past end of buffer

[Matthew DeVore]

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.

Signed-off-by: Matthew DeVore <matvore@google.com>
---
 url.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/url.c b/url.c
index 25576c390b..c0bb4e23c3 100644
--- a/url.c
+++ b/url.c
@@ -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 == '+')
-- 
2.17.1

[PATCH 2/2] url: do not allow %00 to represent NULL in URLs

[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.

Signed-off-by: Matthew DeVore <matvore@google.com>
---
 url.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/url.c b/url.c
index c0bb4e23c3..cf791cb139 100644
--- a/url.c
+++ b/url.c
@@ -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
-- 
2.17.1

Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On 2019-06-03 at 20:45:26, Matthew DeVore wrote:
> 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".
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH 1/2] url: do not read past end of buffer

[René Scharfe]

Am 03.06.19 um 22:45 schrieb Matthew DeVore:
> 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.
>
> Signed-off-by: Matthew DeVore <matvore@google.com>
> ---
>  url.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/url.c b/url.c
> index 25576c390b..c0bb4e23c3 100644
> --- a/url.c
> +++ b/url.c
> @@ -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) {

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!

>  			int val = hex2chr(q + 1);
>  			if (0 <= val) {
>  				strbuf_addch(out, val);
>  				q += 3;
>  				len -= 3;
>  				continue;
>  			}
>  		}
>
>  		if (decode_plus && c == '+')
>

Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs

[René Scharfe]

Am 03.06.19 um 22:45 schrieb 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.

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.

>
> Signed-off-by: Matthew DeVore <matvore@google.com>
> ---
>  url.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/url.c b/url.c
> index c0bb4e23c3..cf791cb139 100644
> --- a/url.c
> +++ b/url.c
> @@ -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 1/2] url: do not read past end of buffer

[Matthew DeVore]

On Tue, Jun 04, 2019 at 07:00:34AM +0200, René Scharfe wrote:
> Am 03.06.19 um 22:45 schrieb Matthew DeVore:
> > 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.
> >
> > Signed-off-by: Matthew DeVore <matvore@google.com>
> > ---
> >  url.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/url.c b/url.c
> > index 25576c390b..c0bb4e23c3 100644
> > --- a/url.c
> > +++ b/url.c
> > @@ -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) {
> 
> 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)) {

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.)

Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs

[Matthew DeVore]

On Tue, Jun 04, 2019 at 07:01:01AM +0200, René Scharfe wrote:
> 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.

Re: [PATCH 2/2] url: do not allow %00 to represent NULL in URLs

[Matthew DeVore]

On Tue, Jun 04, 2019 at 01:02:43AM +0000, brian m. carlson wrote:
> 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.

OK. Good to know that there is no justification to support %00 in URLs.

> 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.