Bug 633544 (CVE-2011-1011)

Just to summarize the situation as I see it here.  This is believed to be fixed now in the recent policycoreutils versions, such as policycoreutils-2.0.83-28.fc13, via this patch:

http://pkgs.fedoraproject.org/gitweb/?p=policycoreutils.git;a=blob;f=policycoreutils-rhat.patch;h=d4db5bc06027de23d12a4b3f18fa6f9b1517df27;hb=HEAD#l2197

It extends seunshare_mount() to mount with nosuid/nodev/noexec (this does not seem to make a difference for the problem reported by Tavis) and forces sticky bit on the /tmp directory upon mount.  However, there does not seem to be any change to prevent user from removing sticky bit right after it's added, and the sticky bit does not prevent non-privileged user from removing file from a directory he or she owns.

AFAIK, Tavis proposed two possible fixes:
- restrict seunshare to a dedicated group
- create temporary directory in some safe location and with safe ownership and permissions (which include sticky bit)

Note: Allowing user to specify arbitrary directory to bind-mount can also allow bypassing certain filesystem permissions restrictions (getting access to the directory that is not normally accessible to the user), but verify_mount() check makes this bypass fairly restricted and uninteresting.

I guess the only solution to this would be to force the tmp directory to be either a tmpfs directory or on /tmp it self.  Would that satisfy the problem?

Wouldn't the use of /tmp itself beat the purpose of seunshare-ing /tmp?  It also can not be done with current seunshare due to the verify_mount() ownership check:

$ seunshare -t /tmp -h $HOME /bin/bash
Invalid mount point, reporting to administrator: Operation not permitted

tmpfs should provide an easy way to have a run-specific directory that can be trashed as the end of the session easily, at the cost of memory use.  What is more important for intended sandbox use - have concurrently running instances not share the same directory as /tmp, or have /tmp content persist across sandbox runs?

No I was talking about seunshare enforcing that the user say

seunshare -t /tmp/mytmp -h $HOME/bin/bash

Meaning the user has to create the directory within /tmp to mount over /tmp.

I think we should add tmpfs support, but I can see a use case for users having persistance.  (Of course this could be argued against.)

(In reply to comment #12)
> No I was talking about seunshare enforcing that the user say
> 
> seunshare -t /tmp/mytmp -h $HOME/bin/bash
> 
> Meaning the user has to create the directory within /tmp to mount over /tmp.

You do not want mytmp to be user-owned, otherwise your new /tmp will be user-owned.

So Tomas you think the only valid/safe thing to do is to use tmpfs?

Not really.  The approach Tavis proposed - to make seunshare create root-owned directory with safe permissions - should work as well.

Just a quick summary of the options, with some pros and cons listed:

- /var/sandbox/tmp-$USER, root:$USER, 1770
  - pro: directory to store per-user tmp directories is not world-writeable,
    hence it's easier to create per-user directories safely
  - pro: persistence
  - con: same directory shared between concurrent sandbox runs
  - con: another user-writeable directory (in addition to expected things as
    /home, /tmp, /var/tmp)

- the above can be changed to use existing temporary directory instead of /var/sandbox, which avoids unexpected user-writeable directory, but needs more sanity checks, other pros/cons are similar to the above; this can be "DoS"-ed (one unprivileged user pre-created tmp-$USER for some other user (when using /tmp/sandbox-tmp-$USER) or all users (when using /tmp/sandbox/tmp-$USER))

- per-run temporary directory similar to current /tmp/.sandboxXXXXXXXX,
  root:$USER, 1770 permissions
  - pro: new per-run temporary directory
  - pro: possible "keep after run" persistence
  - con: more work to remove it safely after run

- tmpfs use should be the variant of the previous, without "keep after run" persistence, but with easier clean-up

Neither of these preserves the flexibility of the current version, so we should pick the approach that is the most aligned with sandbox's intended use case.

Dan, does any of the above option looks preferred wrt existing seunshare uses?

I looked into using tmpfs rather then a directory the user controls, but that would break one of the use cases.   I have firefox setup to execute sandbox everytime it downloads a pdf or .doc file, but if it is setup to download to /tmp, this would break using tmpfs.

sandbox creates NEWHOME and NEWTMP and then copies all files in the command path to those directories.

sandbox -X evince /tmp/mydoc.pdf 

Copies /tmp/mydoc.pdf to NEWTMP and then mounts NEWTMP over /tmp

What about if we chown root:root NEWTMP and then mount.  

chown UID:UID NEWTMP 

when done.

(In reply to comment #18)
> What about if we chown root:root NEWTMP and then mount.  
> 
> chown UID:UID NEWTMP 
> 
> when done.

Does "when done" refer to the end of seunshare run?  That may only work if there's a reliable way to delay that ownership restore (hence seunshare exit) until after all processes that may have been started from the process executed by seunshare exited.  seunshare currently does not ensure that, it exits once its child process terminates.  So the plain chmod back after waitpid won't help.

I see there's no clean up code in the current seunshare version at all.  Is it not needed to clean up the private mounts?  Does kernel do that automatically once no process that can see them exists?

Yes.  Umount and destruction of the share happens automatically.  Not sure if there is a way for seunshare to know if all of the processes are exited.

One observation is that you can cd to /proc/<seunshare child pid>/root and check ./tmp there to see if it still has NEWTMP mounted.  Running on two terminals (noted as [1] and [2] below):

[1] ~ $ ls -lid /tmp /tmp/test-sandbox
65025 drwxrwxrwt. 32 root root 12288 Nov 24 14:41 /tmp
97564 drwxr-xr-x.  2 test test  4096 Nov 24 14:41 /tmp/test-sandbox

[1] ~ $ seunshare -v -t /tmp/test-sandbox -h $HOME /bin/bash
Mount /home/test on /home/test
Mount /tmp/test-sandbox on /tmp

[1-seunshare] ~ $ echo $$
5940

[2] ~ $ cd /proc/5940/root/

[2] /proc/5940/root $ ls -lid ./tmp/
97564 drwxr-xr-t. 2 test test 4096 Nov 24 14:41 ./tmp/

[1-seunshare] ~ $ cat blah.sh 
#!/bin/sh
echo "Sleeping for $1 sec"
sleep $1
echo "Done with $1 sec sleep"

[1-seunshare] ~ $ ./blah.sh 60 & ./blah.sh 90 & ./blah.sh 120 &
[1] 6000
[2] 6001
[3] 6002

[1-seunshare] ~ $ exit               

[1] ~ $ ls -lid /proc/5940/root/tmp
ls: cannot access /proc/5940/root/tmp: No such file or directory

while [2] still sees it's root:

[2] /proc/5940/root $ ls -lid ./tmp/
97564 drwxr-xr-t. 2 test test 4096 Nov 24 14:41 ./tmp/

Once all processes are done:

[2] /proc/5940/root $ ls -lid ./tmp/
65025 drwxrwxrwt. 32 root root 12288 Nov 24 14:52 ./tmp/


Still chowning arbitrary user directory to root may prove problematic.

Interesting, not sure if this is a problem or not.  Sandbox sends a signal to all of its subprocess to try to get them to exit.

The major sticking point then is trying to figure a way to update the contents of /tmp with files if it was a temporary file system.

One potential solution would be to create the tmp directory within the $HOME and then cp the contents over the tmpfs once it is created.

Or only allow certain path to be used as NEWTMP, such as /var/tmp/<something>?

How does that help?

If using the chowning approach (chown to root:user at the beginning of the seunshare run and chown back once all child processes exited), this will limit which directories can get their ownership changed.  Also limiting where stale / left-over directories can be if seunshare is terminated before it can chown directory back.

is there any update on this issue, did you make a decision? (the solution I prefer is forcing the /tmp directory to be /var/sandbox/tmp-$USER root:root and 1777, and making /var/sandbox 700 root:root, I think this will work?)

I would consider this bug fairly urgent, I admit I dont have a working exploit to turn this into a root shell, but a quick search of suid binaries available shows that at least ksu assumes /tmp is sticky.

Right now an unprivileged user can destroy a system like this:

$ seunshare -v -t /tmp/seunshare/ -h /tmp/seunshare/ -- `which ksu` root
Mount /tmp/seunshare/ on /home/taviso
Mount /tmp/seunshare/ on /tmp
WARNING: Your password may be exposed if you enter it here and are logged 
         in remotely using an unsecure (non-encrypted) channel. 
Kerberos password for root: : 
[1]+  Stopped                 seunshare -v -t /tmp/seunshare/ -h /tmp/seunshare/ -- `which ksu` root
$ ls -l /tmp/seunshare/
total 4.0K
-rw-------. 1 root taviso 35 Feb 18 23:21 krb5cc_0.1
$ rm -f /tmp/seunshare/krb5cc_0.1 
$ ln /etc/passwd /tmp/seunshare/krb5cc_0.1
$ fg
seunshare -v -t /tmp/seunshare/ -h /tmp/seunshare/ -- `which ksu` root

ksu: Cannot read password while reading password for 'root'

Authentication failed.

...and /etc/passwd just got nuked. Do you mind if I publish it so that administrators can remove seunshare if they're concerned about privilege escalation?

If I don't receive a response by Monday, I'll assume you don't mind.

Dan, which of the current use cases are broken or negatively impacted by the proposed single NEWTMP for all seunchare runs approach?  Some pros/cons mentioned above.  I should probably add some issues with the first run, when user-specific sandbox-tmp directory has not been created yet.

Tavis, how about we just go with removing -t PATH and just using -t, which will mount /dev/shm as 1777 root:root.

sandbox -X evince /tmp/download.pdf

Needs to work.

So the way this works now is, sandbox command creates the tmpdir, and then copies the content into it, then seunshare mounts the tmpdir over /tmp/

This will not work on tmpfs since the tmpdir will not exist until after seunshare executes.  

I could change the code to specify a tmpdir and have seunshare create its own tmpdir or tmpfs and copy the files from the local tmpdir into the tmpfs.

Would this satisfy the needs.

Yes, that sounds okay to me so long as seunshare creates it.

Tavis, it is a little more difficult, but I have tried to recreate the vulnerabilty and have failed.  Are you saying you ksu without the root password and desctroyed the /etc/passwd file or are you saying your ksu was successful and destroyed the file?

Yes, ksu without the password destroys the file.

I confirm that ksu steps above work on EL6 with default krb5 config.

Tomas, I'd like to publish a warning about this, would you prefer I wait? (I don't mind waiting if you guys are planning to work on this quickly). However if not much progress has been made since September, I think it's fairer to administrators to publish a warning asap.

Even if the worst (that I can demonstrate) is destroying a system, I think it's a reasonable assumption that there is a configuration/package/customer software that relies on the stickyness of /tmp (perfectly reasonably) to prevent arbitrary code execution.

Tavis.

Well, I waited an extra day because of the bank holiday in america, but there have been no objections, so I'm sending an advisory now.

Hey Tavis, thanks.

http://lists.grok.org.uk/pipermail/full-disclosure/2011-February/079490.html

Created attachment 480587 [details]
Rewrote seunshare.c to create tmpfs /tmp and label it correctly.

The program then copies the original /tmp files into the tmpfs.

One restriction on this version is that neither the home or tmp directories can actually be in /tmp.

Maybe i'm reading it wrong, but you're running system() with euid=0?

If so, there is a command injection by specifying a temporary directory called `id` or something like that, as well as problems with inheriting an untrusted environment.

Tavis, I think the getfilecon() would have failed on a command injection. 

However, I think if you go this way, you want to validate that its a directory but you might be subject to a TOCTOU race. You might need to hand copy all files using the openat, mkdirat, fchmodat, fchownat syscalls.

Unless I am missing something, the current patch might allow concurrent use in which case the user will be consuming system memory with the tmpfs.

Also, if you have a malicious app that forks and calls setsid and tries to escape, will it be left holding resources when seunshare dies?

Steve: Why do you say that? `id` is a valid filename. I tested it, and it works.

Regardless, running system() with euid != uid is hard to get right due to the untrusted environment.

bash tries to drop privs in such case.  It does not check set[ug]id return value, it seems, but the easy nproc limit trick to make that fail is not applicable here.

seunshare also got setfsuid call (too) early on, so it always fails on setfilecon("/tmp").

(In reply to comment #40)
> Also, if you have a malicious app that forks and calls setsid and tries to
> escape, will it be left holding resources when seunshare dies?

Yes.  They are also not well visible, as those private mounts don't show up in /proc/mounts.

Created attachment 480816 [details]
Here is a patch to the original seunshare to fix some of the problems found

One it now opens the mntdir and uses the fd to checking access.
tmpdir is no longer an option for seunshare, if there is a subdir of the homedir named .sandboxtmp, then the contents of this directory will be copied to the /tmp directory after dropping capabilities.

Moved the setfsuid call lower in the program.  While still not checking if it was successful, code below will fail on NFS homedirs if it was not successful.

I also moved the large cgroups code block into its own function.

I am setting the context on the /tmp directory based on the context of the homedir after it was mounted.  

I also added a mount bind of /tmp on /var/tmp to allow the sandbox to write to /tmp and /var/tmp.

Created attachment 480817 [details]
Updated code

I looked at the code, there's a TOCTOU with verify mount I think.

You do verify_mount(); then mount(); so you can do something like:

# Create a legal mount directory
mkdir /home/foo/seunshare/bin

# give it to seunshare
seunshare -h ~/seunshare/bin &

# between the verify() and the mount(), do:
$ rm -rf ~/seunshare; ln -s / ~/seunshare

If you win the TOCTOU, you get mount("/home/foo/seunshare/bin", "/home"), which is mount("/bin", "/home"). I'm not sure if that can be exploited, but it's worrying.

We also verify the mount after it is done, so you can not replace the mount with something now owned by the user.

I tested it by adding a sleep() in verify_mount., it results in a weird state.

63 61 253:3 /taviso/testing/bin//deleted /home/taviso\040(deleted) rw,relatime - ext4 /dev/mapper/vg_thinkstation-lv_home rw,seclabel,barrier=1,data=ordered

And the second verify_mount does not fail.

Yes, you're right, just stating that it's scary that the mount will succeed.

I do not have any specific attack against it.

We new about this race, but not much we can do about it other then verify the final directory is owned by the user.  Since fmount(fd) does not exist.

As far as the tmpfs wasting resources comments from Steve, I think we should separate them out.  

sandbox is intended to be run multiple times.  Every pdf/.doc file I download from the internet I run a sandbox on.  In the future I could think of people running sandboxes on different firefox instances.  I could even see people using sandbox for MLS desktops, rather then XACE.

(In reply to comment #45)
> Created attachment 480817 [details]
> Updated code

Needs an obvious fix for the asprintf line to compile.

Does it really need to restrict tmpdir to ~/.sandboxtmp?  Such restriction should also make verify_mount's strncmp(mntdir, "/tmp", 4) check redundant, as home is mounted before tmp.

(In reply to comment #51)
> As far as the tmpfs wasting resources comments from Steve, I think we should
> separate them out.  
> 
> sandbox is intended to be run multiple times.  Every pdf/.doc file I download
> from the internet I run a sandbox on.  In the future I could think of people
> running sandboxes on different firefox instances.

sandbox randomizes categories range when generating contexts for the process to be run inside the sandbox.  If you currently run two concurrent sandbox instances with the same tmpdir, the last one's context is set on /tmp, making it unavailable to other instances.

Any suggestion on how to avoid memory hogging via seunshare?  tmpfs default size is 50% of RAM, hence 6 instances should allow user to hog all memory on systems that follow old SWAP = 2 * RAM recommendation.  That still doesn't sound good for something that's installed by default and hard to remove.

I think we still need to make sure the user does not specify /tmp for the homedir, because /tmp will be mounted over.  Not sure if things would work correctly if /tmp/homedir is bind mounted on ~/ and then /tmp is mounted on with a tmpfs.  I guess we could hack this out and see if it works.

Please provide the asprintf fix needed for compile, since this is compiling on my box.

Mount options for tmpfs
       size=nbytes
              Override  default  maximum  size of the filesystem.  The size is
              given in bytes, and rounded up to entire pages.  The default  is
              half  of  the memory. The size parameter also accepts a suffix %
              to limit this tmpfs instance to that percentage of your physical
              RAM:  the default, when neither size nor nr_blocks is specified,
              is size=50%


We could give a number here.  

Steve has suggested that we have a config file for seunshare that can set limits, maybe even allow admins to specify which users are allowed to use it.  Secondly we could write to /var/run/seunshare/user and record the number of sandboxes that are running.  Then the admin could configure /etc/sandbox.conf with a max number of sandboxes.

max_tmp=1%
max_sessions=20
users=*
or 
users=dwalsh
users=

(In reply to comment #53)
> I think we still need to make sure the user does not specify /tmp for the
> homedir, because /tmp will be mounted over.

The same problem should affect released seunshare versions, right?  They have no problem with use as like: seunshare -h /tmp/a/home -t /tmp/a/tmp command

> Please provide the asprintf fix needed for compile, since this is compiling on
> my box.

Attached version contains:

    char *cpbuf=NULL;
    if (asprintf(&buf, "/bin/cp -RTp %s/.sandboxtmp /tmp 2> /dev/null", pwd->pw_dir) < 0) {
        fprintf(stderr, _("Failed to allocate copy command: %s\n"), strerror(errno));
        return -1;
    }

    rc = system(cpbuf);

and fails to compile with:

  seunshare.c:549: error: ‘buf’ undeclared (first use in this function)

I suspect you already did "asprintf(&buf" -> "asprintf(&cpbuf" change after attaching code to bugzilla.

> Steve has suggested that we have a config file for seunshare that can set
> limits, maybe even allow admins to specify which users are allowed to use it. 
> Secondly we could write to /var/run/seunshare/user and record the number of
> sandboxes that are running.  Then the admin could configure /etc/sandbox.conf
> with a max number of sandboxes.
> 
> max_tmp=1%
> max_sessions=20

I agree with max_tmp, but we've already discussed that session tracking needs more work as we can not decrement active session counter after wait()ing for the seunshare's child to exit.  With the robust session tracking in use, is tmpfs still attractive compared to chmoding/chowning directory at the beginning and at the end?


That setfilecon still fails for me, as it's reached after setfsuid call, and uncovers this use-after-free bug:

        rc =setfilecon("/tmp", filecon);
        freecon(filecon);
        if (rc < 0) {
            fprintf(stderr, _("Failed to set context %s on /tmp: %s\n"), filecon, strerror(errno));

seunshare -h /tmp/home -t /tmp/home/tmp command
seunshare -h /tmp/tmp/home -t /tmp/tmp command 

Is allowed in the release version with some hacky checks for overlaps.

Ok, yes I have fixed the code.  Sorry about that.

How about we dump tmpfs and go with making a tmpdir in /tmp and mount bind over /tmp.  Then I don't need to worry about memory use.  

Not sure why setfilecon works for me and not for you.

Might have something to do with the file system used for /tmp?

I could attempt to do the setfilecon before the setfsuid, but on a rootsquash file system it will not be able to get the file context of the user directory.

(In reply to comment #55)
> How about we dump tmpfs and go with making a tmpdir in /tmp and mount bind over
> /tmp.  Then I don't need to worry about memory use.  

What's the tmpdir above?  A directory passed to seunshare as -t argument?  This should work too, if the directory is chowned/chmoded at the beginning properly.  It can't be chowned back seunshare exit though, so sandbox won't have privileges to remove it.  It can remove most of its content though (in most cases, only root:$USER owned directory is expected to be left around), and tmpwatch should take care of the rest over the time.

This can be combined with some of the ideas from the version using tmpfs.  seunshare can accept arbitrary tmpdir as its argument.  It will create new temporary directory with proper permissions in /tmp, copy/move files to that new temporary directory and bind mount it.  At the end of run, move temporary files back to the original tmpdir (to provide persistence) and remove directory that was bind mounted.  Moving should be done with user privileges, delete with root privs, possibly reusing some of the tmpwatch code to do it safely.

Removing usable /tmp from the sandboxed processes that are running after seunshare's termination is not a concern, I believe.

Created attachment 482122 [details]
Ok this version allocates a directory in /tmp an labels it correctly

It then rsyncs the homedir/.sandboxtmp direcory to /tmp and when it is done rsync's it back


The reason it has to be a subdir of homedir, is we want to drop capabilities before doing any of the "rsyncing".  Since it mounts the tmpdir and the homedir, then drops capabilities, if the /tmp contents are not a subdir of homedir, I can no longer get to the contents.


This seems to work.  Since the /tmp is a subdir of the real /tmp, we end up using the same file system, and quota should work fine.  The only problem I see is around leaving the tmpdirectory in /tmp.  We can not delete it since we have dropped capabilities,  I attempt to remove all files from the directory.  This means we need to wait for tmpreaper to clean up the directories.

Tomas has suggested on line that I change the tmpdir to be 

root,dwalsh,1770.

Tavis do you think there is risk in this?

I made this change and created a file owned by root in my /tmp directory,  As dwalsh I tried to delete the file and got.

rm dan
rm: remove write-protected regular empty file `dan'? y
rm: cannot remove `dan': Operation not permitted

This seems like a better solution since a different user could stick a file into my tmpdir with the current patch.

I think that sounds okay, but as Tomas says 1770 is definitely the right permissions.

Created attachment 482153 [details]
Yet another version....  This one cleans up after itself.

seunshare now does

seteuid(root)
create the tmpdir,
forks,
Parent drops_capabilities with euid == 0,
waits for the child pid to exit,

	childprocess
        unshares,
        mounts,
        drops_capabilities and
	setresuid(dwalsh)
	rsync ~/.sandboxtmp to /tmp
	fork()
		Child execs program
	Parent waits for child to exit
	rsync /tmp to ~/.sandboxtmp
rmdir(tmpdir)

I guess it will be okay, but only because you can't specify a subdirectory. Otherwise because ruid matches you can STOP it, wait for tmpwatch to clean everything up, then make a symlink.

I think it's a scary design. Is /var/sandbox/tmp-xxxxx with /var/sandbox 0700, not a possibility?

Tavis.

rmdir will not follow a symlink.   I am not doing a full cleanup.  just rmdir(tmpdir),  Which will fail if it is anything other then a directory.

Why do you think /var/sandbox/tmp-xxxx is better, 

It breaks quota.

You end up with a new random directory where you can dump files.

My mechanism maintains all files in your homedir. and /tmp.

Tomas suggested the 0770.

If you are running F15 you can download the latest policycoreutils and selinux-policy from brew.  If we get consensus I will push the packages to F13,F14,F15 and RHEL6

(In reply to comment #63)
> rmdir will not follow a symlink.   I am not doing a full cleanup.  just
> rmdir(tmpdir),  Which will fail if it is anything other then a directory.

What Tavis points out is that you can stop seunshare right after mkdtemp, leave it stopped for a couple of days so the temporary directory gets old enough to be removed by tmpwatch, create symlink with the same name and continue seunshare.  If you stop before chmod, seunshare will change permission of the wrong directory.  If you stop before mount, you can mount a directory that is not accessible to the user, or a directory that is user owned and non-sticky.

> Why do you think /var/sandbox/tmp-xxxx is better

Not tmpwatch clean-up by default, even if tmpwatch is configured to look there too, user won't be able to create symlink with the name matching directory that was previously removed.

Created attachment 482691 [details]
seunshare with post-mount checks

I've added pre-chmod and post-mount check to avoid using unintended directory.

I've also changed the code to fork + drop privs + execute command when needed, i.e. it copies files to a temporary directory before mounting it, so that removes restriction on where the user temporary directory can be located.

The way rsync is used is not ideal, as it won't preserve permissions and contexts.

I did some testing with seunshare.  sandbox -X evince /tmp/file.pdf works too.  sandbox -M bash does not work quite well in both old and new version, as sandbox_t does not seem to have permissions to access /home.

A quick summary of the changes that were done in response to this bugs:

seunshare now creates a runtime temporary directory owned by root and with the sticky bit set properly.  Files form the user-specified directory are copied to the runtime directory and the changes synced back (using rsync) at the end of the seunshare run.

seunshare was also moved from the policycoreutils subpackage (installed by default on all systems) to policycoreutils-sandbox subpackage.  This subpackage can be easily removed where sandbox functionality is not required, providing a cleaner mitigation for the case of possible future seunshare issues.

Correct

This issue has been addressed in following products:

  Red Hat Enterprise Linux 6

Via RHSA-2011:0414 https://rhn.redhat.com/errata/RHSA-2011-0414.html