Bug 203645 - makefs: Coverity CID 976312: SIGSEGV with option -l 3
Summary: makefs: Coverity CID 976312: SIGSEGV with option -l 3
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Enji Cooper
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-08 18:41 UTC by scdbackup
Modified: 2017-07-24 16:47 UTC (History)
2 users (show)

See Also:
ngie: mfc-stable10+
ngie: mfc-stable9+
ngie: mfc-stable8-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description scdbackup 2015-10-08 18:41:23 UTC
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;
Comment 1 scdbackup 2015-10-19 21:26:54 UTC
Also present in NetBSD.

http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.sbin/makefs/cd9660.c?annotate=1.49

280:                OPT_NUM('l', "isolevel", isoLevel,
281:                    1, 3, "ISO Level"),

879:        if (!(S_ISDIR(node->type)))
880:                newnode->fileDataLength = node->inode->st.st_size;

1772:        else if (diskStructure->isoLevel == 2)
1773:                conversion_function = &cd9660_level2_convert_filename;
1774:        return (*conversion_function)(diskStructure, oldname, newname, is_file);
Comment 2 commit-hook freebsd_committer 2015-11-02 08:43:20 UTC
A commit references this bug:

Author: ngie
Date: Mon Nov  2 08:43:15 UTC 2015
New revision: 290264
URL: https://svnweb.freebsd.org/changeset/base/290264

Log:
  Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by
  dereferencing a NULL function pointer

  Add some asserts to ensure that isolevel is always either 1 or 2.

  PR: 203645
  Reported by: Thomas Schmitt <scdbackup@gmx.net>
  MFC after: 1 week
  Sponsored by: EMC / Isilon Storage Division

Changes:
  head/usr.sbin/makefs/cd9660.c
Comment 3 commit-hook freebsd_committer 2015-11-02 09:17:23 UTC
A commit references this bug:

Author: ngie
Date: Mon Nov  2 09:16:51 UTC 2015
New revision: 290265
URL: https://svnweb.freebsd.org/changeset/base/290265

Log:
  Add testcases for -t cd9660 -o isolevel=[1-3]

  -- -o isolevel=1 currently fails because of path comparison issues,
     so mark it as an expected failure.
  -- -o isolevel=3 is not implemented, so expect it to fail as an out
     of bounds value [*].

  PR: 203645
  MFC after: 1 week
  X-MFC with: r290264
  Sponsored by: EMC / Isilon Storage Division

Changes:
  head/usr.sbin/makefs/tests/makefs_cd9660_tests.sh
Comment 4 scdbackup 2015-11-02 10:38:10 UTC
The patch does not care for oversized data files,
which currently get misrepresented silently.
Such a data file should rather be excluded or
taken as reason for abort.
Comment 5 commit-hook freebsd_committer 2015-11-09 09:20:19 UTC
A commit references this bug:

Author: ngie
Date: Mon Nov  9 09:20:01 UTC 2015
New revision: 290594
URL: https://svnweb.freebsd.org/changeset/base/290594

Log:
  MFC r290265,r290267,r290270:

  r290265:

  Add testcases for -t cd9660 -o isolevel=[1-3]

  -- -o isolevel=1 currently fails because of path comparison issues,
     so mark it as an expected failure.
  -- -o isolevel=3 is not implemented, so expect it to fail as an out
     of bounds value [*].

  PR: 203645
  Sponsored by: EMC / Isilon Storage Division

  r290267:

  Clean up mtree keyword support a slight bit and add a few more default keywords

  - Parameterize the mtree keywords as $DEFAULT_MTREE_KEYWORDS
  - Test with the extra mtree keywords, `mode,gid,uid`.
  - Add a note about mtrees with time support not working with makefs right now

  Sponsored by: EMC / Isilon Storage Division

  r290270:

  Add testcases for -t ffs -o version=[12]

  Verify the filesystem type using dumpfs. Add preliminary support
  for NetBSD (needs to be validated)

  Sponsored by: EMC / Isilon Storage Division

Changes:
_U  stable/10/
  stable/10/usr.sbin/makefs/tests/makefs_cd9660_tests.sh
  stable/10/usr.sbin/makefs/tests/makefs_ffs_tests.sh
  stable/10/usr.sbin/makefs/tests/makefs_tests_common.sh
Comment 6 commit-hook freebsd_committer 2015-11-09 09:22:21 UTC
A commit references this bug:

Author: ngie
Date: Mon Nov  9 09:22:12 UTC 2015
New revision: 290595
URL: https://svnweb.freebsd.org/changeset/base/290595

Log:
  MFC r290264:

  Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by
  dereferencing a NULL function pointer

  Add some asserts to ensure that isolevel is always either 1 or 2.

  PR: 203645
  Reported by: Thomas Schmitt <scdbackup@gmx.net>
  Sponsored by: EMC / Isilon Storage Division

Changes:
_U  stable/10/
  stable/10/usr.sbin/makefs/cd9660.c
Comment 7 commit-hook freebsd_committer 2015-11-09 09:23:23 UTC
A commit references this bug:

Author: ngie
Date: Mon Nov  9 09:23:13 UTC 2015
New revision: 290596
URL: https://svnweb.freebsd.org/changeset/base/290596

Log:
  MFstable/10 r290595:

  MFC r290264:

  Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by
  dereferencing a NULL function pointer

  Add some asserts to ensure that isolevel is always either 1 or 2.

  PR: 203645
  Reported by: Thomas Schmitt <scdbackup@gmx.net>
  Sponsored by: EMC / Isilon Storage Division

Changes:
_U  stable/9/
_U  stable/9/usr.sbin/
_U  stable/9/usr.sbin/makefs/
  stable/9/usr.sbin/makefs/cd9660.c
Comment 8 Enji Cooper freebsd_committer 2015-11-09 09:50:39 UTC
(In reply to scdbackup from comment #4)
> The patch does not care for oversized data files,
> which currently get misrepresented silently.
> Such a data file should rather be excluded or
> taken as reason for abort.

ISO-9660 level 1/2 both need to be fixed and ISO-9660 level 3 needs to be completely implemented. Feel free to post follow up bugs. I just wanted to make sure the segfault was corrected as implementing level 3 seemed like a larger task to perform for this bug.
Comment 9 scdbackup 2015-11-09 11:11:11 UTC
I agree that it is most important to put a cap on that SIGSEGV.

But implementing multi-extent files (which effectively constitute ISO 9660
level 3) would be a substantial change.

Further it will not be enough to enhance makefs alone, because the readonly
filesystem implementation in the FreeBSD kernel is unable to properly
represent such files.

So for now - and only to complete the safety cap - makefs should refuse to
record data files which must be represented as multi-extent files. I.e.
files with more than 4 GiB -1 bytes of content.
This test is needed anyways to make sure that chosen level 1 and 2 do not
exceed the limits which are specified in ECMA-119 chapter 10 "Levels of
interchange":

"10.1 Level 1.
 - each file shall consist of only one File Section;
 [...]
 10.2 Level 2
 - each file shall consist of only one File Section;
 [...]
"
File Section is what other filesystems name "Extent".
The byte counter of a File Section is an unsigned  32 bit number (written
doubly in both endiannesses, as usual in ISO 9660).

So if the --isolevel option is given, then user may expect to be protected from
shooting the own foot by oversized data files.
Comment 10 Enji Cooper freebsd_committer 2017-07-24 16:47:29 UTC
(In reply to scdbackup from comment #9)

Closing bug. Will talk with emaste about implementing the enhancements discussed in comment # 9 and will CC you on the bug.

Please don't reopen this bug.