[PATCH 2.6.5-mm4] sys_access race fix

[PATCH 2.6.5-mm4] sys_access race fix

[Fabian Frederick]

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

Andrew,

	I'm trying to remove the race in sys_access code.
AFAICS, fsuid is checked in "permission" level so I pushed real fsuid
capture forward.At that state, I can task_lock (which was impossible
before user_walk).Could you tell me if I can improve this one ?

Regards,
Fabian

[-- Attachment #2: open1.diff --]
[-- Type: text/x-patch, Size: 2191 bytes --]

diff -Naur orig/fs/open.c edited/fs/open.c
--- orig/fs/open.c	2004-04-12 19:52:14.000000000 +0200
+++ edited/fs/open.c	2004-04-13 20:30:07.000000000 +0200
@@ -463,8 +463,8 @@
 
 /*
  * access() needs to use the real uid/gid, not the effective uid/gid.
- * We do this by temporarily clearing all FS-related capabilities and
- * switching the fsuid/fsgid around to the real ones.
+ * We do this under task_lock by temporarily clearing all FS-related 
+ * capabilities and switching the fsuid/fsgid around to the real ones.
  */
 asmlinkage long sys_access(const char __user * filename, int mode)
 {
@@ -476,40 +476,31 @@
 	if (mode & ~S_IRWXO)	/* where's F_OK, X_OK, W_OK, R_OK? */
 		return -EINVAL;
 
-	old_fsuid = current->fsuid;
-	old_fsgid = current->fsgid;
-	old_cap = current->cap_effective;
-
-	current->fsuid = current->uid;
-	current->fsgid = current->gid;
-
-	/*
-	 * Clear the capabilities if we switch to a non-root user
-	 *
-	 * FIXME: There is a race here against sys_capset.  The
-	 * capabilities can change yet we will restore the old
-	 * value below.  We should hold task_capabilities_lock,
-	 * but we cannot because user_path_walk can sleep.
-	 */
-	if (current->uid)
-		cap_clear(current->cap_effective);
-	else
-		current->cap_effective = current->cap_permitted;
-
 	res = __user_walk(filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
 	if (!res) {
+		task_lock(current);
+		old_fsuid = current->fsuid;
+		old_fsgid = current->fsgid;
+		old_cap = current->cap_effective;
+		current->fsuid = current->uid;
+		current->fsgid = current->gid;
+
+		if (current->uid)
+			cap_clear(current->cap_effective);
+		else
+			current->cap_effective = current->cap_permitted;
+
 		res = permission(nd.dentry->d_inode, mode, &nd);
 		/* SuS v2 requires we report a read only fs too */
 		if(!res && (mode & S_IWOTH) && IS_RDONLY(nd.dentry->d_inode)
 		   && !special_file(nd.dentry->d_inode->i_mode))
 			res = -EROFS;
 		path_release(&nd);
+		current->fsuid = old_fsuid;
+		current->fsgid = old_fsgid;
+		current->cap_effective = old_cap;
+		task_unlock(current);
 	}
-
-	current->fsuid = old_fsuid;
-	current->fsgid = old_fsgid;
-	current->cap_effective = old_cap;
-
 	return res;
 }
 

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Chris Wright]

* Fabian Frederick (Fabian.Frederick@skynet.be) wrote:
> Andrew,
> 
> 	I'm trying to remove the race in sys_access code.
> AFAICS, fsuid is checked in "permission" level so I pushed real fsuid
> capture forward.At that state, I can task_lock (which was impossible
> before user_walk).Could you tell me if I can improve this one ?

This changes the semantics of the directory checks implicit
during the pathname resolution.

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

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Fabian Frederick]

On Tue, 2004-04-13 at 20:50, Chris Wright wrote:
> * Fabian Frederick (Fabian.Frederick@skynet.be) wrote:
> > Andrew,
> > 
> > 	I'm trying to remove the race in sys_access code.
> > AFAICS, fsuid is checked in "permission" level so I pushed real
fsuid
> > capture forward.At that state, I can task_lock (which was impossible
> > before user_walk).Could you tell me if I can improve this one ?
> 
> This changes the semantics of the directory checks implicit
> during the pathname resolution.

