[Bug 203645] makefs: Coverity CID 976312: SIGSEGV with option -l 3

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203645

            Bug ID: 203645
           Summary: makefs: Coverity CID 976312: SIGSEGV with option -l 3
           Product: Base System
           Version: 11.0-CURRENT
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Some People
          Priority: ---
         Component: bin
          Assignee: freebsd-bugs at FreeBSD.org
          Reporter: scdbackup at gmx.net

usr.sbin/makefs/cd9660.c

CID 976312: Explicit null dereferenced (FORWARD_NULL)i
   4. var_deref_op: Dereferencing null pointer conversion_function.

1740        return (*conversion_function)(oldname, newname, is_file);

--------------- Source analysis:

The function pointer is non-NULL only if diskStructure.isoLevel is
1 or 2. A snippet in cd9660_parse_opts() indicates that the value
can indeed be 3:

         option_t cd9660_options[] = {
                 { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" },
                 { "isolevel", &diskStructure.isoLevel, 1, 3, "ISO Level" },
                 ...



The line 1740 is in function cd9660_convert_filename().
ISO 9660 level 3 allows the same file names as level 2.

The use of ISO level 3 is not announced anywhere in the ISO
image but rather becomes visible only if a file is large enough
to need more than one extent. Extents can have 4 GiB - 1 byte
of size.

The offer of ISO level 3 is questionable because neither FreeBSD
nor NetBSD can read files with multiple extents from ISO 9660.
  http://lists.freebsd.org/pipermail/freebsd-hackers/2012-April/038552.html

Surely nobody has ever created a level 3 ISO with makefs.
Not only would it SIGSEGV, but also the struct stat.st_size value
is assigned to int64_t cd9660node.fileDataLength without any check in
cd9660.c line 871:
                newnode->fileDataLength = node->inode->st.st_size;
Later it is handed over to cd9660_bothendian_dword()
which expects uint32_t (see cd9660/cd9660_conversion.c)
cd9660.c line 834:
        cd9660_bothendian_dword(newnode->fileDataLength,
            newnode->isoDirRecord->size);
So only one extent will be produced with a size as given by the
low 4 bytes of the disk file size.

--------------- Remedy proposal:

Restrict isoLevel to 1 and 2.

-                { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" },
-                { "isolevel", &diskStructure.isoLevel, 1, 3, "ISO Level" },
+                { "l", &diskStructure.isoLevel, 1, 2, "ISO Level" },
+                { "isolevel", &diskStructure.isoLevel, 1, 2, "ISO Level" },

Restrict data file size to 4 GiB - 1 byte.

        /* Set the size */
-       if (!(S_ISDIR(node->type)))
+       if (!(S_ISDIR(node->type))) {
+               if (node->inode->st.st_size > (off_t) 4096 * 1024 * 1024 - 1) {
+
+                       >>> error out in appropriate way <<<
+
+               }
                newnode->fileDataLength = node->inode->st.st_size;
+       }

To appease checkers use the same conversion function for all
levels above 1:

-         else if (diskStructure.isoLevel == 2)
+         else
                  conversion_function = &cd9660_level2_convert_filename;

-- 
You are receiving this mail because:
You are the assignee for the bug.