rawspeed: implement a safer way to bounds check

[pedrocr]

Rawspeed works by reading the whole file into memory and then indexing into it. This together with using file inputs as offsets can lead to out of bounds reads in memory. Implement a better internal API to make these checks both easier to write and harder to skip. This covers three main cases:

1. Whenever we are accessing data directly from the file we used to do something like:

uchar8 *data = mFile->getData(offset);

Replace that with:

uchar8 *data = mFile->getData(offset, len);

and then do proper bounds checking to make sure we can safely read len bytes from the returned pointer.

2. When we want to read a byte stream we will often do something like:

ByteStream in(mFile->getData(off), mFile->getSize()-off);

which basically means we allow reading from the offset all the way to the end of the file. Make that simpler by just doing:

ByteStream in(mFile, off);

and have that do all the needed bounds checking.

3. When we want to read a byte stream up to a certain number of bytes we will do something like:

ByteStream in(mFile->getData(off), len);

which may be unsafe if len > mFile->getSize()-off. Instead do:

ByteStream in(mFile, off, len);

and that will make sure the bounds checking is done properly.

[pedrocr]

@LebedevRI I've only tested this very lightly but it should go a long way to helping reduce the out of bounds accesses you've found in a more robust way than trying to whack-a-mole everywhere. The next step would probably be to require a size parameter in TiffEntry::getData() to do similar checks.

[klauspost]

I like it. There are no expensive checks, and it makes the code cleaner a lot of places.

A PR upstream is welcome!

[pedrocr]

@LebedevRI can I update this and merge or do you still want to do some other changes before?

[LebedevRI]

@pedrocr
I don't care. That was my last attempt as rawspeed contribution.
All those CVE's will stay there until someone else re-discovers them again.
Oh, and leaks too :)

[pedrocr]

@LebedevRI No clue why you're doing that but it's your call. Are you also not going to submit upstream what you've already changed?

[pedrocr]

This is the current diff between darktable and rawspeed upstream:

https://gist.github.com/pedrocr/3c8231de996fa1f330af

Those are all from your commits @LebedevRI.

[wgoetz]

Pedro Côrte-Real wrote:

Merged #1099.

black output! nikon d800 and 1j5

[rawspeed] FileMap: Attempting to read file out of bounds.
[rawspeed] FileMap: Attempting to read file out of bounds.

last working commit:
fbede70 rawspeed: add a
zero_is_not_bad for future use

---

Reply to this email directly or view it on GitHub:
#1099 (comment)

[pedrocr]

@wgoetz master should work fine again. Could you please submit a 1J5 file to rawsamples.ch so we have something to do regression testing against? Thanks

[LebedevRI]

On Sun, Feb 7, 2016 at 10:36 PM, Pedro Côrte-Real notifications@github.com
wrote:

@LebedevRI https://github.com/LebedevRI No clue why you're doing that
but it's your call.

Are you also not going to submit upstream what you've already changed?

Correct. You may revert those commits if you wish so.

—

Reply to this email directly or view it on GitHub
#1099 (comment)
.