[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Reimar Döffinger]

On Tue, Jul 04, 2017 at 09:50:31PM +0530, Paras Chadha wrote:
> On Tue, Jul 4, 2017 at 4:12 AM, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> wrote:
> > > +            data_size *= naxisn[i];
> > > +            if (data_size < 0)
> > > +                return AVERROR_INVALIDDATA;
> >
> > If this is supposed to be overlow check as well: same issue as before: you
> > need to PREVENT the overflow, afterwards it's too late, at least for signed
> > types.
> > Also the golden rule of input sanitization: sanitize/range check FIRST,
> > THEN do the arithmetic.
> > NEVER do it the other way run without at least spending 30 minutes per
> > operation making sure it's really still safe.
> >
> 
> Actually, here also overflow is not possible unless someone intentionally
> put values of naxisn[i] that would cause overflow. Most of the FITS files
> are in MB's or at max in GB's (although i have not seen any in GB). So, if
> someone intentionally put wrong values of size in header then it would
> crash or cause undefined behavior.

crash == usually exploitable
undefined behaviour == possibly exploitable with risk of becoming
exploitable at any point of time with new compiler versions etc.

Both are completely unacceptable.

> Just to prevent that i have added a
> check.

The problem is, your check doesn't prevent it.
It SOMETIMES MIGHT detect it, AFTER it happened.
You can reduce the problem somewhat by using unsigned types,
then at least the overflow would not be undefined behaviour,
but then you have lots of weird corner-case that ALL the
following code must ALWAYS deal correctly with.
For example (not that you would have this specific code in FFmpeg):
ptr = malloc(data_size);
fread(ptr, 1, naxisn[0], somefile);
would be exploitable if you had an overflow.

> So, can suggest a better way to do this ?

As said, always check the inputs BEFORE use.
One way would be
if (naxisn[i] > INT_MAX / data_size) goto err_out;
However you still have to carefully decide on
which limit value (INT_MAX in my example) is the
right compromise between limiting use-cases and
making the code easy to write and maintain.
If you choose it too close, the following code
can't even safely do "data_size + 1" without
another overflow check.
av_image_check_size might be a guideline,
as well as the default value of max_alloc_size.

> Regarding range check, FITS
> standard does not have any limit on the range of values of dimension,
> theoretically it can be anything but practically ofcourse it cannot exceed
> the limits of 64 bit integer. So, how should i go about this ?

As you can see from max_alloc_size, anything
that results in more than 2 GB in a single
packet won't really work by default anyway.
(3D images COULD have their 3rd dimension split
up into separate packages for example, but the
might be something better to leave for later?).

> > > +    fits->image = 0;
> > > +    pos = avio_tell(s->pb);
> > > +    while (!fits->image) {
> > > +        if ((ret = avio_seek(s->pb, pos+size, SEEK_SET)) < 0)
> > > +            return ret;
> > > +
> > > +        if (avio_feof(s->pb))
> > > +            return AVERROR_EOF;
> > > +
> > > +        pos = avio_tell(s->pb);
> > > +        if ((size = find_size(s->pb, fits)) < 0)
> > > +            return size;
> > > +    }
> > > +
> > > +    if ((ret = avio_seek(s->pb, pos, SEEK_SET)) < 0)
> > > +        return ret;
> >
> > Does this seek ever do anything?
> >
> 
> Yes, it seeks back the pointer to point to the begining of the header.

Yes, I failed at reading code (missed that the find_size does
the whole header reading).
Maybe you should just be using av_append_packet while reading the
header? So that when you are done with parsing the header, you
already have all the header data in the packet and the just need
to use av_append_packet again to get the image data itself.
(av_append_packet is not perfect/the most efficient way of doing it,
but if the last bit of speed does not matter so much it tends
to be easy way to avoid seeking in demuxer without increasing complexity
much, sometimes even simplifying things).