sysfs: Add hook for checking the file capability of opener

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Quoted 5 lines · Chun-Yi

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Don't you mean "open by privileged and then written by unprivileged?" Or if not, exactly how is this a problem? You check the capabilities when you do the write and if that is not allowed then, well

And you are checking the namespace of the person trying to do the write when the write happens, which is correct here, right?

If you really want to mess with wake locks in a namespaced environment, then put it in a real namespaced environment, which is {HUGE HINT} not sysfs.

So no, this patch isn't ok...

Hi Greg,

Quoted 11 lines · Greg Kroah-Hartman

Quoted 5 lines · Chun-Yi

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Don't you mean "open by privileged and then written by unprivileged?" Or if not, exactly how is this a problem? You check the capabilities when you do the write and if that is not allowed then, well

Sorry for I didn't provide clear explanation.

The privileged means CAP_BLOCK_SUSPEND but not file permission. The file permission has already relaxed for non-root user. Then the expected behavior is that non-root process must has CAP_BLOCK_SUSPEND capability for writing wake_lock sysfs.

But, the CAP_BLOCK_SUSPEND restrict can be bypassed:

int main(int argc, char* argv[])

{
        int fd, ret = 0;

        fd = open("/sys/power/wake_lock", O_RDWR);
        if (fd < 0)
                err(1, "open wake_lock");

        if (dup2(fd, 1) != 1)	// overwrite the stdout with wake_lock
                err(1, "dup2");
        sleep(1);
        execl("./string", "string");	//string has capability

        return ret;
}

This program is an unpriviledged process (has no CAP_BLOCK_SUSPEND), it opened wake_lock sysfs and overwrited stdout. Then it executes the "string" program that has CAP_BLOCK_SUSPEND.

The string program writes to stdout, which means that it writes to wake_lock. So an unpriviledged opener can trick an priviledged writer for writing sysfs.

Quoted 7 lines

And you are checking the namespace of the person trying to do the write when the write happens, which is correct here, right?

If you really want to mess with wake locks in a namespaced environment, then put it in a real namespaced environment, which is {HUGE HINT} not sysfs.

I don't want to mess with wake locks in namespace.

Quoted 39 lines · joeyli

Hi Greg,

Quoted 11 lines · Greg Kroah-Hartman

Quoted 5 lines · Chun-Yi

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Don't you mean "open by privileged and then written by unprivileged?" Or if not, exactly how is this a problem? You check the capabilities when you do the write and if that is not allowed then, well

Sorry for I didn't provide clear explanation.

The privileged means CAP_BLOCK_SUSPEND but not file permission. The file permission has already relaxed for non-root user. Then the expected behavior is that non-root process must has CAP_BLOCK_SUSPEND capability for writing wake_lock sysfs.

But, the CAP_BLOCK_SUSPEND restrict can be bypassed:

int main(int argc, char* argv[])

{
        int fd, ret = 0;

        fd = open("/sys/power/wake_lock", O_RDWR);
        if (fd < 0)
                err(1, "open wake_lock");

        if (dup2(fd, 1) != 1) // overwrite the stdout with wake_lock
                err(1, "dup2");
        sleep(1);
        execl("./string", "string");  //string has capability

        return ret;
}

Quoted 3 lines

This program is an unpriviledged process (has no CAP_BLOCK_SUSPEND), it opened wake_lock sysfs and overwrited stdout. Then it executes the "string" program that has CAP_BLOCK_SUSPEND.

That's the problem right there, do not give CAP_BLOCK_SUSPEND rights to "string". If any user can run that program, there's nothing the kernel can do about this, right? Just don't allow that program on the system :)

Quoted 3 lines

The string program writes to stdout, which means that it writes to wake_lock. So an unpriviledged opener can trick an priviledged writer for writing sysfs.

That sounds like a userspace program that was somehow given incorrect rights by the admin, and a user is taking advantage of it. That's not the kernel's fault.

Quoted 49 lines · Greg Kroah-Hartman

Quoted 39 lines · joeyli

Hi Greg,

Quoted 11 lines · Greg Kroah-Hartman

Quoted 5 lines · Chun-Yi

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Don't you mean "open by privileged and then written by unprivileged?" Or if not, exactly how is this a problem? You check the capabilities when you do the write and if that is not allowed then, well

Sorry for I didn't provide clear explanation.

The privileged means CAP_BLOCK_SUSPEND but not file permission. The file permission has already relaxed for non-root user. Then the expected behavior is that non-root process must has CAP_BLOCK_SUSPEND capability for writing wake_lock sysfs.

But, the CAP_BLOCK_SUSPEND restrict can be bypassed:

