Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 540006

Summary: [Auditing] sys-apps/openrc: checkpath: {hard,symbolic} link as possible attack vector to gain privilege escalation
Product: Gentoo Security Reporter: Kristian Fiskerstrand (RETIRED) <k_f>
Component: AuditingAssignee: Gentoo Security Audit Team <security-audit>
Status: RESOLVED FIXED    
Severity: normal CC: ago, blueness, chainsaw, dkg, flow, mpagano, prometheanfire, sandino, williamh
Priority: Normal    
Version: unspecified   
Hardware: All   
OS: Linux   
See Also: https://bugs.gentoo.org/show_bug.cgi?id=603408
https://bugs.gentoo.org/show_bug.cgi?id=603414
https://bugs.gentoo.org/show_bug.cgi?id=603382
https://bugs.gentoo.org/show_bug.cgi?id=603488
https://bugs.gentoo.org/show_bug.cgi?id=602596
https://bugs.gentoo.org/show_bug.cgi?id=603518
https://bugs.gentoo.org/show_bug.cgi?id=603522
https://bugs.gentoo.org/show_bug.cgi?id=603606
https://bugs.gentoo.org/show_bug.cgi?id=603702
https://bugs.gentoo.org/show_bug.cgi?id=603706
https://bugs.gentoo.org/show_bug.cgi?id=603806
Whiteboard:
Package list:
Runtime testing required: ---
Bug Depends on: 540706    
Bug Blocks:    
Attachments: patch from debian kernels (post 3.6)
0001-checkpath-security-fix-for-chown-and-chmod.patch
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

Description Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 19:04:55 UTC
checkpath --owner on a file, in my case residing in /var/lib/sks allows for giving control to other files on the filesystem for that process through use of a hardlink. Example: 

ls -l /etc/passwd
-rw-r--r-- 2 root root 1226 Dec 22 20:05 /etc/passwd

cd /var/lib/sks
ln /etc/passwd /var/lib/sks/passwd.log

# /etc/init.d/sks-db restart
 * Stopping SKS db ...                                                    [ ok ]
 * /var/lib/sks/passwd.log: correcting owner
 * Starting SKS db ...                                                    [ ok ]

# ls -l /etc/passwd
-rw-r--r-- 3 sks sks 1226 Dec 22 20:05 /etc/passwd

now the user of a potentially compromised SKS server could further leverage an attack against the system. 

Potential mitigation: Do not let checkpath touch files if hardlink exists outside of basedir of the file or an otherwise provided basedir (in the event a multi directory system is encountered and one is manually specified in the init script)
Comment 1 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 19:20:16 UTC
Adding dkg (from debian) as CC to be able to participate in discussion
Comment 2 Daniel Kahn Gillmor 2015-02-13 19:35:01 UTC
I think some versions of the linux kernel provide configurable mitigations against non-privileged users being able to create hardlinks to files that they don't own, which would make this attack not work.

see /proc/sys/fs/protected_hardlinks (since Linux 3.6) in proc(5) for more details.

Of course, a hardlink could still be made on systems with an older (or unconfigured) kernel, which would allow the exploit through again.


> Do not let checkpath touch files if hardlink exists outside of basedir of the
> file or an otherwise provided basedir (in the event a multi directory system is 
> encountered and one is manually specified in the init script)

these tests seem quite hard to do properly.  I don't know of any way to detect where the other names are for an inode that has a link count > 1; so i guess you could approximate it by just ensuring link counts of 1 for normal files (though maybe there are race conditions there?).

