Re: [Libcdio-devel] Rock Ridge and libisofs/xorriso 'AL' extension

[Thomas Schmitt]

Hi,

the git instructions came just in time when my final tests were running.
I did not even have time to dig in my old mailboxes.

Pushed is now my change to address bug 45015 by its two main reasons:
The stray byte in the unused part of the root directory block and also
the up to now "remaining problem" which i numbered "1":
Detecting and handling of directory records which cross block limits.

Crossing is illegal and would indicate that a rogue non-zero byte value
was read from what is believed to be the start of a directory record.
This risk is elevated towards libisofs, because libcdio and Linux accept
any System Use Area data as chain of SUSP fields. Misinterpreted bytes
from a hypothetical other protocol could then easily lead to reading
phony directory record lengths.
Such a phony length made bug 45015 a memory spoiler.

Commit:
  
http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=TS-RockRidge-Fix&id=faf6f6d5399c194db876a3ee07b1920e1447e9fb

---------------------------------------------------------------------

In my previous mail i wrote "non-Rock-Ridge SUSO fields".
That should have been       "non-Rock-Ridge SUSP fields".
We have enough acronyms. No need for me to invent new ones. :))

---------------------------------------------------------------------

Rocky Bernstein wrote:
> Nice would be to a have test case for this change. A test check for lack of
> a memory leak would be great too.

We'd need to test the rogue ISO of bug 45015, whether 

  iso-info --no-joliet -f -i libcdio-heapoverflow-get_rock_ridge_filename.iso

reports no error and all its files:

  ISO-9660 Information
       2048 /a
          0 /b.txt
          3 /a/c.txt

Further we would have to create a healthy ISO, which contains directories
of various lengths (i.e. file counts and name lengthes) but not necessarily
much file content data.
Then iso-info would have to be checked whether it lists the same file names
or at least the same number of files, as were put into the ISO.

--------------------------------------------------------------------

As for my change now, i ran iso-info under valgrind with my collection of
test ISOs. Mostly from distros and bug hunts.
Some are made by non-Linux software.

No valgrind protests.
Also no
  iso-info: Error in opening ISO-9660 image
except with a non-ISO file that should not wear its name suffix ".iso"
and were iso-info correctly reports:
  ++ WARN: unexpected PVD type 0

Further i compared the output lines between a run of the current
HEAD iso-info and my version.

This way i found another ISO which causes an error with the current HEAD.
It has the name "crash.iso" and belongs to
  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=774152
a produce of "AFL" which i assume is also the criminal behind bug 45015:
  https://en.wikipedia.org/wiki/American_fuzzy_lop_%28fuzzer%29

All other output lines match between both iso-info versions.
(I needed to filter away the valgrind lines, of course.)

So for now, i deem my change an improvement over HEAD.

---------------------------------------------------------------------

> I still am not sure I understand the comment:
> > +              The formula does not exactly round up, as it enlarges offset

Normally one would say that the necessary operation is to round up
to the next block limit.

But i also had to consider the borderline case of an offset that is
aligned to a block limit. Then we need no rounding up but rather
have to skip the full block.
Thus the formula as it is now.

It should be equivalent to the one in Linux:

  ctx->pos = (ctx->pos + ISOFS_BLOCK_SIZE) & ~(ISOFS_BLOCK_SIZE - 1);

as long as ISOFS_BLOCK_SIZE is a power of 2.
Else Linux will fail and mine will stay correct. ECMA-119 does not say that
block size 5000 would be illegal. But we all would capsize on such a value,
i assume.


---------------------------------------------------------------------
The change for public review:
---------------------------------------------------------------------

Hopefully final fix for https://savannah.gnu.org/bugs/?45015 
 