int main(int argc, char* argv[])

{
        int fd, ret = 0;

        fd = open("/sys/power/wake_lock", O_RDWR);
        if (fd < 0)
                err(1, "open wake_lock");

        if (dup2(fd, 1) != 1) // overwrite the stdout with wake_lock
                err(1, "dup2");
        sleep(1);
        execl("./string", "string");  //string has capability

        return ret;
}

Quoted 3 lines

This program is an unpriviledged process (has no CAP_BLOCK_SUSPEND), it opened wake_lock sysfs and overwrited stdout. Then it executes the "string" program that has CAP_BLOCK_SUSPEND.

That's the problem right there, do not give CAP_BLOCK_SUSPEND rights to "string". If any user can run that program, there's nothing the kernel can do about this, right? Just don't allow that program on the system :)

Quoted 7 lines

Quoted 3 lines

The string program writes to stdout, which means that it writes to wake_lock. So an unpriviledged opener can trick an priviledged writer for writing sysfs.

That sounds like a userspace program that was somehow given incorrect rights by the admin, and a user is taking advantage of it. That's not the kernel's fault.

Isn't it? Pretty much any setuid program will write to stdout or stderr; even the glibc linker code does so if you set LD_DEBUG. (Normally the output isn't entirely attacker-controlled, but it is in the case of stuff like "procmail", which I think Debian still ships as setuid root.) setuid programs should always be able to safely call read() and write() on caller-provided file descriptors. Also, you're supposed to be able to receive file descriptors over unix domain sockets and then write to them without trusting the sender. Basically, the ->read and ->write VFS handlers should never look at the caller's credentials, only the opener's (with the exception of LSMs, which tend to do weird things to the system's security model).

Quoted 61 lines · Jann Horn

Quoted 49 lines · Greg Kroah-Hartman

Quoted 41 lines · joeyli

Hi Greg,

Quoted 8 lines · Greg Kroah-Hartman

Quoted 6 lines · Chun-Yi

The wake lock/unlock sysfs interfaces check that the writer must has CAP_BLOCK_SUSPEND capability. But the checking logic can be bypassed by opening sysfs file within an unprivileged process and then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski in CVE-2013-1959.

Quoted 4 lines

Don't you mean "open by privileged and then written by unprivileged?" Or if not, exactly how is this a problem? You check the capabilities when you do the write and if that is not allowed then, well

Sorry for I didn't provide clear explanation.

The privileged means CAP_BLOCK_SUSPEND but not file permission. The file permission has already relaxed for non-root user. Then the expected behavior is that non-root process must has CAP_BLOCK_SUSPEND capability for writing wake_lock sysfs.

But, the CAP_BLOCK_SUSPEND restrict can be bypassed:

int main(int argc, char* argv[])

{
       int fd, ret = 0;

       fd = open("/sys/power/wake_lock", O_RDWR);
       if (fd < 0)
               err(1, "open wake_lock");

       if (dup2(fd, 1) != 1) // overwrite the stdout with wake_lock
               err(1, "dup2");
       sleep(1);
       execl("./string", "string");  //string has capability

       return ret;
}

Quoted 3 lines

This program is an unpriviledged process (has no CAP_BLOCK_SUSPEND), it opened wake_lock sysfs and overwrited stdout. Then it executes the "string" program that has CAP_BLOCK_SUSPEND.

That's the problem right there, do not give CAP_BLOCK_SUSPEND rights to "string". If any user can run that program, there's nothing the kernel can do about this, right? Just don't allow that program on the system :)

Quoted 7 lines

Quoted 3 lines

The string program writes to stdout, which means that it writes to wake_lock. So an unpriviledged opener can trick an priviledged writer for writing sysfs.

That sounds like a userspace program that was somehow given incorrect rights by the admin, and a user is taking advantage of it. That's not the kernel's fault.

Quoted 11 lines

Isn't it? Pretty much any setuid program will write to stdout or stderr; even the glibc linker code does so if you set LD_DEBUG. (Normally the output isn't entirely attacker-controlled, but it is in the case of stuff like "procmail", which I think Debian still ships as setuid root.) setuid programs should always be able to safely call read() and write() on caller-provided file descriptors. Also, you're supposed to be able to receive file descriptors over unix domain sockets and then write to them without trusting the sender. Basically, the ->read and ->write VFS handlers should never look at the caller's credentials, only the opener's (with the exception of LSMs, which tend to do weird things to the system's security model).

So a root program gets the file handle to the sysfs file and then passes it off to a setuid program and the kernel should somehow protect from this?

