Regression: ppp_check_access() may break existing configurations

[LGA1150]

openwrt/openwrt#23540 (comment)

60392cf
The check requires all paths from the root dir down to not have group write or others write permissions, which is too strict and may break existing configurations.

There are better ways to solve the TOCTOU bug. One may be using openat2() + fexecve() on fd.

[jkroonza]

@LGA1150

The moment there is write anywhere on the path it becomes potentially possible for normal users to modify the target file. At least, that was our reasoning. openat2 isn't exposed via libc. It's also Linux specific, not POSIX, so can't rely on that.

others write I agree with, but if group is root then group-write IMHO is acceptable.

We should probably also check facls in the check process too:

jkroon@plastiekpoot /tmp $ mkdir foo
jkroon@plastiekpoot /tmp $ setfacl -m u:jkroon2:rwx foo
jkroon@plastiekpoot /tmp $ ls -la foo
total 2496
drwxrwxr-x+ 2 jkroon jkroon    4096 May 27 11:23 .
drwxrwxrwt  9 root   root   2547712 May 27 11:23 ..
jkroon@plastiekpoot /tmp $ getfacl foo
# file: foo
# owner: jkroon
# group: jkroon
user::rwx
user:jkroon2:rwx
group::r-x
mask::rwx
other::r-x

Or maybe that's why we did the w filter on group?

@paulusmack I wasn't aware of fexecve - that certainly looks to be an interesting option. Combined with fstat() to get permissions on the file itself. So open(), then fstat() on the file, check permissions (and ACLs) on the file itself. Then fexecve() or read()/mmap() as the case may be after that? Then I suspect we don't even need to check the rest of the path. There is only so much we can do to protect here, and for files containing secrets we can also ban read for anybody other than root ... (just to force the admin to be a little bit more thorough).

[Neustradamus]

@LGA1150: Thanks for your report and the quick @jkroonza answer!

[paulusmack]

openwrt/openwrt#23540 (comment)

Interesting, thanks. I see that /lib/netifd has mode 0755 on the OpenWRT instance I have here (OpenWRT 23.05.5), do we know why it got changed to 0775?

60392cf The check requires all paths from the root dir down to not have group write or others write permissions, which is too strict and may break existing configurations.

I would agree with allowing the file to have group write permission if the group is root, and it looks like that would solve the openwrt issue.

There are better ways to solve the TOCTOU bug. One may be using openat2() + fexecve() on fd.

It's not a TOCTOU problem that we're trying to solve; what we're looking for is assurance that the file we're using is the file that the administrator intended us to use. I'm not sure that checking just the file itself gives us that. What we're trying to check is that the path to the file doesn't contain a weak point where an attacker could redirect the path to point to a different file. Even if that alternate file was owned by root and not writable by others, if an attacker can choose any root-owned file on the system then I would not want to bet on the attacker not being able to get some sort of unauthorized access that way.

[paulusmack]

Another change that would solve the problem is to apply the checks only if pppd is run by a non-root user (which implies that the executable has been installed setuid-root). But it seems better to have the checks even if pppd is run by root as long as they are not too invasive. Or maybe if pppd is being run by root then a check failure could be a warning rather than a fatal error.

[jkroonza]

@paulusmack the PR take an opposite, and IMHO better approach. If I was aware previously of fexecve I might well have suggested that from the get go.

