[FFmpeg-devel] [PATCH] Add XPM decoder

[Nicolas George]

Le duodi 22 ventôse, an CCXXV, Paul B Mahol a écrit :
> Because conflicting entries have not been added yet. Last time I
> compared it was different.

Well, unlike some people on this mailing-list, I actually check my facts
before sending a mail. And I repeat, I did not see any conflict.

> Also when Last time I tried it was soo slow that made 10k colors very
> slow to decode.

Then make it faster, since obviously you are capable of it, but
duplicating it is unacceptable.

> > convert is not a very good name.
> 
> OK, what you propose?

hex_char_to_number(), for example.

> >> +            for (j = 0; reject && reject[j]; j++) {
> >> +                if (string[i] == reject[j])
> >> +                    break;
> > Use strchr().
> That is slower.

I very much doubt that.

> >> +static uint32_t hexstring_to_rgba(const char *p, int len)
> > This is a misnomer.
> What it should be instead?

Probably "color_string_to_rgba()".

> >> +        strncpy(color_name, p, len);
> >> +        color_name[len] = '\0';
> > This is completely wrong.
> What should it be instead?

It should check len against sizeof(color_name), obviously. Could you not
find it yourself? The magic number in the size of the array should have
been a dead giveaway.

> Better not return error and instead display what is already decoded.

I strongly disagree.

> > Also, you forgot to parse colors in standard X11 scheme
> > "rgb:RRRR/GGGG/BBBB".
> Are there such files?

Of course.

> > If I read this correctly, you are skipping random characters until a
> > quote is found. This is not how a robust parser should be written.
> Come on.

Are you trying to communicate?

> >> +    if (sscanf(ptr, "\"%u %u %u %u\",",
> >> +               &avctx->width, &avctx->height, &ncolors, &cpp) != 4) {
> > This is not properly checking the final quote and comma.
> Really?

Yes, really. Check the man page of sscanf() if you do not remember how
it works.

> >> +    size = 1;
> >> +    j = 1;
> >> +    for (i = 0; i < cpp; i++) {
> >> +        size += j*94;
> >> +        j *= 95;
> >> +    }
> >> +    size *= 4;
> >
> > This is a DoS waiting to happen.
> Come on. I fuzzed this with afl-fuzzer up to 25 cycles.

You should have given 25 seconds of thought instead. An attacker has
only to make cpp just large enough to eat all memory and give a few
colors to force the allocation of very sparse entries to actually access
it to make a DoS.

> >> +    if (size < 0) {
> > This is deliberately invoking an undefined behaviour.
> How?

The arithmetic can not make size negative, only an integer overflow.

Furthermore, if there are several integer overflows, size can come back
positive but smaller than what will be accessed later, which is really
really bad.

Actually, I think you just pushed an exploitable security hole.

> >> +    ptr += mod_strcspn(ptr, ",") + 1;
> > Same remark as above: skipping random contents. Same for other uses of
> > mod_strcspn().
> It is not skipping random contents.

Oh? Then pray tell me what part of the code detects an invalid file with
random text at between the quote and the comma?

> > index looks like a misnomer.
> It is name for index to pixels in XPM structure.

An index is a number, this is not a number.

> > This whole loop can go beyond the end of the input buffer very easily.
> Input buffer is padded with NULLs.

My bad, re-reading it more carefully, you were right on this instance.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170312/7407c71f/attachment.sig>