And i don't know what you'd have to do for the directory checks either, since they've all got link counts > 1 typically..
Comment 3 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 19:38:57 UTC
(In reply to Daniel Kahn Gillmor from comment #2)
> I think some versions of the linux kernel provide configurable mitigations
> against non-privileged users being able to create hardlinks to files that
> they don't own, which would make this attack not work.
> 
> see /proc/sys/fs/protected_hardlinks (since Linux 3.6) in proc(5) for more
> details.
> 

Thanks, the VM I tested this on is running 3.16.5 and it works even while doing su - sks --shell=/bin/bash so even with the user having a default shell as /sbin/nologin I'm assuming a malicious binary (if the sks service were to be compromised) could do the same, however, that will of course have to be verified somehow.
Comment 4 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 19:46:52 UTC
(In reply to Kristian Fiskerstrand from comment #3)
> (In reply to Daniel Kahn Gillmor from comment #2)
> > I think some versions of the linux kernel provide configurable mitigations
> > against non-privileged users being able to create hardlinks to files that
> > they don't own, which would make this attack not work.
> > 
> > see /proc/sys/fs/protected_hardlinks (since Linux 3.6) in proc(5) for more
> > details.
> > 
> 
> Thanks, the VM I tested this on is running 3.16.5 and it works even while
> doing su - sks --shell=/bin/bash so even with the user having a default
> shell as /sbin/nologin I'm assuming a malicious binary (if the sks service
> were to be compromised) could do the same, however, that will of course have
> to be verified somehow.

cat /proc/sys/fs/protected_hardlinks 
0

echo 1 > /proc/sys/fs/protected_hardlinks 

su - sks --shell=/bin/bash
ln /etc/passwd passwd.log
ln: failed to create hard link 'passwd.log' => '/etc/passwd': Operation not permitted

blocks this attack vector, indeed.
Comment 5 Daniel Kahn Gillmor 2015-02-13 20:00:55 UTC
Debian patches the kernel to default to 1 for these configurations:

https://bugs.debian.org/609455

I don't think this fixes things for our non-Linux kernels, though.  I don't know if gentoo has any non-Linux kernels.

the patch for post-3.6 kernels is:

http://sources.debian.net/src/linux/3.16.7-ckt4-3/debian/patches/debian/fs-enable-link-security-restrictions-by-default.patch/
Comment 6 Daniel Kahn Gillmor 2015-02-13 20:02:38 UTC
Created attachment 396408 [details, diff]
patch from debian kernels (post 3.6)

I'm attaching debian's patch to set these values to 1 by default.  I don't think we've gotten any pushback about it, and it's been enabled since the release of wheezy (well over a year ago)
Comment 7 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 20:09:52 UTC
(In reply to Daniel Kahn Gillmor from comment #6)
> Created attachment 396408 [details, diff] [details, diff]
> patch from debian kernels (post 3.6)
> 
> I'm attaching debian's patch to set these values to 1 by default.  I don't
> think we've gotten any pushback about it, and it's been enabled since the
> release of wheezy (well over a year ago)

Since this is moving more in the discussion of kernel config I'm CCing hardened and kernel as well. I would expect this is already added in our hardened kernel? Have you received pushback on it, or should we look into using protected_hardlinks in gentoo-sources as well?

Any thoughs on what do do with our fbsd etc kernels?
Comment 8 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-13 20:19:21 UTC
Ok, using alias is a bad idea for this bug as it doesn't actually give access to the individual developers. mea culpa. 

Adding some individual developers from the various projects instead.
Comment 9 Daniel Kahn Gillmor 2015-02-13 20:42:02 UTC
fwiw, even with this mitigation in the distro's kernel by default, i'm uncomfortable claiming that this sort of problem is resolved, mainly because we want to let people run their own kernels and because not all kernels support such a mitigation.

In general, I think blind recursive changes of file ownership are a mistake, and "fixup" code like the checkpath stuff in the sks pre_start initscript is most likely just papering over problems that exist elsewhere.

You could replace checkpath with a message that warns/fails hard if the expected ownership/permissions are not correct, so that the admin gets an immediate and clear explanation of the problem.  Or maybe you could even quarantine the files or something and go through an automated rebuild so that the service can restart?  But charging ahead and changing ownership without human intervention seems like a mistake to me.
Comment 10 Matthew Thode ( prometheanfire ) archtester Gentoo Infrastructure gentoo-dev Security 2015-02-13 21:34:23 UTC
Also, given that this really isn't a vuln in hard links (bad, but not bad enough to be called a vuln) I think it should be fixed in checkpath as well.  After all, it's checkpath that is doing the badness.  Hardlinks have always been discouraged for this reason.  Makes me wonder if they can be disabled fully at all, would be a neat experiment.
Comment 11 William Hubbs gentoo-dev 2015-02-14 04:35:24 UTC
(In reply to Matthew Thode ( prometheanfire ) from comment #10)
> Also, given that this really isn't a vuln in hard links (bad, but not bad
> enough to be called a vuln) I think it should be fixed in checkpath as well.
> After all, it's checkpath that is doing the badness.  Hardlinks have always
> been discouraged for this reason.  Makes me wonder if they can be disabled
> fully at all, would be a neat experiment.

As discussed above in comment #2, I'm not sure what the fix in checkpath  could be, especially for directories since their link counts will not be 1.
Comment 12 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-14 12:19:16 UTC
(In reply to William Hubbs from comment #11)
> (In reply to Matthew Thode ( prometheanfire ) from comment #10)
> > Also, given that this really isn't a vuln in hard links (bad, but not bad
> > enough to be called a vuln) I think it should be fixed in checkpath as well.
> > After all, it's checkpath that is doing the badness.  Hardlinks have always
> > been discouraged for this reason.  Makes me wonder if they can be disabled
> > fully at all, would be a neat experiment.
> 
> As discussed above in comment #2, I'm not sure what the fix in checkpath 
> could be, especially for directories since their link counts will not be 1.

Well, if we focus on single files to begin with (and this is already differentiated by --file), could it be implemented a check for count > 1 and if this is the case, use warning only (possibly including suggested commands to change peroms instead of it being auto-run). 

Could this behavior could be used for directories as well (all dirs, no matter the link count)? Or maybe it should be the default behavior for checkpath for both directories and files in the first place? Could we have difference in behaviour where auto-changing permission is made optional only and configurable if we want to retain this as an alternative?
Comment 13 Mike Pagano gentoo-dev 2015-02-14 16:24:23 UTC
(In reply to Daniel Kahn Gillmor from comment #9)
> fwiw, even with this mitigation in the distro's kernel by default, i'm
> uncomfortable claiming that this sort of problem is resolved, mainly because
> we want to let people run their own kernels and because not all kernels
> support such a mitigation.

I agree with this, but if the group feels we should carry this patch in gentoo-sources anyways, I can make sure this goes in for our next release.

But, I agree it's not all encompassing as people run their own kernels and we would only be protecting an unknown subset.
Comment 14 Daniel Kahn Gillmor 2015-02-14 17:07:53 UTC
(In reply to Mike Pagano from comment #13)
> (In reply to Daniel Kahn Gillmor from comment #9)
> > fwiw, even with this mitigation in the distro's kernel by default, i'm
> > uncomfortable claiming that this sort of problem is resolved, mainly because
> > we want to let people run their own kernels and because not all kernels
> > support such a mitigation.
> 
> I agree with this, but if the group feels we should carry this patch in
> gentoo-sources anyways, I can make sure this goes in for our next release.
> 
> But, I agree it's not all encompassing as people run their own kernels and
> we would only be protecting an unknown subset.

Yes, i actually do think that gentoo should carry this patch in the kernel too. I'm just saying that doesn't seem sufficient to me.
Comment 15 Daniel Kahn Gillmor 2015-02-14 17:53:19 UTC
(In reply to Kristian Fiskerstrand from comment #12)

> Well, if we focus on single files to begin with (and this is already
> differentiated by --file), could it be implemented a check for count > 1 and
> if this is the case, use warning only (possibly including suggested commands
> to change peroms instead of it being auto-run). 

I don't know how checkpath is implemented, but it strikes me that unless this is all done on a single file descriptor, there could be a TOCTOU race condition.

In C, i think something like (with much more error checking of course) avoids the race condition:

 1 fd = open(fname, flags??);
 2 fstat(fd, *stat)
 3 if (stat.st_uid != target_uid) {
 4    if (stat.st_nlink != 1) {
 5       warn(fname);
 6       errcount++;
 7    } else {
 8       fchown(fd, target_uid, -1);
 9    }
10 }

Though actually, maybe there's still a race: if the compromised service unlinks the file from the vulnerable directory between lines 1 and 2, the attack would still work.  Any suggestions how to avoid this race condition?

> Could this behavior could be used for directories as well (all dirs, no
> matter the link count)? Or maybe it should be the default behavior for
> checkpath for both directories and files in the first place? Could we have
> difference in behaviour where auto-changing permission is made optional only
> and configurable if we want to retain this as an alternative?

I think this is the safest way to go, but it sounds like it breaks the semantics of checkpath -- how widely used is checkpath?  (it's not in debian at all)  Is it used just to catch errors, or is it used pro-actively as a flavor of chown?
Comment 16 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-14 18:02:21 UTC
(In reply to Daniel Kahn Gillmor from comment #15)
> (In reply to Kristian Fiskerstrand from comment #12)
> 


> 
> I think this is the safest way to go, but it sounds like it breaks the
> semantics of checkpath -- how widely used is checkpath?  (it's not in debian
> at all)  Is it used just to catch errors, or is it used pro-actively as a
> flavor of chown?

Certainly not used a chown replacement, it is only for runtime checking of files that might change. A quick grep through the Gentoo tree shows:
/mnt/data/gentoo-x86 $ grep -r "checkpath" -- * | grep -v "ChangeLog" | wc -l
318

... but most of that is for permissions only
/mnt/data/gentoo-x86 $ grep -r "checkpath" -- * | grep -v "ChangeLog" | grep -- "--owner" | wc -l
24

So it isn't as wildly used that I'd consider it blocking a change in behavior as long as it is communicated
Comment 17 Daniel Kahn Gillmor 2015-02-14 18:53:22 UTC
(In reply to Kristian Fiskerstrand from comment #16)

> Certainly not used a chown replacement, it is only for runtime checking of
> files that might change. A quick grep through the Gentoo tree shows:
> /mnt/data/gentoo-x86 $ grep -r "checkpath" -- * | grep -v "ChangeLog" | wc -l
> 318
> 
> ... but most of that is for permissions only
> /mnt/data/gentoo-x86 $ grep -r "checkpath" -- * | grep -v "ChangeLog" | grep
> -- "--owner" | wc -l
> 24

Again, i don't have a copy of checkpath or its documentation, but i suspect changing ownership isn't the only risk here; can it change the group, or the permissions, or other file metadata?  If so, all of these modifications are a risk.  Can you point me to a manpage or something?  I don't have a gentoo installation set up right now.

> So it isn't as wildly used that I'd consider it blocking a change in
> behavior as long as it is communicated

cool, this sounds like the right way to go to me.  If you do phased deprecation, you could try something like this (assuming checkpath is on version n right now):

 version n+1: when checkpath is invoked with --owner, and it is going to change ownership, go ahead and change the ownership, but also emit a warning to stderr "In version n+3, checkpath will not actively change ownership for you, and will only warn about ownership.  If you want to manually change ownership, you should be using chown (while being aware of the risks)."

 version n+2:  change ownership,  warn, and return a non-zero error code.

 version n+3: warn, and return a non-zero error code.

-----

or, you could collapse that into a two-stage transition.  (or maybe it's better to just rip the band-aid off entirely at once)
Comment 18 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-14 20:40:44 UTC
(In reply to Daniel Kahn Gillmor from comment #17)
> (In reply to Kristian Fiskerstrand from comment #16)
> 

..

> -----
> 
> or, you could collapse that into a two-stage transition.  (or maybe it's
> better to just rip the band-aid off entirely at once)

I'd consider a Gentoo NEWS item for the OpenRC upgrade some weeks in advance sufficient notice without any deprecation path.
Comment 19 Mike Pagano gentoo-dev 2015-02-14 23:15:22 UTC
(In reply to Daniel Kahn Gillmor from comment #14)
> (In reply to Mike Pagano from comment #13)
> > (In reply to Daniel Kahn Gillmor from comment #9)
> > > fwiw, even with this mitigation in the distro's kernel by default, i'm
> > > uncomfortable claiming that this sort of problem is resolved, mainly because
> > > we want to let people run their own kernels and because not all kernels
> > > support such a mitigation.
> > 
> > I agree with this, but if the group feels we should carry this patch in
> > gentoo-sources anyways, I can make sure this goes in for our next release.
> > 
> > But, I agree it's not all encompassing as people run their own kernels and
> > we would only be protecting an unknown subset.
> 
> Yes, i actually do think that gentoo should carry this patch in the kernel
> too. I'm just saying that doesn't seem sufficient to me.

Queued up for the next release of 3.10, 3.12, 3.14, 3.18 and 3.19.
Comment 20 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-15 13:27:43 UTC
(In reply to Mike Pagano from comment #19)

..

> Queued up for the next release of 3.10, 3.12, 3.14, 3.18 and 3.19.

Very good, thanks!
Comment 21 William Hubbs gentoo-dev 2015-02-15 19:11:43 UTC
(In reply to Mike Pagano from comment #19)
> (In reply to Daniel Kahn Gillmor from comment #14)
> > (In reply to Mike Pagano from comment #13)
> > > (In reply to Daniel Kahn Gillmor from comment #9)
> > > > fwiw, even with this mitigation in the distro's kernel by default, i'm
> > > > uncomfortable claiming that this sort of problem is resolved, mainly because
> > > > we want to let people run their own kernels and because not all kernels
> > > > support such a mitigation.
> > > 
> > > I agree with this, but if the group feels we should carry this patch in
> > > gentoo-sources anyways, I can make sure this goes in for our next release.
> > > 
> > > But, I agree it's not all encompassing as people run their own kernels and
> > > we would only be protecting an unknown subset.
> > 
> > Yes, i actually do think that gentoo should carry this patch in the kernel
> > too. I'm just saying that doesn't seem sufficient to me.
> 
> Queued up for the next release of 3.10, 3.12, 3.14, 3.18 and 3.19.

Is there any chance we can send this to lkml and see if we can get it pushed upstream?
Comment 22 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-15 19:12:39 UTC
(In reply to William Hubbs from comment #21)
> (In reply to Mike Pagano from comment #19)
> > (In reply to Daniel Kahn Gillmor from comment #14)
> > > (In reply to Mike Pagano from comment #13)
> > > > (In reply to Daniel Kahn Gillmor from comment #9)
> > > > > fwiw, even with this mitigation in the distro's kernel by default, i'm
> > > > > uncomfortable claiming that this sort of problem is resolved, mainly because


..


> > 
> > Queued up for the next release of 3.10, 3.12, 3.14, 3.18 and 3.19.
> 
> Is there any chance we can send this to lkml and see if we can get it pushed
> upstream?

The upstream that first and formost needs fixing is OpenRC's checkpath
Comment 23 William Hubbs gentoo-dev 2015-02-15 19:22:49 UTC
(In reply to Kristian Fiskerstrand from comment #22)
> The upstream that first and formost needs fixing is OpenRC's checkpath


I did not say I was not going to fix checkpath.

I'm just saying that, since two distros feel that hardlinks should be protected at the kernel level, we might want to push that kernel patch upstream.
Comment 24 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-15 19:39:46 UTC
(In reply to William Hubbs from comment #23)
> (In reply to Kristian Fiskerstrand from comment #22)
> > The upstream that first and formost needs fixing is OpenRC's checkpath
> 
> 
> I did not say I was not going to fix checkpath.
> 
> I'm just saying that, since two distros feel that hardlinks should be
> protected at the kernel level, we might want to push that kernel patch
> upstream.

That is indeed a fair point. I suggest we come back to that at a later stage. It is already discussed at least on [0] and it seems the behavior was turned the other way around in [1]: "In commit 800179c9b8a1 ("This adds symlink and hardlink restrictions to
the Linux VFS"), the new link protections were enabled by default, in
the hope that no actual application would care, despite it being
technically against legacy UNIX (and documented POSIX) behavior.

However, it does turn out to break some applications.  It's rare, and
it's unfortunate, but it's unacceptable to break existing systems, so
we'll have to default to legacy behavior.
"

So immediately I wouldn't find it likely that upstream will go away from legacy behavior, even if that was the original intention until a few corner cases occured and they needed to keep backwards campatibility. I consider that a good case for keeping the patch downstream.

References:
[0] http://lwn.net/Articles/521626/
[1] http://www.spinics.net/lists/stable-commits/msg21052.html
Comment 25 William Hubbs gentoo-dev 2015-02-15 21:04:21 UTC
Created attachment 396558 [details, diff]
0001-checkpath-security-fix-for-chown-and-chmod.patch

This patch should make the mode and permission changes fail if the
target is a file and has more than one hard link.

Please test.

Thanks,

William
Comment 26 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-15 21:22:22 UTC
(In reply to William Hubbs from comment #25)
> Created attachment 396558 [details, diff] [details, diff]
> 0001-checkpath-security-fix-for-chown-and-chmod.patch
> 
> This patch should make the mode and permission changes fail if the
> target is a file and has more than one hard link.
> 
> Please test.
> 

Nice. As openrc is kind enough to use epatch_user testing this is quite easy as well, just throwing the patch in /etc/portage/patches/sys-apps/openrc/ and re-emerging I now get the following when attempting the same attack:

eta_sks2 sks # ls -l passwd.log 
-rw-r--r-- 2 root root 1146 Feb 13 20:52 passwd.log

eta_sks2 sks # /etc/init.d/sks-db restart
 * Stopping SKS db ...                                                    [ ok ]
 * checkpath: chown: Too many hard links to /var/lib/sks/passwd.log
 * ERROR: sks-db failed to start

eta_sks2 sks # rm passwd.log 

eta_sks2 sks # /etc/init.d/sks-db restart
 * Starting SKS db ...                                                    [ ok ]

So that blocks this attack
Comment 27 William Hubbs gentoo-dev 2015-02-15 22:14:23 UTC
This is on master in commit b17af3c, and will be included in
OpenRC-0.14.
Let me know if we should have another 0.13.x release.
Comment 28 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-15 22:26:16 UTC
(In reply to William Hubbs from comment #27)
> This is on master in commit b17af3c, and will be included in
> OpenRC-0.14.
> Let me know if we should have another 0.13.x release.

It depends on the release schedule of course. I'm not sure what other features etc have 0.14 for milestone? If it is a while down the road I'd prefer a new 0.13.x release for this as well.
Comment 29 Daniel Kahn Gillmor 2015-02-16 08:00:59 UTC
(In reply to William Hubbs from comment #25)
> Created attachment 396558 [details, diff] [details, diff]
> 0001-checkpath-security-fix-for-chown-and-chmod.patch
> 
> This patch should make the mode and permission changes fail if the
> target is a file and has more than one hard link.

Thanks for the patch!

This still leaves a race condition, unfortunately. Between the call to stat() on line 71 and the calls to chmod() and chown() on lines 137 and 145 is a window of time that the owner of the parent directory can unlink() the current path and link in any other file in its place.

Perhaps the answer is "we're ok leaving that race condition since it seems quite short"?

Or you could make it so that mismatches of owner, group, or permissions are all treated in the same way that checkpath treats things that should be normal files but are directories (or vice versa).
Comment 30 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-16 08:40:55 UTC
(In reply to Daniel Kahn Gillmor from comment #29)
> (In reply to William Hubbs from comment #25)
> > Created attachment 396558 [details, diff] [details, diff] [details, diff]
> > 0001-checkpath-security-fix-for-chown-and-chmod.patch
> > 
> > This patch should make the mode and permission changes fail if the
> > target is a file and has more than one hard link.
> 
> Thanks for the patch!
> 
> This still leaves a race condition, unfortunately. Between the call to
> stat() on line 71 and the calls to chmod() and chown() on lines 137 and 145
> is a window of time that the owner of the parent directory can unlink() the
> current path and link in any other file in its place.
> 
> Perhaps the answer is "we're ok leaving that race condition since it seems
> quite short"?

I'm personally fine with that race condition (slim as it is) existing. For one thing, one difference here is that the process would need to hook into the init scripts and try to monitor when it is executed a start in order for that race condition to be applicable. 

> 
> Or you could make it so that mismatches of owner, group, or permissions are
> all treated in the same way that checkpath treats things that should be
> normal files but are directories (or vice versa).
Comment 31 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-17 20:00:16 UTC
Auditing bugs are restricted to Security by default. Given that this is verified as an issue upstream and a fix exists, as well as the issue is described in the commit message I'm lifting the restriction.
Comment 32 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-17 21:20:24 UTC
(In reply to Kristian Fiskerstrand from comment #31)
> Auditing bugs are restricted to Security by default. Given that this is
> verified as an issue upstream and a fix exists, as well as the issue is
> described in the commit message I'm lifting the restriction.

For the record: I consider this a security hardening measure and not a vulnerability per se, as such I have not, and do not intend to request a CVE for this issue. If anyone objects to this assessment please let it be heard.
Comment 33 Daniel Kahn Gillmor 2015-02-19 15:29:40 UTC
(In reply to Kristian Fiskerstrand from comment #32)
> (In reply to Kristian Fiskerstrand from comment #31)
> > Auditing bugs are restricted to Security by default. Given that this is
> > verified as an issue upstream and a fix exists, as well as the issue is
> > described in the commit message I'm lifting the restriction.
> 
> For the record: I consider this a security hardening measure and not a
> vulnerability per se, as such I have not, and do not intend to request a CVE
> for this issue. If anyone objects to this assessment please let it be heard.

The attack vector appears to be a delayed privilege escalation to root by any service that uses checkpath in its initscripts with the --owner option.

Mitigations are:

 * some kernels make it impossible to link to files you do not own or otherwise control. (the stock gentoo kernel is enabling this feature soon)
 * The attack only works within the filesystem reviewed by checkpath, so systems with granular filesystems may be less exposed.
 * the privilege escalation is delayed, not immediate; it happens only after a service is restarted, and presumably the attacking service then still needs to do something for escalation like appending to /etc/passwd or /etc/sudoers.


On the flip side, the fixes that have been put in place during the course of this discussion still have a slight race condition.

It still seems like a vulnerability to me, and not a hardening measure, but the mitigations that are in place certainly do constrain the attack.


It occurs to me to ask whether anyone has reviewed what checkpath does with symlinks -- does it act on the referenced file or on the symlink itself?
Comment 34 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-19 16:02:24 UTC
(In reply to Daniel Kahn Gillmor from comment #33)


> It occurs to me to ask whether anyone has reviewed what checkpath does with
> symlinks -- does it act on the referenced file or on the symlink itself?

Thanks for asking this. I did the silly thing and assumed a result, as usual, I should not have done that.


eta_sks2 sks # ln -s /etc/passwd passwd.log

eta_sks2 sks # ls -l /etc/passwd
-rw-r--r-- 1 root root 1146 Feb 13 20:52 /etc/passwd

eta_sks2 sks # ls -l passwd.log 
lrwxrwxrwx 1 root root 11 Feb 19 16:59 passwd.log -> /etc/passwd

# /etc/init.d/sks-db restart
 * Stopping SKS db ...                                                    [ ok ]
 * /var/lib/sks/passwd.log: correcting owner
 * Starting SKS db ...     

 ls -l /etc/passwd
-rw-r--r-- 1 sks sks 1146 Feb 13 20:52 /etc/passwd

So symlink is also affected by this
Comment 35 William Hubbs gentoo-dev 2015-02-19 19:12:05 UTC
Created attachment 396990 [details, diff]
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

This patch should take care of the symbolic link case.
Please test and let me know.

Thanks,

William
Comment 36 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-19 19:37:19 UTC
Comment on attachment 396990 [details, diff]
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

Defunct
Comment 37 William Hubbs gentoo-dev 2015-02-19 19:42:42 UTC
Created attachment 396992 [details, diff]
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

There is one more change that needed to be made; we should be using
lstat() instead of stat().

This updated version of the patch takes care of that.

Please test and let me know if it works.

Thanks,

William
Comment 38 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-19 21:02:26 UTC
Comment on attachment 396992 [details, diff]
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

Obsoleted
Comment 39 William Hubbs gentoo-dev 2015-02-19 21:19:05 UTC
Created attachment 396998 [details, diff]
0001-checkpath-do-not-chown-or-chmod-symbolic-links.patch

The attached patch is the fix for this issue. It is applied in commit
a0378f3. It will also be back-ported to openrc-0.13.11.
Comment 40 Anthony Basile gentoo-dev 2015-02-20 13:29:33 UTC
(In reply to Kristian Fiskerstrand from comment #30)
> (In reply to Daniel Kahn Gillmor from comment #29)
> > Perhaps the answer is "we're ok leaving that race condition since it seems
> > quite short"?
> 
> I'm personally fine with that race condition (slim as it is) existing. For
> one thing, one difference here is that the process would need to hook into
> the init scripts and try to monitor when it is executed a start in order for
> that race condition to be applicable. 
> 

You can avoid the race when checking for a sym link by using readlink() which is atomic.  If it succeeds, then you have a sym link.  I'm not sure how to do the same with hard links without more complicated code.

Slim races can still be triggered by brute forcing attempts until they succeed even in this case --- yes its like look for a tiny needle of a critical window in a large haystack which is the time for the script to run, but why not?  You just keep trying until you hit it.  It still worth thinking about how to remove the race completely.
Comment 41 Daniel Kahn Gillmor 2015-02-20 15:59:22 UTC
(In reply to Anthony Basile from comment #40)

> You can avoid the race when checking for a sym link by using readlink()
> which is atomic.  If it succeeds, then you have a sym link.  I'm not sure
> how to do the same with hard links without more complicated code.

how does this avoid the race condition?  If readlink() fails, then you don't have a symlink, so you proceed to operate on it.  The race now exists in the time between the call to readlink() and the operation, right?

And if it *is* a symlink, and the target of the symlink doesn't have the expected ownership/permissions, what should checkpath do about it?

> Slim races can still be triggered by brute forcing attempts until they
> succeed even in this case --- yes its like look for a tiny needle of a
> critical window in a large haystack which is the time for the script to run,
> but why not?  You just keep trying until you hit it.  It still worth
> thinking about how to remove the race completely.

I'm not convinced that there is such a way, though i'd be happy to learn otherwise.

I think these doing these operations in an automated fashion is inherently dangerous, and is likely papering over some other defect, since they shouldn't be necessary in the first place.  If you want to remove the automated race condition, you should make checkpath into a warning-only tool, and expose the anomolous situation to human scrutiny.

Another approach, i suppose, would be to open a new file in the same directory, set its ownership and permissions appropriately, copy the content from the old file, and atomically move the new file to the old name, thereby unlinking the old file.  This sounds like it might have a lot of scary side effects i haven't even considered, though.
Comment 42 William Hubbs gentoo-dev 2015-02-20 21:32:09 UTC
(In reply to Daniel Kahn Gillmor from comment #41)
> I think these doing these operations in an automated fashion is inherently
> dangerous, and is likely papering over some other defect, since they
> shouldn't be necessary in the first place.  If you want to remove the
> automated race condition, you should make checkpath into a warning-only
> tool, and expose the anomolous situation to human scrutiny.

That depends. If the file or directory checkpath is fixing is in /run, it may not exist at all, so checkpath needs to create it.

If I do make it into a warning only tool in the future, that would be a significant change to user-facing behaviour. In fact, I think it would obsolete most of checkpath's functionality since most of those tests can be done via posix sh.

> Another approach, i suppose, would be to open a new file in the same
> directory, set its ownership and permissions appropriately, copy the content
> from the old file, and atomically move the new file to the old name, thereby
> unlinking the old file.  This sounds like it might have a lot of scary side
> effects i haven't even considered, though.

Yes, that sounds pretty scary, because we don't know what the file or directory contains. Also, it wouldn't work for pipes or fifos.
Comment 43 Sergey Popov gentoo-dev 2015-02-24 14:05:54 UTC
(In reply to Daniel Kahn Gillmor from comment #2)
> And i don't know what you'd have to do for the directory checks either,
> since they've all got link counts > 1 typically..

Is there any filesystem at least in Linux that is capable of doing directory(not file, but directory hardlinks)?

web cluster # ln 1 2
ln: '1': hard link not allowed for directory

That's happened for me on ext4, ceph, tmpfs and reiserfs. But i am in doubt if this restriction is local to those fs and not whole Linux VFS code(still need someone familiar with kernel to verify my thoughts, though)
Comment 44 Kristian Fiskerstrand (RETIRED) gentoo-dev 2015-02-28 21:55:26 UTC
(In reply to Sergey Popov from comment #43)
> (In reply to Daniel Kahn Gillmor from comment #2)
> > And i don't know what you'd have to do for the directory checks either,
> > since they've all got link counts > 1 typically..
> 
> Is there any filesystem at least in Linux that is capable of doing
> directory(not file, but directory hardlinks)?

I'm not familiar with any. As for the original report this issue has now been resolved. Should we RESOVLE FIXED it?
Comment 45 Sergei Trofimovich (RETIRED) gentoo-dev 2016-06-03 22:29:42 UTC
(In reply to Sergey Popov from comment #43)
> (In reply to Daniel Kahn Gillmor from comment #2)
> > And i don't know what you'd have to do for the directory checks either,
> > since they've all got link counts > 1 typically..
> 
> Is there any filesystem at least in Linux that is capable of doing
> directory(not file, but directory hardlinks)?
> 
> web cluster # ln 1 2
> ln: '1': hard link not allowed for directory
> 
> That's happened for me on ext4, ceph, tmpfs and reiserfs. But i am in doubt
> if this restriction is local to those fs and not whole Linux VFS code(still
> need someone familiar with kernel to verify my thoughts, though)

It's a generic linux feature. Directory hardlinks are prohibited. As a result single mountpoint is still a tree. Done thusly:

link syscall starts here:
http://lxr.free-electrons.com/source/fs/namei.c#L4161

4161 SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname)

and the actual check is here:

http://lxr.free-electrons.com/source/fs/namei.c#L4056

4033 int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry, struct inode **delegated_inode)
4034 {
4035         struct inode *inode = old_dentry->d_inode;
...
4056         if (S_ISDIR(inode->i_mode))
4057                 return -EPERM;