[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-id: <E1A90A90-6E02-45B9-9CD8-986306431E80@mac.com>
Date: Mon, 25 Jul 2011 20:00:57 -0400
From: Jeff Johnson <n3npq@....com>
To: Solar Designer <solar@...nwall.com>
Cc: oss-security@...ts.openwall.com
Subject: Re: CVE Request -- rpm -- Fails to remove the SUID/SGID
bits on package upgrade (RH BZ#598775)
On Jul 25, 2011, at 7:02 PM, Solar Designer wrote:
> Jeff,
>
> Thank you for your comments!
>
> On Mon, Jul 25, 2011 at 03:39:15PM -0400, Jeff Johnson wrote:
>> There were a series of CVE's applied (and some withdrawn) against
>> whatever happens to be called "rpm".
>>
>> The patch here was dropped when RPM was forked and the CVE was
>> essentially a replay of an issue that was already fixed 5 years ago
>> (and the patch was NOT dropped in @rpm5.org cvs).
>
> I am not sure I understand what you mean here. As I wrote, I am aware
> of two CVEs relevant to the general issue: CVE-2005-4889 (package
> removals) and CVE-2010-2059 (package upgrades). The corresponding
> issues were in fact fixed in rpm4 at different times. Neither fix was
> reverted in rpm4. Neither CVE id was withdrawn.
>
There were some other related issues having to do with ACL's and CAPABILITY
behavior with hard links that were opened and closed at the time.
The whole issue of whether a "package manager" is responsible for side effects
like hard links leaving privileged executables is rather complex and arguable:
Is a package manger responsible for operations that it did not perform?
This was the reason to close the other CVE's with ACL's (because rpm does nothing with ACL's)
and there was an addition from Panu at the time to handle de-privileging files
with CAPABILITIES attached. There's perhaps other threat vectors with SE-Linux
xattr's and such as well.
Caveat:
This is all from memory when the CVE's were entered. And I was somewhat
annoyed (and less than helpful) when I was accused of not doing
what I said I did by the person who actually dropped a patch while
forking rpm.
> Are you saying that the fix for CVE-2005-4889 was somehow dropped from
> rpm5, another CVE id was assigned, and the fix was re-introduced?
> I have no idea - I am just trying to guess what you might have meant.
>
No. Here's roughly what happened:
Back in 2005 there was a patch from SuSE that solved the issue.
I checked that patch in, and closed a bug as fixed.
RPM was forked at version rpm-4.4.2, the check-in ended up in rpm-4.4.3.
In the process of rebasing from cvs to git (or was it mercurial) a patch was missed.
Five years go by … the person who did the original patch was annoyed
that the patch was not in his version of rpm and decided that I was
a liar (which is where my annoyance comes from). The truth is actually
much more banal: a patch got lost, and so a 5 year old already fixed problem
ended up with a 2nd CVE and a few others at the time related to related
privilege operations on hard linked files.
>> (aside)
>> I believe there are better fixes if the link count is more carefully
>> checked always and everywhere. While rpm package metadata does not
>> (and SHOULD not) carry an expected value for st->st_nlinks, its
>> rather easy to synthesize an expected link count given the inode
>> information (which is in rpm metadata) and to warn (either with --verify,
>> or perhaps always) if the link count is not as expected.
>
> Unfortunately, only doing the chmod() to safe perms if the link count is
> other than expected is prone to a race condition. rpm4 actually had
> this race condition introduced, then removed:
>
> commit 89be57ad9239c9ada0cba94a5003876b456d46bf
> Author: Panu Matilainen <pmatilai@...hat.com>
> Date: Fri Jun 11 08:17:12 2010 +0300
>
> If there are no hardlinks, dont bother with s-bit and caps removal
>
> commit 26874707edfe73e153383284f9fe33cfd9879bb1
> Author: Michal Schmidt <mschmidt@...hat.com>
> Date: Tue Jun 22 15:51:41 2010 +0200
>
> Revert "If there are no hardlinks, dont bother with s-bit and caps removal"
>
> Deciding whether it is necessary to remove the SUID bit based on
> the current link count creates an opportunity for a race condition.
> A hardlink could be created just between lstat() and chmod().
>
> This reverts commit 89be57ad9239c9ada0cba94a5003876b456d46bf.
>
I trust your careful work more than most. I have private opinions on the
expertise of both of those committers that color my judgement, so I shall
recuse myself from direct discussion on the efforts.
I believe you is all that I'm really saying.
Since we're on the topic of racy:
Should I choose to add openat(2) and all the other new syscalls in RPM?
I can easily do the coding: I've chosen not to largely because of "portability"
and that in many cases I believe that the raciness is solved through other means,
and that there's so many easier ways to gain root than to attempt to
exploit an RPM install.
But these are just my excuses for being lazy: if you think there is value
in using openat(2) et al, then I will undertake that development in spite
of the modest loss of portability when openat(2) et al are not available.
>> There are other (and better) approaches if the actual values on
>> the file system, including files not contained in packages, is
>> stored in an rpmdb: its a fundamental design flaw in RPM that
>> only package metadata installed in an rpmdb is ever used
>> for security auditing.
>
> To me, system integrity checking for security purposes is mostly not a
> package manager task, although sometimes it is in fact useful that rpm
> can do it, even if to a very limited extent.
>
The fundamental problem is that none has precisely defined what
a package manager is SUPPOSED to do. Specifically
What threat model should a package manager address?
I cannot answer that question myself: I've added digital signatures (which
are routinely disabled) and SELinux file context instantiation (SELinux
wishes RPM to become "untrusted" as they rightly should: the privilege
granted to RPM by SELinux policy is extraordinary), and I am currently
(and rather slowly because the implementations are very difficult) have
been looking at making RPM "trusted" in the hardware enforced TCG/TPM/TCB
sense) and I simply do not know what thread model SHOULD apply to "package management".
I do know that there are efforts underway, bothe from Tresys for SELinux, and
from MSSF in MeeGo, to distribute security sensitive information through
*.rpm conveyance. These efforts would be simplified imho if RPM can
somehow become "trusted" in the TCB sense, whatever that might mean.
Please note that my comments are pragmatic observations nobly. My private
belief is that there are so many other and easier ways to break into a box
than to fuss (and fuzz) a binary format like *.rpm these days that the
risks from RPM exploits are increasingly unimportant. But these are excuses
to be lazy, nothing more: I can and will undertake whatever implementations
are deemed important (like using openat(2) or adding capabilities or whatever
else is deemed necessary).
> As to having rpm check/remove some files that are closely related to a
> package being verified/removed but that didn't come from the package,
> isn't this what %ghost is for? It won't verify those files' contents,
> but I think that maintaining a database of hashes of changing files on a
> system is not a package manager's task anyway.
>
If you go back to the original bug report in Debian, the circumstances
were basically this:
A machine was infected with a root kit.
The sysadmin removed a package and thought the problem was solved.
The root kit had hard linked files and so the problem persisted.
Removing privilege when erasing is more to meet "expectations" imho. Nothing
wrong per se with that at all.
That patch (from Debian) was picked up by SuSE for RPM and led to the original CVE.
Meanwhile %ghost was added to deal with certain programs like uucp/hylafax/amanda
that are particularly sensitive to file ownership.
There's actually a design flaw with %ghost btw since there is no file type
associated with the path: any object on the path is "gud enuf" for %ghost.
The flaw there is that SELinux file contexts have a type qualifier needed
to qualify the *RE's that apply to a path and %ghost is useless there.
I've considered adding a %spook directive (essentially %ghost with a file type)
in order to be able to retrieve a SELinux file context for %spook paths.
(and yes I have a quirky sense of humor ;-)
>> But there's no harm at all in removing SUID/SGID bits from files that are being
>> removed in case there's an additional link that has been added.
>
> Yes, and it has to be done regardless of link count (as long as we don't
> have an atomic "chmod if st_nlink is ..." operation).
>
The term "has to be done" depends on a context. I am interested in hearing
your reasoning, so I will supply the contrarian opinion:
A package manger is responsible for its own operations only.
A hard link is an external side effect that could have been
intended rather than malicious, and removing SUID/SGID on
an intended non-malicious link is surprising.
Again: I'm expressing the contrarian POV only to hear your definition
of "has to be done", not to be contrarian or obnoxious or anything else.
> The purpose of my posting was to suggest that a similar cleanup is also
> needed for things that are not SUID/SGID binaries, but also at least for
> device files and for regular files with world or group write permissions.
> Since it is not obvious if that list is exhaustive or not and since new
> file types may appear later, I felt that chmod'ing all files to be
> removed to 0 is a safer thing to do.
>
If you can hint at what you would like to see, I can likely take care of
the rest. The package/file state machines (and their recursion) isn't
the soundest code I've ever written: the recursion was done so that
only a single symbol was added to a library API (which it would be easier
to attach a RPC mechanism to do ...), not for any other engineering reason.
> Of course, even that might not reset attributes stored outside of the
> Unix permissions mask, such as fscaps. So those need to be taken care
> of separately, which fscaps-aware builds of rpm4 already do. Perhaps
> rpm5 does as well - I haven't looked yet.
>
RPM5 doesn't handle capabilities (but its rather easy to add).
The problem I have is the cost of attaching Yet Another item of metadata
to every file path in an additional tag.
AFAIK, there are few MUSTHAVE usages being attempted with capabilities.
Lets say there are ~10K files that might reasonably need/use capabilities.
Since the number of file paths managed through packages is of order >1M,
there is a cost-benefit tradeoff here, between the additional storage
needed everywhere compared to the benefit of being able to manage capabilities
with a package manager.
The proper engineering answer (imho) is NOT to add yet another per-file attribute
(like symlink endpoints) that is only occasionally used, but rather introduce
an attribute attachment mechanism for the much smaller 10K population that needs/uses
capabilities, and use the path to associate the capabilities with files
instead of what RPM traditionally has done (and @rpm.org has chosen to do
with capabilities).
> The patch that I posted was against rpm 4.2, which was not fscaps-aware.
> This is why I did not bother with that aspect of the issue there.
>
Yes capabilities did not exist (iirc) when rpm-4.2 was released.
hth
73 de Jeff
> Thanks again,
>
> Alexander
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
