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

[Matthew DeVore <matvore@comcast.net>]

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