chmod is broken for non-superuser

[grahamh]

Description

Permissions checking doesn't produce correct POSIX behaviour for chmod (and probably chown, etc). This won't be noticed from the desktop, as Haiku's default user is a superuser. However, when you try to run something like a CGI script from apache, which needs to run as a non-privileged user, it can lose control of files it creates. For example, this causes a Foswiki install to break.

How to reproduce

/data> uname -a
Haiku shredder 1 r39138 Oct 25 2010 07:43:53 BePC Haiku

# make a directory
/data> mkdir j

# and set it to be owned by user "sshd"
/data> chown sshd:users j

# log in as "sshd"
/data> su -l -s bash sshd

Welcome to the Haiku shell.

# go back to the directory, and make a file
/var/empty> cd /data/j
/data/j> touch file
/data/j> ls -l file
-rw-r--r-- 1 sshd users 0 Oct 25 21:38 file

# set it to read-only
/data/j> chmod 444 file
/data/j> ls -l file
-r--r--r-- 1 sshd users 0 Oct 25 21:38 file

# now set it back to writable
/data/j> chmod 644 file
chmod: changing permissions of `file': Operation not allowed

# uh-oh...

Analysis

The bfs add-on has a single CheckPermissions function (part of the Inode object), which is supposed to decide whether any given operation that the VFS layer has passed in is allowed by the inode's read-write flags. It's called from a number of hooks in kernel_interface.cpp, including the bfs_write_stat hook which implements chmod (and several other things). However, chmod() is a special case for access permissions compared to other hooks such as open(). POSIX states that a file's owner should always be able to set it back to writable - otherwise it's possible to put a file in a state where you can no longer use it.

Note: the same issue is present in ext2fs, and possibly others.

Suggested fix

The following patch makes it work OK for me (for BFS), although I doubt it conforms to any style guides :-)

Index: Inode.cpp
===================================================================
--- Inode.cpp	(revision 38949)
+++ Inode.cpp	(working copy)
@@ -487,7 +487,7 @@
 
 
 status_t
-Inode::CheckPermissions(int accessMode) const
+Inode::CheckPermissions(int accessMode, int write_stat /* optional */) const
 {
 	uid_t user = geteuid();
 	gid_t group = getegid();
@@ -505,6 +505,11 @@
 		}
 	}
 
+	// users are always allowed to chmod() files they own
+	if (write_stat && user == (uid_t)fNode.UserID()) {
+		return B_OK;
+	}
+
 	// shift mode bits, to check directly against accessMode
 	mode_t mode = Mode();
 	if (user == (uid_t)fNode.UserID())
Index: kernel_interface.cpp
===================================================================
--- kernel_interface.cpp	(revision 38949)
+++ kernel_interface.cpp	(working copy)
@@ -777,7 +777,14 @@
 	// TODO: we should definitely check a bit more if the new stats are
 	//	valid - or even better, the VFS should check this before calling us
 
-	status_t status = inode->CheckPermissions(W_OK);
+	status_t status;
+	// chmod, chown need to allow a file's owner to change its
+	// permissions even if it's set to read-only
+	const uint32 ownership = B_STAT_MODE | B_STAT_GID | B_STAT_UID;
+	if ((mask & ownership) && !(mask & ~ownership))
+		status = inode->CheckPermissions(W_OK, true);
+	else
+		status = inode->CheckPermissions(W_OK);
 	if (status < B_OK)
 		RETURN_ERROR(status);
 
Index: Inode.h
===================================================================
--- Inode.h	(revision 38949)
+++ Inode.h	(working copy)
@@ -99,7 +99,7 @@
 
 			Volume*				GetVolume() const { return fVolume; }
 
-			status_t			CheckPermissions(int accessMode) const;
+			status_t			CheckPermissions(int accessMode, int write_stat = false) const;
 
 			// small_data access methods
 			small_data*			FindSmallData(const bfs_inode* node,

[axeld]

Your concerns about the coding style are definitely justified ;-)

Anyway, thanks for looking into this. The way you changed bfs_write_stat() doesn't make any sense to me, though. Why should you have different rights depending on what you change? The way it's done now, you actually get a permission boost when doing ownership related changes.

If those actually need different access rights, they should be separated from the other changes. I guess that would need some more investigations, though.

[grahamh]

Problem is that CheckPermissions is used for lots of different things. If a file is mode 444 (r--r--r--), then, for example,

• You shouldn't be able to open() it with O_RDWR
• You shouldn't be able to unlink() it
• You shouldn't be able to truncate() it
• You shouldn't be able to change the last modified date

(all of these call CheckPermissions, and I think the last two are done by bfs_write_stat)

but...

• You should be able to chmod() it
• You should be able to chown() it (I think - the POSIX spec is a bit vague on that)

and both of these are also done in bfs_write stat. Which is why I think it needs the escalation of permissions when doing B_STAT_MODE, B_STAT_GID or B_STAT_UID, but should throw EACCES for other things like B_STAT_SIZE.

[axeld]

The last two are indeed done by fs_write_stat(). In any case, this would still call for a solution like:

if CheckPermissions(..., true)
    update ownership info if requested
else
   return B_NOT_ALLOWED

if CheckPermissions(..., false)
    perform the rest of the request changes
else if requested-non-ownership-changes
    return B_NOT_ALLOWED

The way you suggested in your patch, changing ugo+r file to ugo+rx while truncating it in a single fs_write_stat() would actually succeed, but that should obviously not be allowed.

Maybe have a CheckPermissions() version that takes the stat modifier bit, ie. like this:

if ((mode & B_STAT_MODE) != 0) {
    status_t status = CheckPermissions(W_OK, B_STAT_MODE);
    if (status != B_OK)
        return status;

    ...
}
if ((mode & B_STAT_GID) != 0) {
    status_t status = CheckPermissions(W_OK, B_STAT_GID);
    ...
}

[grahamh]

The change and truncate together would be stopped by the " ! (mask & ~ownership)" clause, which looks for any bits set that aren't related to chmod/chown.

But I think your alternative CheckPermissions with the flag supplied is a better way.

[bonefish]

JFYI, I believe I managed to implement it correctly in ​checksumfs.

[rohityadav]

Hi, patches attached for fixing the issue in BFS and ext2 file systems. Tested and works for me.

[axeld]

Replying to bonefish:

JFYI, I believe I managed to implement it correctly in ​checksumfs.

Looks indeed nice. Though I think I've spotted a bug: in line 1439 you claim that the transaction is started, and the node added to it. In fact, the node is never added.

[axeld]

Replying to rohityadav:

Hi, patches attached for fixing the issue in BFS and ext2 file systems. Tested and works for me.

Thanks for the patches! However, you just moved grahamh's change into CheckPermissions() - this does still have the same issues of not allowing certain actions to happen at a time. What I suggested instead was pretty much how Ingo solved it in his corruptionfs.

Besides that, there are indeed a few coding style problems:

• no superfluous parenthesis (ie. around (user == (uid_t)node.UserID())).
• use (a & b) != 0 instead of just (a & b).
• operators always go to the next line (the &&).

Would be great if you could update your patch!

[rohityadav]

Replying to axeld:

Thanks for the patches! However, you just moved grahamh's change into CheckPermissions() - this does still have the same issues of not allowing certain actions to happen at a time. What I suggested instead was pretty much how Ingo solved it in his corruptionfs.

Hi again Axel, okay... so I spend a couple of hours to move a lot of code to something like Ingo did, but finally I thought moving and refactoring grahamh's code was the simplest thing, so I did. Besides, it make CheckPermissions powerful (i think, not sure :). Anyway I'll correct 'em as you suggested... So, my question is do you want me to follow Ingo's solution as in corruptionfs? I'll send the corrections right away.

[axeld]

I would still use CheckPermissions(), I would just call them one by one similar to what Ingo did. But it's your decision.

[rohityadav]

Okay, I've attached newer patch for BFS, which contains parts of code copied/forked from Ingo's solution. It works for me, but still you may want to check and review it. After that I'll do the same for ext2.

[axeld]

This is not really what I meant with regards to the CheckPermissions() usage; since you aren't using it as I originally suggested, it doesn't seem to make any sense to use it at all for this; "isOwnerOrRoot = uid == 0 || uid == node->UID()" is at least way clearer.

I would just remove the statMask from CheckPermissions() again, and make this pretty much a copy of how Ingo solved it, then. If you're still motivated, go ahead, otherwise I could do the changes to your patch as well :-)

[rohityadav]

Fixes chmod issue in BFS (new)

[rohityadav]

Fixes chmod issue in ext2fs (new)

[rohityadav]

Updated! Hope the patches are nicer now, enough to be committed soon :)

[korli]

Could we have this code in a common place? It's likely to be the same for most file systems.

[grahamh]

Need to be a bit careful if making this kind of thing common. Most filesystems will need to do this, but not all of them. (For example I don't think that MSDOS filesystems have a concept of "file owner".) I would guess that's why it's not done in the VFS layer?

[bonefish]

Replying to grahamh:

Need to be a bit careful if making this kind of thing common. Most filesystems will need to do this, but not all of them. (For example I don't think that MSDOS filesystems have a concept of "file owner".) I would guess that's why it's not done in the VFS layer?

File systems that don't have those features should simply pretend they do and emulate the behavior accordingly. The main reasons for leaving permission checks to the file system implementation are:

1. Performance. The VFS could use the read_stat() and/or access() hooks to do that before invoking the FS hook that performs the actual operation, but that would have more overhead than the FS doing it.
2. Atomicity (more importantly). Only the FS can make sure the check and the actual operation are performed atomically, since the locks for the volume/node are not known outside the FS implementation.

[axeld]

Applied in hrev39378, and hrev39379, thanks a lot Rohit Yadav!

To Ingo: the check_access() a.k.a. CheckPermissions() function could easily be a VFS delivered utility function. BTW there is at least one functional difference between the previous version in BFS: before, root could always enter a directory, no matter if any X was set. In your version, that is no longer possible; I would assume the former version is the correct one, though.

[bonefish]

Replying to axeld:

Applied in hrev39378, and hrev39379, thanks a lot Rohit Yadav!

To Ingo: the check_access() a.k.a. CheckPermissions() function could easily be a VFS delivered utility function.

Sure.

BTW there is at least one functional difference between the previous version in BFS: before, root could always enter a directory, no matter if any X was set. In your version, that is no longer possible; I would assume the former version is the correct one, though.

Possibly. At least it's what one might expect and what e.g. Linux does. But ​POSIX says:

"If a process has appropriate privileges: [...] If execute permission is requested, access shall be granted if execute permission is granted to at least one user by the file permission bits or by an alternate access control mechanism; otherwise, access shall be denied."

At least if one ties directory entry resolution to the execute permission, it would be correct to also deny root the access when none of the X bits is set.

Anyway, I only claimed that I got the permission handling in write_stat() right, not everywhere. :-) E.g. access() is wrong for sure as the standard requires to use the real user and group IDs for the check. If we want to support the faccessat() AT_EACCESS flag, we have to change the hook signature anyway, though (e.g. pass the user and group IDs).