| Message ID | 9628f0bfeda578a1c7d157d61b87f5c430567d74.1559670300.git.matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Harden url.c URL-decoding logic | expand |
Matthew DeVore <matvore@google.com> writes: > 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. > > Helped-by: René Scharfe <l.s.r@web.de> > 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..9ea9d5611b 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 < 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. > if (0 <= val) { > strbuf_addch(out, val); > q += 3; > len -= 3; > continue; > } > } > > if (decode_plus && c == '+')
diff --git a/url.c b/url.c index 25576c390b..9ea9d5611b 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 < 0 || len >= 3)) { int val = hex2chr(q + 1); if (0 <= val) { strbuf_addch(out, val); q += 3; len -= 3; continue; } } if (decode_plus && c == '+')
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. Helped-by: René Scharfe <l.s.r@web.de> Signed-off-by: Matthew DeVore <matvore@google.com> --- url.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)