[PATCH] Filesystem linking protections

[PATCH] Filesystem linking protections

[Lorenzo Hernández García-Hierro]

[-- Attachment #1.1: Type: text/plain, Size: 1325 bytes --]

Hi,

This patch adds two checks to do_follow_link() and sys_link(), for
prevent users to follow (untrusted) symlinks owned by other users in
world-writable +t directories (i.e. /tmp), unless the owner of the
symlink is the owner of the directory, users will also not be able to
hardlink to files they do not own.

The direct advantage of this pretty simple patch is that /tmp races will
be prevented.

Results reported by the Collision Regression Test Suite with patch
applied:
 (...)
 Symlink restrictions                     : Not vulnerable
 Hardlinking restrictions                 : Not vulnerable
 (...)
Results with patch *not applied*:
 (...)
 Symlink restrictions                     : Vulnerable
 Hardlinking restrictions                 : Vulnerable
 (...)

This patch is based on grSecurity linking protections, but first
implemented by the OpenWall patch.

I propose it's merging, as the overhead is *minimal* (if there's any
overhead), because the modified functions get called only once when
following a symlink or creating a hardlink.

The patch can be also downloaded from:
http://pearls.tuxedo-es.org/patches/linking-protections-2.6.11-rc3.patch

Cheers,
-- 
Lorenzo Hernández García-Hierro <lorenzo@gnu.org> 
[1024D/6F2B2DEC] & [2048g/9AE91A22][http://tuxedo-es.org]


[-- Attachment #1.2: linking-protections-2.6.11-rc3.patch --]
[-- Type: text/x-patch, Size: 1864 bytes --]

diff -Nur linux-2.6.11-rc3/fs/namei.c linux-2.6.11-rc3.slink/fs/namei.c
--- linux-2.6.11-rc3/fs/namei.c	2005-02-06 21:40:41.000000000 +0100
+++ linux-2.6.11-rc3.slink/fs/namei.c	2005-02-07 19:15:22.690689272 +0100
@@ -519,6 +519,19 @@
 	err = security_inode_follow_link(dentry, nd);
 	if (err)
 		goto loop;
+
+	/* Prevent users to follow symlinks owned by other users in
+	 * world-writable +t directories, unless the owner of the
+	 * symlink is the owner of the directory.
+	 */
+	if (S_ISLNK(dentry->d_inode->i_mode) &&
+	    (dentry->d_parent->d_inode->i_mode & S_ISVTX) 
+	    && (dentry->d_parent->d_inode->i_uid != dentry->d_inode->i_uid) &&
+	    (dentry->d_parent->d_inode->i_mode & S_IWOTH) && (current->fsuid != dentry->d_inode->i_uid)) {
+		err = -EACCES;
+		goto loop;
+	}
+			
 	current->link_count++;
 	current->total_link_count++;
 	nd->depth++;
@@ -1985,7 +1998,22 @@
 	new_dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(new_dentry);
 	if (!IS_ERR(new_dentry)) {
-		error = vfs_link(old_nd.dentry, nd.dentry->d_inode, new_dentry);
+		error = 0;
+		
+		/* Check that the user who is trying to make the hardlink owns
+		 * the target file being linked (DAC->@old_nd.dentry->d_inode) */
+		if (current->fsuid != old_nd.dentry->d_inode->i_uid && 
+		(!S_ISREG(old_nd.dentry->d_inode->i_mode) || (old_nd.dentry->d_inode->i_mode & S_ISUID) || 
+		((old_nd.dentry->d_inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) ||
+	     	(generic_permission(old_nd.dentry->d_inode, MAY_READ | MAY_WRITE, NULL))) &&
+	    	!capable(CAP_FOWNER) && current->uid) {
+		error = -EPERM;
+		}
+		
+		/* If @error is empty, then we apply the *normal* behavior */
+		if (!error)
+			error = vfs_link(old_nd.dentry, nd.dentry->d_inode, new_dentry);
+		
 		dput(new_dentry);
 	}
 	up(&nd.dentry->d_inode->i_sem);

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Filesystem linking protections

[Chris Wright]

* Lorenzo Hernández García-Hierro (lorenzo@gnu.org) wrote:
> This patch adds two checks to do_follow_link() and sys_link(), for
> prevent users to follow (untrusted) symlinks owned by other users in
> world-writable +t directories (i.e. /tmp), unless the owner of the
> symlink is the owner of the directory, users will also not be able to
> hardlink to files they do not own.
> 
> The direct advantage of this pretty simple patch is that /tmp races will
> be prevented.

The disadvantage is that it can break things and places policy in the
kernel.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Filesystem linking protections

[Lorenzo Hernández García-Hierro]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

El lun, 07-02-2005 a las 11:12 -0800, Chris Wright escribió:
> * Lorenzo Hernández García-Hierro (lorenzo@gnu.org) wrote:
> > This patch adds two checks to do_follow_link() and sys_link(), for
> > prevent users to follow (untrusted) symlinks owned by other users in
> > world-writable +t directories (i.e. /tmp), unless the owner of the
> > symlink is the owner of the directory, users will also not be able to
> > hardlink to files they do not own.
> > 
> > The direct advantage of this pretty simple patch is that /tmp races will
> > be prevented.
> 
> The disadvantage is that it can break things and places policy in the
> kernel.

It's just like DAC then, because it never applies any policy than a
simple check relying on kernel's DAC, and standard capabilities &
permissions.DAC-related checks are placed all over the place, but maybe
the place is lacking of some ones that may be important.

About what things it can break, I haven't noticed any issue on it (at
least regarding grSecurity or OpenWall), but of course I would
appreciate a lot any information on them, so, I could report to the
developers that are currently using this in their own solutions.

Thanks in advance,
Cheers.
-- 
Lorenzo Hernández García-Hierro <lorenzo@gnu.org> 
[1024D/6F2B2DEC] & [2048g/9AE91A22][http://tuxedo-es.org]

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Filesystem linking protections

[Chris Wright]

* Lorenzo Hernández García-Hierro (lorenzo@gnu.org) wrote:
> About what things it can break, I haven't noticed any issue on it (at
> least regarding grSecurity or OpenWall), but of course I would
> appreciate a lot any information on them, so, I could report to the
> developers that are currently using this in their own solutions.

In the past it has broken atd and courier.  The hardlink restrictions
had to be relaxed in both cases.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Filesystem linking protections

[John Richard Moser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Chris Wright wrote:
> * Lorenzo Hernández García-Hierro (lorenzo@gnu.org) wrote:
> 
>>This patch adds two checks to do_follow_link() and sys_link(), for
>>prevent users to follow (untrusted) symlinks owned by other users in
>>world-writable +t directories (i.e. /tmp), unless the owner of the
>>symlink is the owner of the directory, users will also not be able to
>>hardlink to files they do not own.
>>
>>The direct advantage of this pretty simple patch is that /tmp races will
>>be prevented.
> 
> 
> The disadvantage is that it can break things and places policy in the
> kernel.
> 

It can break things, yes.  For example, programs which have and use two
separate FS UIDs at the same time, or which attempt to make hardlinks to
files they don't own without CAP_FOWNER or root (should this just be
CAP_FOWNER?  Is root now irrelavent?).

Hang on, when do any programs have 2 FS UIDs at the same time. . . .

I've yet to see this break anything on Ubuntu or Gentoo; Brad Spengler
claims this breaks nothing on Debian.  On the other hand, this could
potentially squash the second most prevalent security bug.

> thanks,
> -chris

- --
All content of all messages exchanged herein are left in the
Public Domain, unless otherwise explicitly stated.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCB8S0hDd4aOud5P8RAvYSAJ9zcGArfbC6i5uM1JW4ZHdELriUzACeOH/q
5ndpSdjporfnFAMK1OrMASE=
=XjWB
-----END PGP SIGNATURE-----

Re: [PATCH] Filesystem linking protections

[Chris Wright]

* John Richard Moser (nigelenki@comcast.net) wrote:
> I've yet to see this break anything on Ubuntu or Gentoo; Brad Spengler
> claims this breaks nothing on Debian.  On the other hand, this could
> potentially squash the second most prevalent security bug.

Yes I know, I've worked on distro with it as well in the past.  And it
has broken atd and courier in the past.  This is something that also
can be done in userspace using sane subdirs in +t world writable dirs,
or O_EXCL so there's work to be done in userspace.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Filesystem linking protections

[John Richard Moser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Chris Wright wrote:
> * John Richard Moser (nigelenki@comcast.net) wrote:
> 
>>I've yet to see this break anything on Ubuntu or Gentoo; Brad Spengler
>>claims this breaks nothing on Debian.  On the other hand, this could
>>potentially squash the second most prevalent security bug.
> 
> 
> Yes I know, I've worked on distro with it as well in the past.  And it
> has broken atd and courier in the past.  This is something that also
> can be done in userspace using sane subdirs in +t world writable dirs,
> or O_EXCL so there's work to be done in userspace.
> 

Yes, mkdtemp() and mkstemp().

Of course we can't always rely on programmers to get it right, so the
idea here is to make sure we ask broken code to behave nicely, and stab
it in the face if it doesn't.  Please try to examine this in that scope.

> thanks,
> -chris

- --
All content of all messages exchanged herein are left in the
Public Domain, unless otherwise explicitly stated.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCB+vThDd4aOud5P8RAssCAJ9L7Cf5pnvI8GdKs1P4cpM2lJvtYACZAXee
a5kkPkxXm9YK0DFSfvDd6fQ=
=00DK
-----END PGP SIGNATURE-----

Re: [PATCH] Filesystem linking protections

[Chris Wright]

* John Richard Moser (nigelenki@comcast.net) wrote:
> Yes, mkdtemp() and mkstemp().
> 
> Of course we can't always rely on programmers to get it right, so the
> idea here is to make sure we ask broken code to behave nicely, and stab
> it in the face if it doesn't.  Please try to examine this in that scope.

It's fine for hardened distro.  But still inappropriate for mainline.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Filesystem linking protections

[John Richard Moser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Chris Wright wrote:
> * John Richard Moser (nigelenki@comcast.net) wrote:
> 
>>Yes, mkdtemp() and mkstemp().
>>
>>Of course we can't always rely on programmers to get it right, so the
>>idea here is to make sure we ask broken code to behave nicely, and stab
>>it in the face if it doesn't.  Please try to examine this in that scope.
> 
> 
> It's fine for hardened distro.  But still inappropriate for mainline.
> 

Perhaps in mainline as an option?  The [*] notations next to things are
really nice, they let you turn kernel stuff on and off :)  It's
appropriate for mainline to support added security isn't it?  I think
following the path of supporting-but-not-forcing is the best route,
because it encourages people to account for systems which may take
advantage of such options, and thus leads to a software base in which
it's quite sane to actually enable those options globally.

That's just how I think though.

> thanks,
> -chris

- --
All content of all messages exchanged herein are left in the
Public Domain, unless otherwise explicitly stated.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCCB+GhDd4aOud5P8RAlD9AJ45JTY20WY6qHe0h0ZIcFasgxJDtACbB1aB
i4hytMAy6Cs1AUNXC296JOk=
=oLVs
-----END PGP SIGNATURE-----

Re: [PATCH] Filesystem linking protections

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Mon, 07 Feb 2005 19:57:06 +0100, Lorenzo =?ISO-8859-1?Q?Hern=E1ndez_?= =?ISO-8859-1?Q?Garc=EDa-Hierro?= said:

> This patch adds two checks to do_follow_link() and sys_link(), for
> prevent users to follow (untrusted) symlinks owned by other users in
> world-writable +t directories (i.e. /tmp), unless the owner of the
> symlink is the owner of the directory, users will also not be able to
> hardlink to files they do not own.

This should be done using the LSM framework.  That's what it's *THERE* for.
I've previously posted an LSM that does these checks (and a few others), it
should be in the archives.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Filesystem linking protections

[Lorenzo Hernández García-Hierro]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

El lun, 07-02-2005 a las 14:14 -0500, Valdis.Kletnieks@vt.edu escribió:
> On Mon, 07 Feb 2005 19:57:06 +0100, Lorenzo =?ISO-8859-1?Q?Hern=E1ndez_?= =?ISO-8859-1?Q?Garc=EDa-Hierro?= said:
> 
> > This patch adds two checks to do_follow_link() and sys_link(), for
> > prevent users to follow (untrusted) symlinks owned by other users in
> > world-writable +t directories (i.e. /tmp), unless the owner of the
> > symlink is the owner of the directory, users will also not be able to
> > hardlink to files they do not own.
> 
> This should be done using the LSM framework.  That's what it's *THERE* for.
> I've previously posted an LSM that does these checks (and a few others), it
> should be in the archives.

vSecurity also implements this and many other "ported" features from
OpenWall and grSecurity.

But It's better to give users a "secure-by-default" status, at least on
those parts that don't affect negatively the stability or the
performance itself.

The LSM hook call is before the check, so, LSM framework still has the
control over it, until it releases the operation giving control back to
the standard function.

If users must rely on LSM or other external solutions for applying basic
security checks (as the framework itself only provides the way to apply
them, the checks need to be implemented in a module), then we are making
them unable to be protected using the "default" configuration.

Cheers,
-- 
Lorenzo Hernández García-Hierro <lorenzo@gnu.org> 
[1024D/6F2B2DEC] & [2048g/9AE91A22][http://tuxedo-es.org]

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Filesystem linking protections

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Mon, 07 Feb 2005 20:34:33 +0100, Lorenzo =?ISO-8859-1?Q?Hern=E1ndez_?= =?ISO-8859-1?Q?Garc=EDa-Hierro?= said:

> But It's better to give users a "secure-by-default" status, at least on
> those parts that don't affect negatively the stability or the
> performance itself.

It's still policy, and should be put someplace where users can manage it.
You're changing the behavior from what POSIX specifies, and that's in general
a no-no for mainline kernel code.

Like an LSM, which happens to be there so users can impose policy without
making any code changes to the kernel.  Implementing a policy that results in
non-POSIXy behavior in an LSM is perfectly OK.. ;)

> The LSM hook call is before the check, so, LSM framework still has the
> control over it, until it releases the operation giving control back to
> the standard function.

Right.. Which means LSM can stop that particular attack even faster than
your patch.. ;)

> If users must rely on LSM or other external solutions for applying basic
> security checks (as the framework itself only provides the way to apply
> them, the checks need to be implemented in a module), then we are making
> them unable to be protected using the "default" configuration.

You're making the very rash assumption that a hard-coded one-size-fits all
"default" that behaves differently than POSIX is suitable for all sites,
including sites that run software that gets broken by this change, and
things like embedded systems where it's not a concern at all, and sites that
already implement some *other* system to ensure that it's not an issue (for
instance, by using an SELinux policy...)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Filesystem linking protections

[Lorenzo Hernández García-Hierro]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

El lun, 07-02-2005 a las 16:45 -0500, Valdis.Kletnieks@vt.edu escribió:
> On Mon, 07 Feb 2005 20:34:33 +0100, Lorenzo =?ISO-8859-1?Q?Hern=E1ndez_?= =?ISO-8859-1?Q?Garc=EDa-Hierro?= said:
> 
> > But It's better to give users a "secure-by-default" status, at least on
> > those parts that don't affect negatively the stability or the
> > performance itself.
> 
> It's still policy, and should be put someplace where users can manage it.
> You're changing the behavior from what POSIX specifies, and that's in general
> a no-no for mainline kernel code.

A sysctl can be a good option, creating a CTL_SECURITY and then
registering stuff under it, but this requires to have the kernel hackers
agree with implementing a new security suite and such.
In short, re-inventing the wheel.

> Like an LSM, which happens to be there so users can impose policy without
> making any code changes to the kernel.  Implementing a policy that results in
> non-POSIXy behavior in an LSM is perfectly OK.. ;)

