Crash on invalid `ifdoffset`

[ziriax]

This line of code should be moved to before the for loop

Otherwise conv.ToUInt16(header, ifdoffset); crashes and no more metadata is read.

So the code should be

                // Field count
                if (ifdoffset > header.Length - 1 || ifdoffset + 2 > header.Length)
                {
                    Errors.Add(new ImageError(Severity.Warning, $"IFD field count overflow for IFD {currentifd}."));
                    continue;
                }
                ushort fieldcount = conv.ToUInt16(header, ifdoffset);
                for (short i = 0; i < fieldcount; i++)

I can provide a PR if you want, but that is most likely more overhead than just moving the line; unless you also want unit tests for this of course...

[ziriax]

Actually, the ifdoffset can also be negative... I guess more checks should be added. At least

if (ifdoffset < 0 || ifdoffset > header.Length - 1 || ifdoffset + 2 > header.Length)

See attached image that reproduces this.

I also notice that you are converting uint to int in a lot of places. Maybe it would be wiser to keep uint where possible, so negative values cannot occur, not sure.

bad_ifd_offset.zip

[ziriax]

Also, the line int totallength = (int)(count * baselength); can cause an overflow.

It would be better to use a long to store this.

Otherwise the line else if (totallength > int.MaxValue) has no effect.

[ArcanoxDragon]

I just encountered this same issue in a product I maintain; I was about to file a new issue and stumbled across this one. I'm going to file a PR to fix the first two things @Ziriax mentioned (moving the ushort fieldcount = ... line to below the if, and adding a negative check) as a start.

[ziriax]

The issue is more severe I'm afraid. conv.ToUInt16 etc is called in many many places without checking the offset. A better solution would be to have these methods return a nullable type, and then force a check on each. Or, maybe easier, throw a specified exception in this converter...