#1 Paths in cmtab are not canonicalized before use


When cryptmount mounts a filesystem, the path names for
the device and mount point that go into /etc/mtab are
canonicalized. When unmounting, is_mounted() compares
the original paths stored in configuration to the
canonicalized versions in mtab; if the two are
different, then unmounting fails.

See support request 1543189
for more details on this issue.

The attached patch against cryptmount-1.1 canonicalizes
all paths in configuration while loading it. If any
errors are encountered while canonicalizing (the path
doesn't exist, it can't be accessed, etc.), then the
offending configuration entry will be deleted. Besides
the 'dev' and 'dir' configuration entries, I also
canonicalize the 'loop' and 'keyfile' paths, just in
case there are any related issues hiding in there.

The patched version works on my system (Fedora Core 5
Linux, running kernel 2.6.17-1.2174_FC5 with SELinux
disabled on an AthlonXP), but I have not checked it for
portability, nor have I made any attempt to
internationalize the error messages.

Rennie deGraaf


  • RW Penney

    RW Penney - 2006-09-02

    Logged In: YES

    Thanks for submitting the patch.
    Unfortunately, I don't think this is a good solution as it
    is based on the 'realpath()' system-call.

    The manual pages on my system declare that realpath() "is
    broken by design", and while the version in glibc-2.3.4 is
    almost certainly pretty solid (and supports the use of a
    NULL second argument), it is declared to be either a BSD or
    XOPEN_EXTENDED extension, which doesn't suggest that it's a
    very portable routine. In addition the realpath.c file in
    util-linux-12q includes another implementation of realpath
    on the grounds that "the libc version has some security
    flaws". This all suggests that using realpath() is not a
    good move.

    I suspect a better solution might simply be to adjust the
    'is_mounted()' routine within cryptmount so that it doesn't
    check BOTH the device & mount-point fields returned by
    getmntent(), or to advise users ONLY to use canonicalized
    paths within the cmtab. I suspect the latter would avoid
    risks of opening extra security loopholes for little
    compelling benefit.

  • RW Penney

    RW Penney - 2006-09-02
    • priority: 5 --> 3
    • status: open --> pending-rejected
  • RW Penney

    RW Penney - 2006-09-02

    Logged In: YES

    I've attached a patch that alters the tests in is_mounted()
    to avoid checking the mount-point returned by getmntent()
    against that in the cmtab. Hopefully this will make the
    problem go away, without significantly reducing robustness
    or security.

  • RW Penney

    RW Penney - 2006-09-02

    Patch against cryptmount-1.1 for is_mounted() to mitigate pathname canonicalization issue

  • Rennie deGraaf

    Rennie deGraaf - 2006-09-05

    Logged In: YES

    You're right that people aren't likely to put '.', '..' or
    double slashes in paths (unless you ever add something like
    a "base path" option or home directory-relative paths), and
    the trailing slash is easily stripped. However, the
    existing code /also/ fails if there is a symbolic link in
    the paths of either the image or the mount point - and
    that's a case that is much more likely to appear and cannot
    be detected and corrected so easily. Checking only one of
    the two paths when unmounting (as your patch does) won't
    help here.

    What would you think of adding a safe re-implementation of
    realpath() to your source tree? I could whip together
    another patch if you'd like. Alternately, even an autoconf
    check to use realpath() on systems which have a safe
    implementation, plus a documentation update, would be better
    than nothing.

    By the way, the security flaws in realpath() exist only when
    using a non-null 2nd argument. (Of course, this is
    non-standard and not supported by all systems.) If the
    second argument is used, then realpath() is prone to buffer
    overflows, since there is no way to predict an upper bound
    on a canonicalized path. The version in util-linux adds a
    buffer length argument to make it safe to use with a
    pre-allocated buffer. There's another always-safe
    realpath() replacement out there as a GNU libc extension
    called canonicalize_file_name(); this one only takes one
    argument and always malloc()s the result.

  • Rennie deGraaf

    Rennie deGraaf - 2006-09-05
    • priority: 3 --> 5
    • status: pending-rejected --> open
  • RW Penney

    RW Penney - 2006-09-05
    • priority: 5 --> 3
    • status: open --> open-postponed
  • RW Penney

    RW Penney - 2006-09-05

    Logged In: YES

    I take your point about symbolic links, however from my
    experiments, the one-line patch that I've already attached
    to this bug-report (02Sep06) deals perfectly well with
    mount-points that contain symbolic links (as well as './'
    '//' oddities). This only seems to leave the issue of
    /dev/mapper/[target-name] containing symbolic links, which
    seems rather less likely to occur than for mount-directories.

    Thanks for the offer of a realpath() patch. However, this
    sounds like it's based on a fundamentally dubious approach -
    trying to emulate the canonicalization performed by
    util-linux/realpath.c through a routine that would be
    written/maintained completely independently.

    I'd recommend that a period of quiet contemplation is needed
    to identify a more portable and maintainable solution that
    goes beyond the patch I've already suggested. I believe this
    patch already considerably reduces the possible impact of
    the canonicalization issue.


  • RW Penney

    RW Penney - 2006-09-22
    • status: open-postponed --> closed-fixed
  • Nobody/Anonymous

    This design is spectacular! You most certainly know how to keep a reader entertained. Between your wit and your videos, I was almost moved to start my own blog (well, almost...HaHa!) Fantastic job. I really loved what you had to say, and more than that, how you presented it. Too cool!
    north face jackets clearance http://northfacecoatoutlet.hpage.com/