CVE-2021-35939: First steps towards fixing the symlink CVEs by pmatilai · Pull Request #1919 · rpm-software-management/rpm

Internal only for now in case we need to fiddle with the API some more, but no reason this couldn’t be made public later.

Whenever directory changes during unpacking, walk the entire tree from starting from / and validate any symlinks crossed, fail the install on invalid links.

This is the first of step of many towards securing our file operations against local tamperers and besides plugging that one CVE, paves the way for the next step by adding the necessary directory fd tracking. This also bumps the rpm OS requirements to a whole new level by requiring the *at() family of calls from POSIX-1.2008.

This necessarily does a whole lot of huffing and puffing we previously did not do. It should be possible to cache secure (ie root-owned) directory structures to avoid validating everything a million times but for now, just keeping things simple.

Any unowned directories will be created inline during processing now so we can just flush this big pile of code that was insecure anyhow.

As an additional bonus creating the directories inline gives us an opportunity to track the creation so we can undo too, but that is not done here.

Handling this in a separate clause makes the logic much clearer and (in theory at least) lets us handle hardlinks to any content, not just regular files.

Fix the initial setmeta value to something meaningful: we will never set metadata on skipped files, and hardlinks are handled with a special logic during install. They’d need different kind of special logic on FA_TOUCH so just play it safe and always apply metadata on those.

Harlink metadata setting on install should happen on the *last* entry of hardlinked set that gets installed (wrt various skip scenarios) as otherwise creating those additional links affects the timestamp. Note in particular the “last file of…” case in fsmMkfile() where we the comment said just that, but set the metadata on the *first* file which would then be NULL’ed away.

This all gets current masked by the fact that we do the metadata setting on a separate round, but that is about to change plus this makes the overall logic clearer anyhow.

Commit a82251b moved metadata setting to a separate step because there are potential benefits to doing so, but the current downsides are worse: as long as we operate in potentially untrusted directories, we’d need to somehow verify the content is what we initially laid down to avoid possible privilege escalation from non-root owned directories.

This commit does not fix that vulnerability, only makes the window much smaller and paves the way for the real fix(es) without introducing a second round of directory tree validation chase to the picture.

Supposedly no functional changes here, we just need all these things converted before we can swap over to relative paths.

This isn’t ideal from the sense that some files may get a success post call while something later can still fail, but things get even weirder with doing it in a separate round where things could fail because of a vanished directory and then we’d still need to call the plugin hook with some result. Also, this lets us skip the backwards walk on the normal case of success, which is nice.

It doesn’t make much sense to call plugins for files that wont be unpacked at all, and in particular it wont make much sense to do the entire directory dance just to be able to pass meaningful path values to plugins. So from now we’ll only be calling file-pre for things that we’re about to lay down, which it how it used to be before splitting the stages anyhow.

All our renames are (for now at least) within a single directory so the second dirfd is kinda redundant, but shrug…

fsmUnpack() is the only place in FSM that needs to deal with rpmio FD types, everywhere else they are nothing but a hindrance that need to be converted to OS level descriptors for use. Better deal with OS level descriptors to begin with.

This will be needed for using fd-based metadata operations.

Notably cap_set_file() doesn’t have a dirfd-based mode, to handle that safely we’ll need to use fd-based operation. Which would be nicer anyhow but symlinks can’t be opened so we’ll have to carry the dirfd/path based mode forever more anyhow (yes Linux has extensions but that’s another story).

We need to support both fd-based and (dirfd+) path based operations due to all the lovely mismatches in POSIX, so lotsa half-duplicated tedious stuff here.

As of this commit, we only use fd based ops for regular files.

Regular file ops are fd-based already, for the rest we need to open them manually. Files with temporary suffix must never be followed, for directories (and pre-existing FA_TOUCHed files) use the rpm symlink “root or target owner allowed” rule wrt following.

This mostly fixes CVE-2021-35938, but as we’re not yet using dirfd-based operatiosn for everything there are corner cases left undone. And then there’s the plugin API which needs updating for all this.

A thinko originating from commit c9b2686 which doesn’t matter greatly as long as we’re still using absolute paths but will fail as soon as dirfd+basename is used.

Also pay more attention to the rc’s: we must not backup, or run file pre plugin hook if we know it’ll fail.

This being one of the more central functions in fsm now, there better be some diagnostics from it too. Especially when we move to dirfd+basename operation.

Sadly the thing that allegedly makes things better mostly just makes things more complicated as symlinks can’t be opened, so we’ll now have to deal with both cases in plugins too. To make matters worse, most APIs out there support either an fd or a path, but very few support the *at() style dirfd + basename approach so plugins are stuck with absolute paths for now.

This is of course a plugin API/ABI change too.

Cross-directory hardlinks shouldn’t be used as there’s no guarantee two directories are on the same filesystem, but these exist in the wild so we need to care.

This is a special case in various places around rpm, worth having a test for.

Within fsm this is just a matter of adjusting error messages to include the directory… if it only wasn’t for the plugins requiring absolute paths for outside users. For the plugins, we need to assemble absolute paths as needed, both in ensureDir() and plugin file slots.