| Message ID | 20190603204526.7723-2-matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Harden url.c URL-decoding logic | expand |
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 == '+') >
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.)
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 == '+')
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(-)