diff --git a/lib/iso9660/iso9660_fs.c b/lib/iso9660/iso9660_fs.c
index c528f1e..9c91e3f 100644
--- a/lib/iso9660/iso9660_fs.c
+++ b/lib/iso9660/iso9660_fs.c
@@ -706,6 +706,50 @@ iso9660_iso_seek_read (const iso9660_t *p_iso, void *ptr, 
lsn_t start,
 
 
 
+/*!
+  Check for end of directory record list in a single directory block.
+  If so, set offset to start of next block and return "true". The caller should
+  then skip the next actions in the loop and rather hop to the loop start
+  by "continue".
+  If "false" is returned, then processing of the caller's loop shall go on 
+  normally.
+*/
+static long int
+iso9660_check_dir_block_end(iso9660_dir_t *p_iso9660_dir, unsigned *offset)
+{
+  if (!iso9660_get_dir_len(p_iso9660_dir))
+    {
+      /*
+        Length 0 indicates that no more directory records are in this
+        block. So move on to the next one, which may end the loop.
+        Doing this joins the habits of Linux and libisofs.
+
+        The formula does not exactly round up, as it enlarges offset
+        even if it encounters (offset % ISO_BLOCKSIZE) == 0 .
+        Then the block would be completely 0. Unplausible. But to go
+        on, it has to be skipped.
+      */
+      *offset += ISO_BLOCKSIZE - (*offset % ISO_BLOCKSIZE);
+      return true;
+    }
+
+  if ((*offset + iso9660_get_dir_len(p_iso9660_dir) - 1) / ISO_BLOCKSIZE
+      != *offset / ISO_BLOCKSIZE)
+    {
+      /*
+        Alleged directory record spans over block limit.
+        Hop to next block where a new record is supposed to begin,
+        if it is not after the end of the directory data.
+       */
+      *offset += ISO_BLOCKSIZE - (*offset % ISO_BLOCKSIZE);
+      return true;
+    }
+
+  return false;
+}
+
+
+
 static iso9660_stat_t *
 _iso9660_dir_to_statbuf (iso9660_dir_t *p_iso9660_dir, bool_3way_t b_xa,
                         uint8_t u_joliet_level)
@@ -989,24 +1033,8 @@ _fs_stat_traverse (const CdIo_t *p_cdio, const 
iso9660_stat_t *_root,
       iso9660_stat_t *p_iso9660_stat;
       int cmp;
 
-      if (!iso9660_get_dir_len(p_iso9660_dir))
-       {
-           /*
-
-            libisofs of the libburnia project uses a directory
-            record length of 0 as an indicator to advance to the next block
-            start;  in this situtation the data size field of the directory
-            indicates its end.
-
-             The formula does not exactly round up, as it increases "offset"
-             even if it encounters: (offset % ISO_BLOCKSIZE) == 0 .
-             In that case the block would be completely 0. Unplausible. But to 
go
-             on, it has to be skipped.
-           */
-         offset += ISO_BLOCKSIZE - (offset % ISO_BLOCKSIZE);
-         offset++;
-         continue;
-       }
+      if (iso9660_check_dir_block_end(p_iso9660_dir, &offset))
+       continue;
 
       p_iso9660_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, dunno,
                                        p_env->u_joliet_level);
@@ -1098,11 +1126,8 @@ _fs_iso_stat_traverse (iso9660_t *p_iso, const 
iso9660_stat_t *_root,
       iso9660_stat_t *p_stat;
       int cmp;
 
-      if (!iso9660_get_dir_len(p_iso9660_dir))
-       {
-         offset++;
-         continue;
-       }
+      if (iso9660_check_dir_block_end(p_iso9660_dir, &offset))
+       continue;
 
       p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, p_iso->b_xa,
                                        p_iso->u_joliet_level);
@@ -1313,11 +1338,8 @@ iso9660_fs_readdir (CdIo_t *p_cdio, const char 
psz_path[], bool b_mode2)
        iso9660_dir_t *p_iso9660_dir = (void *) &_dirbuf[offset];
        iso9660_stat_t *p_iso9660_stat;
 
-       if (!iso9660_get_dir_len(p_iso9660_dir))
-         {
-           offset++;
-           continue;
-         }
+       if (iso9660_check_dir_block_end(p_iso9660_dir, &offset))
+         continue;
 
        p_iso9660_stat = _iso9660_dir_to_statbuf(p_iso9660_dir, dunno,
                                                 p_env->u_joliet_level);
@@ -1396,11 +1418,8 @@ iso9660_ifs_readdir (iso9660_t *p_iso, const char 
psz_path[])
        iso9660_dir_t *p_iso9660_dir = (void *) &_dirbuf[offset];
        iso9660_stat_t *p_iso9660_stat;
 
-       if (!iso9660_get_dir_len(p_iso9660_dir))
-         {
-           offset++;
-           continue;
-         }
+       if (iso9660_check_dir_block_end(p_iso9660_dir, &offset))
+         continue;
 
        p_iso9660_stat = _iso9660_dir_to_statbuf(p_iso9660_dir, p_iso->b_xa,
                                                 p_iso->u_joliet_level);
@@ -1608,11 +1627,8 @@ iso_have_rr_traverse (iso9660_t *p_iso, const 
iso9660_stat_t *_root,
       iso9660_dir_t *p_iso9660_dir = (void *) &_dirbuf[offset];
       iso9660_stat_t *p_stat;
 
-      if (!iso9660_get_dir_len(p_iso9660_dir))
-       {
-         offset++;
-         continue;
-       }
+      if (iso9660_check_dir_block_end(p_iso9660_dir, &offset))
+       continue;
 
       p_stat = _iso9660_dir_to_statbuf (p_iso9660_dir, p_iso->b_xa,
                                        p_iso->u_joliet_level);

---------------------------------------------------------------------------


Have a nice day :)

Thomas