Well, the only major function behind user_walk is path_lookup.
This one has some calls with the nameidata.Other process seems
current->fs->xxx relevant read-only.Maybe you mean the
read_lock(&current->fs->lock) which could bring a deadlock as we
task->lock before ? 

If user_walk had to run in ruid, why would we have permission() then ?

Regards,
Fabian

> thanks,
> -chris

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Chris Wright]

* Fabian Frederick (Fabian.Frederick@skynet.be) wrote:
> Well, the only major function behind user_walk is path_lookup.
> This one has some calls with the nameidata.Other process seems
> current->fs->xxx relevant read-only.Maybe you mean the
> read_lock(&current->fs->lock) which could bring a deadlock as we
> task->lock before ? 

No, point is simply that there's implicit permission check in
__user_walk().

> If user_walk had to run in ruid, why would we have permission() then ?

It's how the standards require the call to be implemented.  The
access(2) check verifies access to the pathname using the ruid not euid.
Part of valid access includes search access on the directory elements of
the full pathname.  Those tests are done during __user_walk.

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

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Andrew Morton]

Fabian Frederick <Fabian.Frederick@skynet.be> wrote:
>
> 
>  	I'm trying to remove the race in sys_access code.
>  AFAICS, fsuid is checked in "permission" level so I pushed real fsuid
>  capture forward.

Do races in access() actually matter?  I mean, some other process could
change things a nanosecond after access() has completed and the value which
the access() caller received is wrong anyway.

Or is there some deeper problem which you are addressing here?

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Do races in access() actually matter?  I mean, some other process could
> change things a nanosecond after access() has completed and the value which
> the access() caller received is wrong anyway.
> 
> Or is there some deeper problem which you are addressing here?

There is a race where, the saved off capabilities could blow away recently
updated capabilites when they are restored.  But, it's only raceable
against tasks that have SETPCAP and are setting another task's caps.
Otherwise it's serialised by the fact that we're dealing with a single
task that can only be in one syscall at a time.  Fixing it would require
something like passing creds into the permission function, instead of
them being deduced from current, a rather invasive change.

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

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Jamie Lokier]

Andrew Morton wrote:
> Do races in access() actually matter?  I mean, some other process could
> change things a nanosecond after access() has completed and the value which
> the access() caller received is wrong anyway.

What about the effect access() has on other callers?  It temporarily
changes current->fsuid, so fs operations in other completely
independent threads are affected.

-- Jamie

Re: [PATCH 2.6.5-mm4] sys_access race fix

[viro]

On Wed, Apr 14, 2004 at 08:11:02AM +0100, Jamie Lokier wrote:
> Andrew Morton wrote:
> > Do races in access() actually matter?  I mean, some other process could
> > change things a nanosecond after access() has completed and the value which
> > the access() caller received is wrong anyway.
> 
> What about the effect access() has on other callers?  It temporarily
> changes current->fsuid, so fs operations in other completely
> independent threads are affected.

Huh?  What does changing a field of tast_struct have to do with behaviour
of some other threads?

Re: [PATCH 2.6.5-mm4] sys_access race fix

[Jamie Lokier]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> Huh?  What does changing a field of tast_struct have to do with behaviour
> of some other threads?

It has a lot to do with a brain malfunction at my end, and nothing to
do with anything else.  Enjoy the embarrassment,

-- Jamie

Re: [PATCH 2.6.5-mm4] sys_access race fix

[fabian.frederick]

Hi,

     It seems this thread has lost its initial goal.We can't do uid changes without lock as reported in current open.c doc.Maybe there's some expert here able to tell me exactly what's wrong in this ?

http://fabian.unixtech.be/kernel/open1.diff

And most of all patch this patch as well ;)

Regards,
Fabian

___________________________________