Bug#872590: libisofs6: integer overflow in susp_iter_next()

2017-08-21 Thread Thomas Schmitt 
Hi,

all the testing did not catch the new bug by which i prevented
multi-session reading. It only showed up with the first incremental
backup run after installing the new xorriso version on my workstation.

Hopefully fixed by
  
https://dev.lovelyhq.com/libburnia/libisofs/commit/a7152f57942c01f46885c5ead6380021cfa466ad


Have a nice day :)

Thomas

Bug#872590: libisofs6: integer overflow in susp_iter_next()

2017-08-19 Thread Thomas Schmitt 
Hi,

the immediate trigger of the bug is fixed by commit
  
https://dev.lovelyhq.com/libburnia/libisofs/commit/91490d5f34422d514b042a9e597be8d614a3a1ea
  "Preventing use of zero sized SUSP CE entry which causes SIGSEGV.
   Debian bug 872590. Thanks Jakub Wilk and American Fuzzy Lop."

Further i installed a size curb for loading CE areas. For that i now only
read the amount of blocks which is necessary and refuse on more than 1 MiB.
  
https://dev.lovelyhq.com/libburnia/libisofs/commit/31088d9acc154779975da0eb429fb680971274f9
  "Avoid to read blocks from start of CE area which do not belong to the
   given file"

A test for proper block addresses of CE areas was added:
  
https://dev.lovelyhq.com/libburnia/libisofs/commit/2a64d89e6e6fee4893ab4809342870acba0ea465
  "Refuse to read CE data blocks from after the end of ISO filesystem"


If the second or third change are bad, then the regression consequences are
severe. So i compared ISO reading of xorriso-1.4.6 with the one of development
version 1.4.7. The results got cleaned of systematic differences by a few
sed expressions:

  xorriso=... path to xorriso 1.4.6 or 1.4.7 ...
  run=... "old" or "new" ...

  for i in *.iso
  do
valgrind "$xorriso" \
  -report_about note -for_backup -hfsplus on \
  -indev "$i" \
  -find / -exec lsdl -- \
  -find / -has_any_xattr -exec get_any_xattr -- \
  -find / -exec getfacl -- \
  2>&1
  done \
  | sed \
 -e 's/^==[0-9][0-9]*=//' \
 -e 's/xorriso 1.4.[67]/xorriso 1.4.X/g' \
 -e 's/^= Command:.*$//' \
 -e 's/^=   total heap usage:.*$//' \
  >/tmp/log_"$run"

Comparison of both log files yields only insignifcant differences and the
improvements which were achieved by fixing the recent bugs found by AFL.
The log file size with my ISO 9660 collection is 90 MB. So there is a decent
statistical base.


Have a nice day :)

Thomas

Bug#872590: libisofs6: integer overflow in susp_iter_next()

2017-08-19 Thread Thomas Schmitt 
Hi,

this is not so eay to fix. Lots of potential holes for the rabbit
to sneak in.

A big problem is this plan of my predecessor developer in libisofs:
/* read all blocks needed to cache the full CE */

The continuation area is allowed to be up to 4 GiB of size.
iter->ce_off is the uint32_t byte offset in that area.
iter->ce_len is the uint32_t number of bytes which belong to the
current file's area within the big shared CE area.

This prevents me from flatly refusing on oversized nblocks values.
Even in regular situations it would be possible that a CE area has
dozens of MiB, because it consolidates SUSP data from many files.

So i will have to change this to only reading of the file's CE blocks.
This might have consequences in other parts of the code.


Next problem is that class SuspIterator knows of the imported ISO
only the aspect of class IsoDataSource, which cannot yet tell the
number of its valid blocks.
Without this info i cannot judge the validity of a read address
before trying to read it. Depending on the medium, a read operation
might succeed even if the block is outside the ISO filesystem size.


Both problems are not unfixable. But the fixes affect other code
parts. Changing the API class IsoDataSource would not solve the problem
because then i would have to keep the older dumb version valid for
API/ABI compatibility.

I shall not forget the risk of regressions which rises when small
holes get fixed by big band-aids. See libcdio's
  https://savannah.gnu.org/bugs/?45015
where the heavy-handed fix of an AFL-found bug caused that all Rock Ridge
info was ignored in ISOs with AAIP entry AL.


So this might last a while.


Have a nice day :)

Thomas

Bug#872590: libisofs6: integer overflow in susp_iter_next()

2017-08-18 Thread Jakub Wilk 

Package: libisofs6
Version: 1.4.6-1

The susp_iter_next() function does the following:

  nblocks = DIV_UP(iter->ce_off + iter->ce_len, BLOCK_SIZE);
  iter->buffer = realloc(iter->buffer, nblocks * BLOCK_SIZE);
  for (block = 0; block < nblocks; ++block) {
  /* ... */
  }
  iter->base = iter->buffer + iter->ce_off;

(I omitted the boring parts.)

An overflow can happen in the computation of nblocks.
For example, in the attached ISO file:
- iter->ce_off is 4294901808;
- iter->ce_len is 65328;
- nblocks is computed as 0;
- iter->base is set to a bogus pointer.

Found using American Fuzzy Lop:
http://lcamtuf.coredump.cx/afl/

--
Jakub Wilk


intoverflow.iso.gz
Description: application/gzip