Re: [PATCH] d3dxof: avoid some strcpy overflows (Coverity)

[Henri Verbeet]

On 4 September 2016 at 11:24, Marcus Meissner <marcus(a)jet.franken.de> wrote:
> @@ -532,8 +532,11 @@ static BOOL is_name(parse_buffer* buf)
>    buf->rem_bytes -= pos;
>
> TRACE("Found name %s\n", tmp);
> +  if (strlen(tmp)+1 > sizeof(buf->value)) {
> +    FIXME("name %s exceeds buffer length\n", tmp);
> +    return FALSE;
> +  }
>    strcpy((char*)buf->value, tmp);
> -
>    return TRUE;
>  }
> There are plenty of things wrong with d3dxof, but assuming the
> preceding code isn't too broken, I don't think this can happen. You
> should probably just get rid of the "tmp" buffer and use read_bytes()
> or something similar after calculating "pos" though. (The TRACE can
> use debugstr_an().) That probably applies to is_string() as well.
> @@ -928,6 +937,10 @@ static BOOL parse_template_option_info(parse_buffer * buf)
>      {
>        if (get_TOKEN(buf) != TOKEN_NAME)
>          return FALSE;
> +      if (strlen((char*)buf->value)+1 > sizeof(cur_template->children[cur_template->nb_children])) {
> +        FIXME("name %s too long for buffer\n", (char*)buf->value);
> +        return FALSE;
> +      }
>        strcpy(cur_template->children[cur_template->nb_children], (char*)buf->value);
> Once you've calculated to number of bytes you need to copy anyway, you
> might as well just use memcpy(). I.e., strcpy() is almost always the
> wrong choice, for one reason or another.

[PATCH] d3dxof: avoid some strcpy overflows (Coverity)

[Matteo Bruni]

2016-09-05 9:34 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
> On 4 September 2016 at 11:24, Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > @@ -532,8 +532,11 @@ static BOOL is_name(parse_buffer* buf)
> >    buf->rem_bytes -= pos;
> >
> > TRACE("Found name %s\n", tmp);
> > +  if (strlen(tmp)+1 > sizeof(buf->value)) {
> > +    FIXME("name %s exceeds buffer length\n", tmp);
> > +    return FALSE;
> > +  }
> >    strcpy((char*)buf->value, tmp);
> > -
> >    return TRUE;
> >  }
> > There are plenty of things wrong with d3dxof, but assuming the
> > preceding code isn't too broken, I don't think this can happen. You
> > should probably just get rid of the "tmp" buffer and use read_bytes()
> > or something similar after calculating "pos" though. (The TRACE can
> > use debugstr_an().) That probably applies to is_string() as well.

Hmm, unless there is something guaranteeing that the name can't be
longer than 100 bytes (and there might be, I haven't looked too
deeply) a check is still needed. Totally agree with the rest here and
below, though.
> > @@ -928,6 +937,10 @@ static BOOL parse_template_option_info(parse_buffer * buf)
> >      {
> >        if (get_TOKEN(buf) != TOKEN_NAME)
> >          return FALSE;
> > +      if (strlen((char*)buf->value)+1 > sizeof(cur_template->children[cur_template->nb_children])) {
> > +        FIXME("name %s too long for buffer\n", (char*)buf->value);
> > +        return FALSE;
> > +      }
> >        strcpy(cur_template->children[cur_template->nb_children], (char*)buf->value);
> > Once you've calculated to number of bytes you need to copy anyway, you
> > might as well just use memcpy(). I.e., strcpy() is almost always the
> > wrong choice, for one reason or another.

FWIW that part also doesn't check if it's overrunning the array on the
other dimension i.e. that nb_children is less than MAX_CHILDREN. Not
that other parts of the code do check for it... The only time that's
checked in parsing.c is at the end of parse_object_parts() and that's
after the fact :/
I guess fixing that should be a separate patch anyway.

[PATCH] d3dxof: avoid some strcpy overflows (Coverity)

[Henri Verbeet]

On 6 September 2016 at 01:41, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> Hmm, unless there is something guaranteeing that the name can't be
> longer than 100 bytes (and there might be, I haven't looked too
> deeply) a check is still needed.
> Right, it checks the input buffer size, but not the output buffer
> size. Still, you effectively calculate the length of the name in
> "pos".