git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Matthew DeVore <matvore@comcast.net>
Date
Jun 4, 2019, 17:22 UTC
Message-ID
<20190604172238.GJ4641@comcast.net>
In-Reply-To
<18ab56e2-3b25-8cfd-438d-72f49145adc4@web.de>
On Tue, Jun 04, 2019 at 07:00:34AM +0200, René Scharfe wrote:
Show 36 quoted lines
> 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.)

Previous: René Scharfe
Message 9 of 9 in “Harden url.c URL-decoding logic”
  1. 0/2 Harden url.c URL-decoding logicMatthew DeVore, Jun 3, 2019
  2. 2/2 url: do not allow %00 to represent NULL in URLsMatthew DeVore, Jun 3, 2019
  3. brian m. carlsonJun 4, 2019
  4. Matthew DeVoreJun 4, 2019
  5. René ScharfeJun 4, 2019
  6. Matthew DeVoreJun 4, 2019
  7. 1/2 url: do not read past end of bufferMatthew DeVore, Jun 3, 2019
  8. René ScharfeJun 4, 2019
  9. Matthew DeVoreJun 4, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.