Fix adfGetCacheEntry() crashing on bad data

[kyz]

Debian bug 862740: unadf crashes with segment fault (core dumped)

There is no length checking of the filename or comment length in adfGetCacheEntry() before using memcpy() to copy them into a struct CacheEntry (which only has space for MAXNAMELEN and MAXCMMTLEN respectively)

Furthermore, while the raw data is read from a struct bDirCacheBlock which declares the cache entries as uint8_t records[488], these raw values are converted to signed integers because nLen and cLen in struct CacheEntry are declared as (signed) char, so any value over 0x80 will be interpreted as negative, e.g. -1 to -128, and then when given as the size_t length of bytes to copy, it will be cast to 0xFFFFFFxx or 0xFFFFFFFFFFFFFFxx. Copying this many bytes inevitably crashes the process

There is a proof-of-concept sample file given with the Debian bug.

The fix:

1. Change adfGetCacheEntry() from returning void to returning an error code. Change all the places it's called from so they return failure if it returns failure.
2. Add length checks

I have not changed the definition of cLen and nLen from signed to unsigned char as I haven't looked at all the places they're used, but that could also be done.

[t-w]

I do not know much about that part of ADFlib - but the changes seem OK to me.

cLen and nLen are changed to uint8_t in #34. They are always used as length/size type, are checked only if larger than 0, so nothing should blow up after such change.

Concerning signed/unsigned ints - I have some more concerns about int types in structs (ie. block structures), many of the things seems to me like unsigned values (ie. sizes, block pointers), normally they just cannot be negative (unless negative values have some hidden meaning...). Also, some data types (like SECTNUM) seem like should be unsigned but eg. -1 is used as error return code. Probably it would be good to review int types across the library and (carefully, running tests etc.) try to clean things up. In #34, I have cleaned-up most int types related warnings, mostly just by setting explicit casts, in few cases changing data type in the structures and/or interfaces - but I didn't want to change too much.

For checking the retcode - in the #34, I have added a lot of similar checks on misc. operations, ie. in adf_file - with I/O anything can fail, any data field can be corrupted/tinkered, in many cases the ADFlib's code is not prepared for that. There can be more things like this to correct/make the lib more robust...

[kyz]

cLen and nLen are changed to uint8_t

That is a good change. It eliminates the immediate crash caused by the cast to signed. However, the crash was just delayed until several accumulations of memcpying from beyond the end of a buffer to beyond the size available for a filename/comment.

I added a test case to confirm that introducing the length checks fixes the crash, and removing them restores the crash.

Probably it would be good to review int types across the library and (carefully, running tests etc.) try to clean things up.

I completely agree with this. And once again, thanks for setting the ball rolling by adding so many tests.