The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 20:53 UTC (Thu) by calumapplepie (guest, #143655)
Parent article: The long road to a fix for CVE-2021-20316

Should we add a new flag O_NOFOLLOW_LIKE_EVER to open() and friends which says "I promise no symlinks are included on this path and it is absolute"? It'd probably be a lot simpler to patch the "check" function to also modify the path to be absolute and without symlinks than to rework every function to work with file descriptors instead of file names. The kernel can presumably preform the check without fear of TOCTOU, tossing back a nice error.

Obviously, it's a bit late, and there are probably a long list of very good reasons that people haven't already done this. But I think this is a bug that will keep appearing time and time again, and making it an easy fix for developers will hopefully mitigate it a lot.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 21:30 UTC (Thu) by jra (subscriber, #55261) [Link] (21 responses)

It's not the openXXX() that's the problem. It's everything else too. Everything that takes a path (and there are many such calls in POSIX) is broken w.r.t symlinks and security unless application developers are insanely careful. No one ever is.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 21:37 UTC (Thu) by Cyberax (✭ supporter ✭, #52523) [Link] (8 responses)

The heart of the issue are processes working at different permission levels and sharing the same namespace. This simply can't be made secure.

Future OSes should reject the ACL and permission nonsense and instead move to true container-like isolation.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 22:13 UTC (Thu) by jra (subscriber, #55261) [Link] (3 responses)

Again, I fully agree with you about that. There's a project being lead by Red Hat to help containerize Sba which I'm hoping will bear fruit soon.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 22:14 UTC (Thu) by jra (subscriber, #55261) [Link] (1 responses)

Damn phone keyboard. For Sba read Samba of course.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 23:52 UTC (Thu) by Cyberax (✭ supporter ✭, #52523) [Link]

LOL.

I actually Googled "Sba RedHat" and just was going to ask what it is.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 15:59 UTC (Fri) by phlogistonjohn (subscriber, #81085) [Link]

For the curious, I believe the projects Jeremy is referring to are our efforts here:
https://github.com/samba-in-kubernetes/samba-container/
and the related projects in our org https://github.com/samba-in-kubernetes/

Note that despite the name "kubernetes" in the org, the container images are designed not to be k8s
specific. I'd love to see other uses of the container images for docker/docker-compose, podman, etc. The name was partly chosen because we do have other k8s specific integration plans... and we could abbreviate it as "SINK" ;-)
Thank you for the opportunity for a bit of free advertising.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 10:10 UTC (Fri) by taladar (subscriber, #68407) [Link] (3 responses)

Containers wouldn't really help you with servers like Samba that need to allow for different permissions for different remote users. Nor would they help for permission changes over time. In fact I would go so far as to say containers wouldn't help you with this sort of issue at all.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 13:30 UTC (Fri) by joib (subscriber, #8541) [Link] (2 responses)

Just putting samba, as is, into a docker/podman/whatever container with full permissions won't fix anything, yes.

But maybe something like when a new user connects, fork a new process to handle that user, create appropriately restricted namespaces for that process (call it a "container" if you like), and finally switch the process uid to that user?

The long road to a fix for CVE-2021-20316

Posted Feb 13, 2022 16:53 UTC (Sun) by marcH (subscriber, #57642) [Link]

Sounds like https://www.infoq.com/news/2014/06/everything-google-cont...

The long road to a fix for CVE-2021-20316

Posted Feb 16, 2022 19:24 UTC (Wed) by ssmith32 (subscriber, #72404) [Link]

You'd still need to deal with giving multiple users different levels of access (r/w, at least), to the same file/directory. But it would help with the directory escape bugs.

The long road to a fix for CVE-2021-20316

Posted Feb 10, 2022 23:51 UTC (Thu) by nix (subscriber, #2304) [Link] (10 responses)

I thought of something a few hours ago, but commented on it in the wrong article... so maybe here it'll be more noticeable. (And I got a chance to elaborate a bit more...)

An alternative approach that breaks much less, probably nothing, while retaining the security benefits of root-only symlinks: non-root-owned symlinks (or, more generally, symlinks with uids not specified in a new file under /proc/sys somewhere: root only by default) are only followed by the user that owns them: other users get the effect of O_NOFOLLOW on that symlink without needing to ask for it.

Now hostile users can create all the symlinks they want, as can non-hostile users, and the use case the non-hostile users wanted still works (they can follow symlinks they created, and symlinks the sysadmin or package installer created), while the hostile users find that they are following the hostile symlinks they created, but nobody else is. This obviously should apply at all stages in path resolution, including if a path contains a symlink to a directory. (I don't think we need to worry about races involving symlink ownership suddenly changing, since only root can do that and if root is malicious you've already lost.)

Anyone still broken by this is probably using gid-shared directories for something, like it was the 1980s still. They're probably using ACLs too. A slightly more permissive mode, off by default or on only in directories with the setgid bit active or perhaps which are gid-writable, could apply the same check based on the gid of the symlink instead of the uid, which would fix this use case too. I'm sceptical this use case is common enough to worry about, though.

But symlink following in the specific case of following symlinks the admin made and symlinks the user themselves made is so useful that it simply shouldn't be broken. It's not some program's decision to not let me stitch things together with symlinks I created: it's mine, and programs should always follow them. Symlinks a random hostile user created, though -- why would I ever want to follow those? And thankfully file ownership tracks this perfectly well so we can always get the answer right :)

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 0:18 UTC (Fri) by rgmoore (✭ supporter ✭, #75) [Link] (2 responses)

While this is an interesting idea, I think it will likely be a non-starter because it changes long-established behavior. There will be things that break because of this, and "don't break userspace" is an important enough principle that it will not fly.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 14:27 UTC (Fri) by nix (subscriber, #2304) [Link]

Well, that's why the whole thing is optional (controlled via a flag in /proc/sys/fs). There are *already* flags in /proc/sys/fs that tweak symlink behaviours: this is just another one. (Actually, this is just a variant of what protected_symlinks == 1 already provides. I'm not sure why that's not enough.)

The long road to a fix for CVE-2021-20316

Posted Feb 12, 2022 2:55 UTC (Sat) by bartoc (guest, #124262) [Link]

Generally, I think linux is fine adding in (usually default to off) options that break userspace in the name of security (I could be wrong). I think something like this is worth trying out at least to see how much stuff breaks, given it could remove a whole class of toctou bugs.

My userspace was just broken by lack of `/dev/mem` on my fedora box (I was trying to fetch the EDID of my display)

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 0:53 UTC (Fri) by mathstuf (subscriber, #69389) [Link] (3 responses)

Wouldn't this make analysis harder? That is, would root follow arbitrary symlinks? If so, all root processes are now vulnerable to these issues. If not, manual readlinks are needed to figure out where some path that was logged actually exists. And once those routines exist, they'll show up in all kinds of compat or convenience wrappers, have bugs themselves, and probably just put us back to where we started except now there'd be umpteen symlink resolution impls to check and maintain.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 12:50 UTC (Fri) by ibukanov (subscriber, #3942) [Link]

I suppose with idea root will follow only root-crated symlinks.

The long road to a fix for CVE-2021-20316

Posted Feb 13, 2022 18:23 UTC (Sun) by khim (subscriber, #9252) [Link] (1 responses)

> Wouldn't this make analysis harder? That is, would root follow arbitrary symlinks?

Root would follow only root-created symlinks. Everyone else would also follow root-created symlinks. Everyone else would just follow their own symlinks.

It's actually interesting idea. Would need to see how many packages would break, but this wouldn't effect things like git (people rarely use unix permissions in the middle of git repos) while most distro-provided system symlinks would work.

I think there are some container solutions which use per-app UID (like on Android), would need to decide what to do about these.

The long road to a fix for CVE-2021-20316

Posted Feb 13, 2022 19:02 UTC (Sun) by mjg59 (subscriber, #23239) [Link]

Hm. I wonder how viable it would be to implement this using the BPF LSM?

The long road to a fix for CVE-2021-20316

Posted Feb 16, 2022 8:59 UTC (Wed) by pmatilai (subscriber, #15420) [Link] (2 responses)

Yup, that is exactly the approach rpm started taking internally since 2017: https://github.com/rpm-software-management/rpm/commit/f2d...
I've yet to see it break any legitimate case in the packaging world.

That commit is of course only a tiny fraction (and buggy at that) of the overall problem which we're only just now getting close to resolving it in a wider sense - much like Samba, rpm traditionally used absolute paths for everything.

The long road to a fix for CVE-2021-20316

Posted Feb 18, 2022 1:50 UTC (Fri) by nix (subscriber, #2304) [Link]

Aha! I see there are no new ideas, only ideas someone else already implemented, better :) The 'target directory owner' idea is a nice addition, which could probably be added in to what I thought up above to close even more cases where legitimate use might be broken without enabling any troublesome cases at all.

The long road to a fix for CVE-2021-20316

Posted Feb 18, 2022 17:46 UTC (Fri) by nybble41 (subscriber, #55106) [Link]

> [from the linked commit] The rationale is that if you can create symlinks owned by user X you *are* user X (or root), and if you also own directory Y you can do whatever with it already, including change permissions.

This is a false assumption. The owner of a file or directory can only change permissions or make other changes to it *if they can obtain a reference*. If you own /a/b but don't have search permissions (+x) on /a (and don't have /a/b as your current directory or some similar corner case) then you can't do anything with /a/b. However, under this rule you can still create a symlink to /a/b which would be followed by other users because you own /a/b, even though you can't access it yourself through /a. (The same issue impacts /proc/sys/fs/protected_symlinks=1.)

One option I haven't seen suggested yet would be to use the intersection of the permissions available to the original user and the owner(s) of any symlink(s) followed while resolving the path. Though in practice you might need more information than just the symlink's UID and GID to serve as the credentials, especially when relying on "negative ACLs", pluggable security modules like SELinux, or narrower group permissions overriding, rather than supplementing, broader "other" permissions.

The long road to a fix for CVE-2021-20316

Posted Feb 12, 2022 2:52 UTC (Sat) by bartoc (guest, #124262) [Link]

embracing the AT calls is still a good idea imo, there have been windows apps with toctou bugs that would have been fixed with a proper openat equivalent, and windows requires administrator ACLs to create symlinks (for exactly this reason)

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 9:27 UTC (Fri) by NYKevin (subscriber, #129325) [Link]

For open, this already exists, see RESOLVE_NO_SYMLINKS in openat2(2). The problem is, nobody uses it (which is not helped by the fact that the man page specifically tells you not to use it, or at least to let the user turn it off and shoot themself in the foot). Besides, there are plenty of APIs which do not (and for some use cases, probably cannot) accept a file descriptor as an argument.

The long road to a fix for CVE-2021-20316

Posted Feb 11, 2022 9:43 UTC (Fri) by dullfire (guest, #111432) [Link]

The addition of such a flag, and then telling people to use it as a fix for TOCTOU races with symlinks would effectively be telling them to drop support for symlinks in those programs.

Which would be a massive regression.