I think that any sysfs file that is relying on the capable() check should just set their permissions properly first, and then it should be ok.

thanks,

greg k-h

There have some discussion in the following mail loop about checking capability in sysfs write handler:

    https://lkml.org/lkml/2018/9/13/978

Sometimes we check the capability in sysfs implementation by using capable function.

But the checking can be bypassed by opening sysfs
file within an unprivileged process then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski for CVE-2013-1959.

Because the sysfs_ops does not forward the file descriptor to the show/store callback, there doesn't have chance to check the capability of file's opener.

This patch adds the hook to sysfs_ops that allows different implementation in object and attribute levels for checking file capable before accessing sysfs interfaces.

The callback function of kobject sysfs_ops is the first implementation of new hook. It casts attribute to kobj_attribute then calls the file capability callback function of attribute level. The same logic can be implemented in other sysfs file types, like: device, driver and bus type.

The capability checking logic in wake_lock/wake_unlock sysfs interface is the first example for kobject. It will check the opener's capability.

Quoted 3 lines · Chun-Yi

There have some discussion in the following mail loop about checking capability in sysfs write handler:

    https://lkml.org/lkml/2018/9/13/978

A sysfs callback should not care about stuff like this.

Worst case, do a simple:

	if (!capable(CAP_FOO))
		return -EPERM

you don't care or need to worry about the file handle for that at all, right?

Quoted 2 lines

Sometimes we check the capability in sysfs implementation by using capable function.

Which should be fine, right?

Quoted 4 lines

But the checking can be bypassed by opening sysfs
file within an unprivileged process then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski for CVE-2013-1959.

And who does this for a sysfs file? And why?

Quoted 3 lines

Because the sysfs_ops does not forward the file descriptor to the show/store callback, there doesn't have chance to check the capability of file's opener.

Which is by design. If you care about open, you are using sysfs wrong.

Quoted 3 lines

This patch adds the hook to sysfs_ops that allows different implementation in object and attribute levels for checking file capable before accessing sysfs interfaces.

No, please no.

Quoted 6 lines

The callback function of kobject sysfs_ops is the first implementation of new hook. It casts attribute to kobj_attribute then calls the file capability callback function of attribute level. The same logic can be implemented in other sysfs file types, like: device, driver and bus type.

Quoted 2 lines

The capability checking logic in wake_lock/wake_unlock sysfs interface is the first example for kobject. It will check the opener's capability.

Why doesn't the file permission of that sysfs file determine who can or can not write to that file?

Hi Greg,

Thanks for your review!

Quoted 11 lines · Greg Kroah-Hartman

Quoted 3 lines · Chun-Yi

There have some discussion in the following mail loop about checking capability in sysfs write handler:

    https://lkml.org/lkml/2018/9/13/978

A sysfs callback should not care about stuff like this.

Worst case, do a simple:

	if (!capable(CAP_FOO))
		return -EPERM

Quoted 3 lines

you don't care or need to worry about the file handle for that at all, right?

The capable() can be bypassed.

Unprivileged process may reads or writes those sysfs if file permission be relaxed by root for non-root user.

Quoted 5 lines

Quoted 2 lines

Sometimes we check the capability in sysfs implementation by using capable function.

Which should be fine, right?

If file permission is enough to restrict sysfs that can only be used by root. Why do some sysfs interfaces use capable()? It's not redundancy?

Quoted 6 lines

Quoted 4 lines

But the checking can be bypassed by opening sysfs
file within an unprivileged process then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski for CVE-2013-1959.

And who does this for a sysfs file? And why?

Just want to bypass the capable() checking.

Quoted 6 lines

Quoted 3 lines

Because the sysfs_ops does not forward the file descriptor to the show/store callback, there doesn't have chance to check the capability of file's opener.

Which is by design. If you care about open, you are using sysfs wrong.

OK~ So the sysfs doesn't care opener's capability.

Quoted 15 lines

Quoted 3 lines

This patch adds the hook to sysfs_ops that allows different implementation in object and attribute levels for checking file capable before accessing sysfs interfaces.

No, please no.

Quoted 8 lines

The callback function of kobject sysfs_ops is the first implementation of new hook. It casts attribute to kobj_attribute then calls the file capability callback function of attribute level. The same logic can be implemented in other sysfs file types, like: device, driver and bus type.

The capability checking logic in wake_lock/wake_unlock sysfs interface is the first example for kobject. It will check the opener's capability.

Quoted 3 lines

Why doesn't the file permission of that sysfs file determine who can or can not write to that file?

I agree that the file permission can restrict the writer of sysfs. But, I still confused for why do some sysfs interface use capable()?