[FFmpeg-devel] [PATCH] OpenEXR decoder rev-4

[Reimar Döffinger]

On Fri, Jul 03, 2009 at 03:39:04PM +0200, Jimmy Christensen wrote:
> > What? variable_buffer_name is exactly 64 bit large and your code will
> > happily try to write any amount of data into it when there is no 0
> > termination. In addition it is on the stack, so this is _trivial_ to
> > exploit, anyone using this code could just make his passwords public and
> > he'd probably be just as safe.
> > Have you never heard of functions like strncpy (or the more useful
> > strlcpy, since it is not portable in FFmpeg as av_strlcpy?).
> > And all that is no excuse to make a copy of it just to compare it. Or
> > do you always when you read a book first copy it, then read the copy
> > while having the copy lying next to you and then burn the copy you made?
> > Because that is more or less what your code does...
> > Or to put in code what would make sense to do:
> > if (buf_end - buf>= 9&&  !strncmp(buf, "channels", 9)) {
> >      buf += 9;
> >      skip_buffertype();
> >      ...
> >
> > As long as you are using strcpy, strcmp, strlen or anything like that on
> > external data, unverified data you are doing it wrong.
> >
> 
> No I haven't heard about strncpy or strlcpy. But thanks for pointing me 
> in the direction.
> 
> The problem is that I don't know what the next variable is called and 
> how long it is. Also I don't know what all of them are called, since the 
> creator have the option of making their own.

I wrote above how you can handle those you recognize. Those you don't
you just have to skip.

> I suppose this should work?
> 
> >         av_strlcpy(variable_buffer_name, buf, (buf_end - buf));
> >         buf += strlen(variable_buffer_name)+1;
> >         av_strlcpy(variable_buffer_type, buf, (buf_end - buf));
> >         buf += strlen(variable_buffer_type)+1;

Now you have added a length that avoids that you overread the on-heap
input buffer but it is still possible to overwrite the on-stack variable
variable_buffer_name. This has improved security not in any relevant
way.

> >> Keeping the
> >> variable_buffer_type is to be able to use it for later in the
> >> interpretation of the different variables. And it also serves for the
> >> purpose of knowing how many bytes to skip to get to the actual data.
> >
> > Huh? It is _not_ used anywhere, not one bit and nowhere at all. Copying
> > data into it is a complete waste of time.
> 
> I actually need it for 1 purpose. To get the length of the variable.

And you need to _copy_ it for that?
Ok, some questions:
Have you ever worked with C strings?
Do you know what they are?
Do you know how to find out the length of one (not! using strlen, strlen
is unsafe)?
If you know the answer to the last one, why do you think that finding the length
requires that you make a copy?