On 02/24/2017 04:34 AM, Greg Kurz wrote:
> On Thu, 23 Feb 2017 15:10:42 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
>> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote:
>>> The local_chmod() callback is vulnerable to symlink attacks because it
>>> calls:
>>>
>>> (1) chmod() which follows symbolic links for all path elements
>>> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>>>     path elements
>>> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>>>     mkdir(), both functions following symbolic links for all path
>>>     elements but the rightmost one
>>>

>>> +        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
>>> +        if (fd == -1) {
>>> +            return -1;
>>> +        }
>>> +        ret = fchmod(fd, credp->fc_mode);
>>> +        close_preserve_errno(fd);  
>>
>> As mentioned on IRC, not sure this is workable since chmod(2) must not
>> require read permission on the file.  This patch introduces failures
>> when the file isn't readable.
>>
> 
> Yeah :-\ This one is the more painful issue to address. I need a
> chmod() variant that would fail on symlinks instead of following
> them... and I'm kinda suffering to implement this in a race-free
> manner.

BSD has lchmod(), but Linux does not.  And Linux does not yet properly
implement AT_SYMLINK_NOFOLLOW.  (The BSD semantics are pretty cool:
restricting mode bits on a symlink can create scenarios where some users
can dereference the symlink but others get ENOENT failures - but I don't
know of any effort to add those semantics to the Linux kernel)

POSIX says support for AT_SYMLINK_NOFOLLOW is optional, precisely
because POSIX does not require permissions on symlinks to matter, but
the way it requires it is that AT_SYMLINK_NOFOLLOW is supposed to be a
no-op for regular files, and cause an EOPNOTSUPP failure for symlinks.

Unfortunately, the Linux man page says that Linux fails with ENOTSUP
(which is identical to EOPNOTSUPP) any time AT_SYMLINK_NOFOLLOW is set,
and not just if it is attempted on a symlink.  And I just verified that
poor behavior as follows:

$ cat foo.c
#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <unistd.h>
#include <errno.h>

int main(int argc, char **argv)
{
    int ret = 1;
    if (symlink("a", "l") < 0)
        goto cleanup;
    if (fchmodat(AT_FDCWD, "l", 0600, AT_SYMLINK_NOFOLLOW) == 0)
        printf("kernel supports mode bits on symlinks\n");
    else if (errno != EOPNOTSUPP) {
        printf("Oops, kernel failed with %m on symlink\n");
        goto cleanup;
    }
    if (creat("f", 0600) < 0)
        goto cleanup;
    if (fchmodat(AT_FDCWD, "f", 0600, AT_SYMLINK_NOFOLLOW) < 0) {
        printf("Oops, kernel failed with %m on regular file\n");
        goto cleanup;
    }
    ret = 0;
 cleanup:
    remove("l");
    remove("f");
    return ret;
}
$ ./foo
Oops, kernel failed with Operation not supported on regular file

If you were to get that kernel bug fixed, then you could use fchmodat()
to do a safe chmod() which does not chase through a final symlink, and
relative to a safe directory fd that you obtain for the rest of the path
(as done elsewhere in the series).  Use the CVE nature of the problem as
leverage on the kernel developers, if that's what it takes.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org