Bug 707

Hello,

Just confirmed that in a few directories (/home), you can create a symbolic link that will allow you to open any file, including /etc/shadow, even though is not in the sudoers list

It is never safe to grant a user sudo permission to edit or execute a file in a directory that is writable by the user.

One user has proposed adding a NOLINK option to disable following symbolic links with sudoedit.  This could work by using the O_NOFOLLOW flag to the open(2) system call on systems that support it.  I'm wary of rejecting attempts to edit via a symbolic link by default as this may break legitimate use cases.

I'm currently testing a change to add a FOLLOW/NOFOLLOW tag along with a sudoedit_follow flag that defaults to being false.

My current plan is to restrict sudoedit to not follow links unless specifically enabled in sudoers.  The following commit implements a sudoedit_follow option which defaults to false and FOLLOW/NOFOLLOW command tags for more fine-grained control.

http://www.sudo.ws/repos/sudo/rev/9636fd256325

Fixed in sudo 1.8.15

I suspect this patch is insufficient; O_NOFOLLOW only works on the last pathname component.

Try the following:

$ mkdir tests
$ cd tests
$ ln -s /etc etc
$ ln -s /etc/passwd passwd
$ ln -s /etc/shadow shadow

Now try to edit the following files: ./etc/passwd ./etc/shadow ./passwd ./shadow


I wrote a tiny test program to demonstrate the issue that does something similar, see attachments on the bug at https://bugs.launchpad.net/ubuntu/+source/sudo/+bug/1512781

I'm not sure this problem is actually solvable.

Thanks

There's no way to check the writability of more than the immediate parent directory without introducing time of check vs. time of use issues.  The best that we can do is to protect against following a symlink as the last element of a path or a writable parent directory.

Ultimately, this is a configuration error.  Just as you would not give access to run commands via sudo along a user-writable path so you should not give sudoedit access to a user-writable path.

> There's no way to check the writability of more than the immediate
> parent directory without introducing time of check vs. time of use
> issues. [...] Ultimately, this is a configuration error.

Well, a configuration error ought to generate an error and not lead to an unexpected privilege escalation, IMO.

There must be ways to improve the situation.

You can ensure that none of the parent directories are writable by the user. If they are not writable, the user has no chance to change anything and thus rely on a time of check vs time of use issue. Or am I missing something?

You should be able to build trust starting with / and building up the full path:
- a directory/symlink in an unwritable directory is safe and can be kept
- as soon as you hit a directory/symlink that is writable or owned by the user, you are unsafe and should abort. As an exception to this rule, we can consider the case of the next component in the path being an immutable directory. Since the user cannot do anything in that directory (no new files, no removal), it can still be considered safe.

Right now, I don't feel like CVE-2015-5602 is properly addressed.

As you say, problem is directory writability, not symlinks.  As far as I know it is not possible to overwrite an existing symlink without the directory being writable (please correct me if I am wrong).

Sudo 1.8.15 introduced an option to check the writability of the parent directory but I was not convinced that it was possible to check the entire path without time of check vs. time of use issues.  On further reflection however, I think it should be possible to do using openat(2).

(In reply to Todd C Miller from comment #9)
> As you say, problem is directory writability, not symlinks.  As far
> as I know it is not possible to overwrite an existing symlink
> without the directory being writable (please correct me if I am
> wrong).

Correct as far as I know.

> Sudo 1.8.15 introduced an option to check the writability of the
> parent directory

Which doesn't work, because the final openat() uses the full path:

> +	return -1;
> +    }
> +
> +    fd = openat(dfd, path, oflags, mode);

Should be:
    fd = openat(dfd, base, oflags, mode);

> +    close(dfd);
> +    return fd;
> +}

> but I was not convinced that it was possible to
> check the entire path without time of check vs. time of use issues. 
> On further reflection however, I think it should be possible to do
> using openat(2).

Yes, I'm preparing a patch that does that.

Created attachment 466 [details]
CVE-2015-5602: Fix directory writability checks for sudoedit

I've committed a modified version of your patch.
https://www.sudo.ws/repos/sudo/rev/c2e36a80a279

Fixed in 1.8.16, available now.