Bug #1352

Unlinking objects in a directory with sticky bit set

Added by qhwt+dfly almost 5 years ago. Updated almost 5 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hi.
On HAMMER filesystem, you can remove an object you do not own even
when the directory containing it has sticky bit set:

$ su
# mkdir -m1777 test
# touch test/FOO
# ls -al test
total 0
drwxrwxrwt 1 root wheel 0 May 2 14:17 .
drwxrwxrwt 1 root wheel 0 May 2 14:16 ..
-rw-r--r-- 1 root wheel 0 May 2 14:17 FOO
# exit
$ rm -f test/FOO
$ ls -al test
total 0
drwxrwxrwt 1 root wheel 0 May 2 14:18 .
drwxrwxrwt 1 root wheel 0 May 2 14:16 ..

This is not the case on an UFS filesystem.

History

#1 Updated by dillon almost 5 years ago

:Hi.
:On HAMMER filesystem, you can remove an object you do not own even
:when the directory containing it has sticky bit set:

Oops! I'll get that one fixed today, thanks for the report!

-Matt

#2 Updated by dillon almost 5 years ago

:Hi.
:On HAMMER filesystem, you can remove an object you do not own even
:when the directory containing it has sticky bit set:

Committed as a kernel-layer fix. Please test also, and if it works
as expected I will cherry-pick it over to 2.2.x as well.

-Matt

#3 Updated by qhwt+dfly almost 5 years ago

On Sat, May 02, 2009 at 10:53:31AM -0700, Matthew Dillon wrote:
> :Hi.
> :On HAMMER filesystem, you can remove an object you do not own even
> :when the directory containing it has sticky bit set:
>
> Committed as a kernel-layer fix. Please test also, and if it works
> as expected I will cherry-pick it over to 2.2.x as well.

Quoting from sticky(8):
... A file in a sticky directory may only be removed or renamed
by a user if the user has write permission for the directory and the user
is the owner of the file, the owner of the directory, or the super-user.

So rename(2) in a directory with sticky bit set should also be taken care of.
Currently there are three commits to be cherry-picked to 2.2 branch:
918101d3be679a76c85ea8f47b94e05f8eb11ee5
ee89633d5330733056052c06919a5dd4c01347bc
dae8d54f0708cf191cbb06ef6aa43cd570ceea90

The filesystems affected by ee89633d are: ext2fs, hpfs, msdosfs, ntfs,
nwfs, smbfs, udf, ufs, and HAMMER, but I only tested ufs and HAMMER.

And a very silly nit-pick would be that with these commits the return
code is changed from EPERM to EACCES. I can't think of a situation
when that could be a problem, though.

#4 Updated by dillon almost 5 years ago

:Quoting from sticky(8):
: ... A file in a sticky directory may only be removed or renamed
: by a user if the user has write permission for the directory and the user
: is the owner of the file, the owner of the directory, or the super-user.
:
:So rename(2) in a directory with sticky bit set should also be taken care of.
:Currently there are three commits to be cherry-picked to 2.2 branch:
: 918101d3be679a76c85ea8f47b94e05f8eb11ee5
: ee89633d5330733056052c06919a5dd4c01347bc
: dae8d54f0708cf191cbb06ef6aa43cd570ceea90
:
:The filesystems affected by ee89633d are: ext2fs, hpfs, msdosfs, ntfs,
:nwfs, smbfs, udf, ufs, and HAMMER, but I only tested ufs and HAMMER.

Is rename(2) working properly? I wasn't quite sure from your posting.
It does seem to work properly in my quick test.

:And a very silly nit-pick would be that with these commits the return
:code is changed from EPERM to EACCES. I can't think of a situation
:when that could be a problem, though.
:

I don't mind synchronizing the error code with linux, if someone wants
to figure out what linux returns in these situations.

-Matt
Matthew Dillon
<>

#5 Updated by qhwt+dfly almost 5 years ago

[sorry, I was talking about an unrelated thing here]
:

> Is rename(2) working properly? I wasn't quite sure from your posting.
> It does seem to work properly in my quick test.

The problem is I (as a non-root user) can still rename(2) an object that
I do not own, in a sticky directory on HAMMER filesystem.

On UFS:
# mkdir -m 1777 sticky && touch sticky/ROOT
# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
mv: rename ROOT to R00T: Operation not permitted

On HAMMER:
# mkdir -m 1777 sticky && touch sticky/ROOT
# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
HEY

#6 Updated by dillon almost 5 years ago

:The problem is I (as a non-root user) can still rename(2) an object that
:I do not own, in a sticky directory on HAMMER filesystem.
:
:On UFS:
:# mkdir -m 1777 sticky && touch sticky/ROOT
:# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
:mv: rename ROOT to R00T: Operation not permitted
:
:On HAMMER:
:# mkdir -m 1777 sticky && touch sticky/ROOT
:# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
:HEY

test28# su -m nobody -c 'cd sticky && mv ROOT R00T2 && echo HEY'
mv: rename ROOT to R00T2: No such file or directory
test28#

There may be a short cut for renaming a file to itself, which is
basically a NOP. I'll track down the path.

-Matt
Matthew Dillon
<>

#7 Updated by qhwt+dfly almost 5 years ago

On Sun, May 03, 2009 at 08:27:46PM -0700, Matthew Dillon wrote:
>
> :The problem is I (as a non-root user) can still rename(2) an object that
> :I do not own, in a sticky directory on HAMMER filesystem.
> :
> :On UFS:
> :# mkdir -m 1777 sticky && touch sticky/ROOT
> :# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
> :mv: rename ROOT to R00T: Operation not permitted
> :
> :On HAMMER:
> :# mkdir -m 1777 sticky && touch sticky/ROOT
> :# su -m nobody -c 'cd sticky && mv ROOT R00T && echo HEY'
> :HEY
>
> test28# su -m nobody -c 'cd sticky && mv ROOT R00T2 && echo HEY'
> mv: rename ROOT to R00T2: No such file or directory
> test28#

This is expected, because the original file(ARE OH OH TEE) has been
renamed to (ARE ZERO ZERO TEE). I guess ZEROs and OHs are not very
distictive on your monitor :)

> There may be a short cut for renaming a file to itself, which is
> basically a NOP. I'll track down the path.
>
> -Matt
> Matthew Dillon
> <>

#8 Updated by dillon almost 5 years ago

:This is expected, because the original file(ARE OH OH TEE) has been
:renamed to (ARE ZERO ZERO TEE). I guess ZEROs and OHs are not very
:distictive on your monitor :)

Nope! Ok, you *could* have used something more obvious there :-)

In anycase, I've tracked it down. The rename code was not checking
the sticky bit for the rename target at all, so it happily overwrite
the file owned by someone else. It was doing a VDELETE test for the
rename source (which tests the sticky bit) and a VCREATE test for the
rename target (which is designed for O_CREAT and doesn't deal with
rename's file replacement, so it wasn't). I added a VRENAME and
NLC_RENAME so I could properly code testing of the rename-target.

I'll commit a fix in a few minutes once I finish testing it.

-Matt
Matthew Dillon
<>

#9 Updated by dillon almost 5 years ago

The problem is even worse. rename() doesn't check directory perms
at all for the source directory on a HAMMER filesystem (because HAMMER
depends on the kernel to make the checks and doesn't do them itself).
I blew it.

That's a huge gaping security hole so we are going to have to
roll a 2.2.2 this week to correct it.

-Matt
Matthew Dillon
<>

#10 Updated by qhwt+dfly almost 5 years ago

Fixed by dae8d54f0708 and 945b476ab592

Also available in: Atom PDF