The approach is to open the file first, then use fstat() on the file descriptor to check that it is root owned, and cannot be modified by non-root (even root group, which incidentally facls seem strange as it seems there's influence on the permission bits from stat, so either we need to ban group-write or perform full and proper facl checks), and only then proceed to use it.

I like the approach, it avoids a bunch of complexity our approach has.

[paulusmack]

@paulusmack the PR take an opposite, and IMHO better approach. If I was aware previously of fexecve I might well have suggested that from the get go.

I went back and looked at your original PR from last year to see what the original rationale was, but it doesn't look like you mentioned anything specific, so I don't know what kind of attack or potential vulnerability you were thinking of. I had previously had an email conversation with some researchers who were concerned that if an attacker could modify some files used by pppd, they could use that to get elevated privileges (I think that also required a setuid-root pppd). Now, this isn't a practical attack as long as the files are not writable by non-root and the attacker can't substitute another file of their choice for the correct file, because if you can write to files owned by root, there are much quicker ways to get unauthorized access.

So this is why I like the idea of checking not just the file itself but also the path to it, in order to cut off the possibility of substitution. On reflection, it should probably be the supplied path that we check, not the real path, because if the supplied path had a weak point (one that was writable by non-root) then we wouldn't detect substitution if the real path of the substitute was entirely controlled by root (but the substitute would still be the wrong file to use).

We also should really be checking the paths for plugins.

The approach is to open the file first, then use fstat() on the file descriptor to check that it is root owned, and cannot be modified by non-root (even root group, which incidentally facls seem strange as it seems there's influence on the permission bits from stat, so either we need to ban group-write or perform full and proper facl checks), and only then proceed to use it.

I like the approach, it avoids a bunch of complexity our approach has.

But it doesn't protect against substitution.

[LGA1150]

Now, this isn't a practical attack as long as the files are not writable by non-root and the attacker can't substitute another file of their choice for the correct file, because if you can write to files owned by root, there are much quicker ways to get unauthorized access.

Anyway the TOCTOU has been present until v2.5.2, so we'd better request a CVE ID.

[paulusmack]

Now, this isn't a practical attack as long as the files are not writable by non-root and the attacker can't substitute another file of their choice for the correct file, because if you can write to files owned by root, there are much quicker ways to get unauthorized access.

Anyway the TOCTOU has been present until v2.5.2, so we'd better request a CVE ID.

What TOCTOU precisely?

[LGA1150]

What TOCTOU precisely?

In run_program(), the race condition between stat() and execve().

[jkroonza]

The approach is to open the file first, then use fstat() on the file descriptor to check that it is root owned, and cannot be modified by non-root (even root group, which incidentally facls seem strange as it seems there's influence on the permission bits from stat, so either we need to ban group-write or perform full and proper facl checks), and only then proceed to use it.
I like the approach, it avoids a bunch of complexity our approach has.

But it doesn't protect against substitution.

It does, because we use the checked file descriptor.

[paulusmack]

What TOCTOU precisely?

In run_program(), the race condition between stat() and execve().

I assume we're talking about ppp <= 2.5.2 here.

For run_program(), the program is in a system directory such as /etc/ppp or in a location supplied by a privileged option, so the location is presumed to have been secured properly by the system administrator. There are any number of programs which run as root and trust the contents of files in system directories, and that is not considered a vulnerability. The stat() serves as an early exit in the case of obvious problems, not as a security check. So I don't consider this a TOCTOU bug.

Put another way, there is nothing that we check with stat() that isn't then checked again by execve() itself. So the point of the early check is just to be able to show an error when we have the full error logging infrastructure available, rather than later when we do the execve() in the child process. If we get an error there, we can really only send it to syslog, which isn't as good.

[paulusmack]

But it doesn't protect against substitution.

It does, because we use the checked file descriptor.

We appear not to be communicating. I don't mean substitution during the short time window between when pppd stats the file and when it reads it, I mean substitution any time between when the system administrator created the file and when pppd reads it. No check that merely looks at the file itself can protect against that.

In any event it seems to me that any normal installation of pppd is fine; the sysadmin would have to have deliberately done something stupid for directories like /etc/ppp or /usr/lib/pppd to be vulnerable. How much effort is it worth going to, to try to protect the system against a stupid sysadmin? I suppose what we are trying to do is make it so that pppd is less likely to get the blame than other system programs.

So, @jkroonza, what was your original motivation for adding the check in PR #561?

[jkroonza]

Exactly the situation under discussion, came from another thread/or discussion if I recall. I can't seem to find that discussion now, might well have came from the default route discussion which lead into what should be privileged/not and which lead to how do we know the scripts and other config files are secure (also the network manager discussion).

Bottom line, it is a TOCTAU or at least -like issue.

If the file is owned by root, and writable by root only, even if that file resides in /tmp - it means root created the file, and no-one else could have overwritten it, so I'm happy with the TOCTAU protection as proposed. It solves the original problem I believe but you are right in that if we don't check the path it could still mean that someone did perform some kind of swap-one-root-script-for-another-which-could-lead-to-lpe problems.