Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First steps towards fixing the symlink CVEs #1919

Merged
merged 32 commits into from Feb 16, 2022

Conversation

pmatilai
Copy link
Contributor

Details in commits, but basically fixes CVE-2021-35939 and lays down some necessary infrastructure for next steps in securing down our file operations.

@pmatilai
Copy link
Contributor Author

pmatilai commented Feb 10, 2022

It should be noted (probably in the commit message too) that as these symlink CVE's overlap and interact in various ways, this does not fully fix CVE-2021-35939 as the directory tracking does not cover all our installation steps yet. Plugging all the holes requires converting all of FSM to the *at() family of calls plus fd-based ops where possible, so this really is just the first step of many to come.

Copy link
Contributor

@DemiMarie DemiMarie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux, using openat2() will be much simpler and more efficient on kernels that support it. RPM is not Linux-specific, but openat2() might be useful where available.

INSTALL Show resolved Hide resolved
lib/fsm.c Show resolved Hide resolved
@pmatilai
Copy link
Contributor Author

I'm quite aware of Linux having all manner of fancy extensions available.
Rpm is portable software and we need to fix this stuff using what's available in POSIX, utilizing non-portable extensions would only make things far more complicated rather than help. There's enough complexities to deal with as it is, thank you very much.

@pmatilai pmatilai force-pushed the dirsafe-pr branch 2 times, most recently from da3507b to 008cbca Compare February 15, 2022 07:30
@pmatilai
Copy link
Contributor Author

pmatilai commented Feb 15, 2022

This is now using fd or dirfd+basename for file ops within the fsm, as much as possible. Plugins pose special problems as external libraries generally dont support dirfd+basename style operation, but may still need to operate on symlinks so we're stuck with "insecure" absolute paths there, for now at least.

I'm seeing a couple of install glitches on fresh chroot install still, but it's getting close now.
Of course a change this big and drastic will have bugs in it initially, I have no illusions about that.

(edit: hmph, the test-suite was passing just a minute ago...)

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.
@pmatilai
Copy link
Contributor Author

pmatilai commented Feb 15, 2022

Okay, test-suite + all my local tests (install to empty chroot etc) pass now 🥳

@DemiMarie
Copy link
Contributor

This is now using fd or dirfd+basename for file ops within the fsm, as much as possible. Plugins pose special problems as external libraries generally dont support dirfd+basename style operation, but may still need to operate on symlinks so we're stuck with "insecure" absolute paths there, for now at least.

Cute (but non-portable) trick: use paths of the form /dev/fd/$FDNUM/something. Works at least on Linux.

@pmatilai
Copy link
Contributor Author

Yeah once we have the basics working and optimized to a reasonable degree we can start looking at utilizing various OS-specific extensions. The gotcha with those is to find ways to provide extra functionality in the specific OS'es without introducing multiple codepaths (which will inevitably bitrot) to accomplish the same thing.

@pmatilai
Copy link
Contributor Author

Anyway...

There will inevitably be bugs in this all, and since the test-suite covers only so much the best way to find the rest is real-world testing. And sitting in a branch does little to achieve that, so I'm merging this as is now.

Danger Will Robinson, if you're in the habbit of running rpm daily snapshots then you'll want to stay alert for a while.

@pmatilai
Copy link
Contributor Author

Oh, and to make it absolutely clear: we're nowhere near done with this, I just want to get this bulk of change over with so we can concentrate with the finer nuances.

@pmatilai pmatilai merged commit 6dd6272 into rpm-software-management:master Feb 16, 2022
1 check passed
@pmatilai pmatilai deleted the dirsafe-pr branch April 7, 2022 11:14
@sandy-lcq
Copy link

@pmatilai Thanks for fixing these CVEs. And I want to double check with you that
does these 32 commits in this pull request fully fix CVE-2021-35937, CVE-2021-35938, CVE-2021-35939?
Any plan to porting it to 4.17.x branch?

@pmatilai
Copy link
Contributor Author

pmatilai commented Feb 8, 2023

There will be no backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants