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