Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read

[Thomas Schmitt]

Hi,

i will review the patches hopefully tomorrow.

But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP
entry has 5 bytes of size.  This is not true. The minimum size is 4.
In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST,
which have 4 bytes. In RRIP there is "Relocated" RE.
Other SUSP-compliant protocols could define 4-byte entries, too.

I will have to analyze the patches whether the assumption of 5 bytes
minimum can cause real harm.

But i see at least two inappropriate applications of the minimum size:

In [2/4] a RRIP NM entry is processed:
  -         csize = entry->len - 5;
  +         csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
The number 5 is meant to skip over the 4 bytes of SUSP header and the one
byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to
NM (version 1, to be exacting), not to SUSP in general.
I propose to leave the 5 as is.

In [4/4] a RRIP SL entry is processed:
-      /* The symlink is not stored as a POSIX symlink, translate it.  */
-      while (pos + sizeof (*entry) < entry->len)
+      /* The symlink is not stored as a POSIX symlink, translate it. */
+      while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
At least in a quick test in GNU/Linux userspace
  struct grub_iso9660_susp_entry {
    uint8_t sig[2];
    uint8_t len;
    uint8_t version;
    uint8_t data[0];
  };
has sizeof 4, not 5.
So i see the risk that this change alters program behavior in situations
where we don't perceive a problem yet.

It is too late in the evening to analyze what sizeof(*entry) has to do
with reading the component records of SL. The component records are the
components of the link target path with 2 bytes header plus the name
bytes. So a size of 3 is plausible like in .../b/... Even a size of 2
is not totally illegal, as Linux allows paths like ...//....


Have a nice day :)

Thomas