It's currently made in vSecurity :)

> > The LSM hook call is before the check, so, LSM framework still has the
> > control over it, until it releases the operation giving control back to
> > the standard function.
> 
> Right.. Which means LSM can stop that particular attack even faster than
> your patch.. ;)

At least I don't interfere with LSM, so, if no LSM hook adds it's own
security checks, then it gets used.

> > If users must rely on LSM or other external solutions for applying basic
> > security checks (as the framework itself only provides the way to apply
> > them, the checks need to be implemented in a module), then we are making
> > them unable to be protected using the "default" configuration.
> 
> You're making the very rash assumption that a hard-coded one-size-fits all
> "default" that behaves differently than POSIX is suitable for all sites,
> including sites that run software that gets broken by this change, and
> things like embedded systems where it's not a concern at all, and sites that
> already implement some *other* system to ensure that it's not an issue (for
> instance, by using an SELinux policy...)

Good point, then the solution is to make it config-dependent, and that's
a thing that kernel hackers seem to dislike.

Lemme know what's the final thought on this, so, I could work out it and
give what you want, without time loss and we all can feel happy with
it :)

Cheers and thanks for the comments,
-- 
Lorenzo Hernández García-Hierro <lorenzo@gnu.org> 
[1024D/6F2B2DEC] & [2048g/9AE91A22][http://tuxedo-es.org]

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Filesystem linking protections

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Mon, 07 Feb 2005 23:00:33 +0100, Lorenzo =?ISO-8859-1?Q?Hern=E1ndez_?= =?ISO-8859-1?Q?Garc=EDa-Hierro?= said:

> A sysctl can be a good option, creating a CTL_SECURITY and then
> registering stuff under it, but this requires to have the kernel hackers
> agree with implementing a new security suite and such.
> In short, re-inventing the wheel.

No, you can do this from within an LSM and the kernel hackers don't have to deal
with it....

(tech note - don't call register_sysctl_table() from within a security_initcall().
Use a separate __initcall() that gets called later - security_initcall() happens
before the kernel has the sysctl infrastructure in place.  Guess how I know that? ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]