This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 633544 - (CVE-2011-1011) CVE-2011-1011 policycoreutils: insecure temporary directory handling in seunshare
CVE-2011-1011 policycoreutils: insecure temporary directory handling in seuns...
Status: CLOSED CURRENTRELEASE
Product: Security Response
Classification: Other
Component: vulnerability (Show other bugs)
unspecified
All Linux
high Severity high
: ---
: ---
Assigned To: Red Hat Product Security
public=20110222,reported=20100817,sou...
: Security
Depends On: 679689 679690 684918 684919
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-13 18:13 EDT by Vincent Danen
Modified: 2015-07-31 02:31 EDT (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-09-02 08:27:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Rewrote seunshare.c to create tmpfs /tmp and label it correctly. (13.85 KB, text/x-csrc)
2011-02-23 17:03 EST, Daniel Walsh
no flags Details
Here is a patch to the original seunshare to fix some of the problems found (18.61 KB, patch)
2011-02-24 12:17 EST, Daniel Walsh
no flags Details | Diff
Updated code (15.01 KB, text/x-csrc)
2011-02-24 12:19 EST, Daniel Walsh
no flags Details
Ok this version allocates a directory in /tmp an labels it correctly (16.46 KB, text/x-csrc)
2011-03-03 12:21 EST, Daniel Walsh
no flags Details
Yet another version.... This one cleans up after itself. (17.02 KB, text/x-csrc)
2011-03-03 15:38 EST, Daniel Walsh
no flags Details
seunshare with post-mount checks (21.33 KB, text/plain)
2011-03-07 09:25 EST, Tomas Hoger
thoger: review? (dwalsh)
Details

  None (edit)
Description Vincent Danen 2010-09-13 18:13:11 EDT
Tavis Ormandy reported that seunshare, part of policycoreutils, was shipped setuid root and failed to enforce proper permissions on an alternate temporary directory mounted as /tmp.  This could potentially lead to privilege escalation in certain privileged applications that assume /tmp to be sticky and root-owned.

Acknowledgements:

Red Hat would like to thank Tavis Ormandy for reporting this issue.
Comment 9 Tomas Hoger 2010-10-13 09:19:57 EDT
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.
Comment 10 Daniel Walsh 2010-10-13 10:23:26 EDT
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?
Comment 11 Tomas Hoger 2010-10-29 08:56:03 EDT
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?
Comment 12 Daniel Walsh 2010-10-29 09:49:20 EDT
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.
Comment 13 Daniel Walsh 2010-10-29 09:50:43 EDT
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.)
Comment 14 Tomas Hoger 2010-10-29 12:08:02 EDT
(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.
Comment 15 Daniel Walsh 2010-11-01 13:28:28 EDT
So Tomas you think the only valid/safe thing to do is to use tmpfs?
Comment 16 Tomas Hoger 2010-11-02 11:22:32 EDT
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.
Comment 17 Tomas Hoger 2010-11-16 12:19:49 EST
Dan, does any of the above option looks preferred wrt existing seunshare uses?
Comment 18 Daniel Walsh 2010-11-16 13:26:58 EST
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.
Comment 19 Tomas Hoger 2010-11-19 07:56:50 EST
(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?
Comment 20 Daniel Walsh 2010-11-19 08:25:33 EST
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.
Comment 21 Tomas Hoger 2010-11-24 09:09:09 EST
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.
Comment 22 Daniel Walsh 2010-11-24 09:40:59 EST
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.
Comment 23 Tomas Hoger 2010-11-24 09:57:11 EST
Or only allow certain path to be used as NEWTMP, such as /var/tmp/<something>?
Comment 24 Daniel Walsh 2010-11-24 10:51:53 EST
How does that help?
Comment 25 Tomas Hoger 2010-11-24 11:43:05 EST
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.
Comment 26 Tavis Ormandy 2011-02-18 17:26:01 EST
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@EXAMPLE.COM: : 
[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@EXAMPLE.COM'

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.
Comment 27 Tomas Hoger 2011-02-21 05:05:41 EST
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.
Comment 28 Daniel Walsh 2011-02-21 14:48:18 EST
Tavis, how about we just go with removing -t PATH and just using -t, which will mount /dev/shm as 1777 root:root.
Comment 29 Daniel Walsh 2011-02-21 15:04:24 EST
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.
Comment 30 Tavis Ormandy 2011-02-21 23:06:05 EST
Yes, that sounds okay to me so long as seunshare creates it.
Comment 31 Daniel Walsh 2011-02-22 10:03:08 EST
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?
Comment 32 Tavis Ormandy 2011-02-22 10:25:02 EST
Yes, ksu without the password destroys the file.
Comment 33 Tomas Hoger 2011-02-22 10:31:57 EST
I confirm that ksu steps above work on EL6 with default krb5 config.
Comment 34 Tavis Ormandy 2011-02-22 12:20:15 EST
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.
Comment 35 Tavis Ormandy 2011-02-22 18:57:33 EST
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.
Comment 36 Eugene Teo (Security Response) 2011-02-22 20:01:21 EST
Hey Tavis, thanks.

http://lists.grok.org.uk/pipermail/full-disclosure/2011-February/079490.html
Comment 38 Daniel Walsh 2011-02-23 17:03:20 EST
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.
Comment 39 Tavis Ormandy 2011-02-23 17:44:33 EST
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.
Comment 40 Steve Grubb 2011-02-23 18:02:46 EST
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?
Comment 41 Tavis Ormandy 2011-02-23 18:16:59 EST
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.
Comment 42 Tomas Hoger 2011-02-24 06:55:26 EST
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").
Comment 43 Tomas Hoger 2011-02-24 07:43:19 EST
(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.
Comment 44 Daniel Walsh 2011-02-24 12:17:30 EST
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.
Comment 45 Daniel Walsh 2011-02-24 12:19:30 EST
Created attachment 480817 [details]
Updated code
Comment 46 Tavis Ormandy 2011-02-24 12:56:41 EST
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.
Comment 47 Daniel Walsh 2011-02-24 13:09:27 EST
We also verify the mount after it is done, so you can not replace the mount with something now owned by the user.
Comment 48 Tavis Ormandy 2011-02-24 13:23:40 EST
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
Comment 49 Daniel Walsh 2011-02-24 13:27:08 EST
And the second verify_mount does not fail.
Comment 50 Tavis Ormandy 2011-02-24 13:46:06 EST
Yes, you're right, just stating that it's scary that the mount will succeed.

I do not have any specific attack against it.
Comment 51 Daniel Walsh 2011-02-24 13:55:38 EST
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.
Comment 52 Tomas Hoger 2011-02-25 06:47:53 EST
(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.
Comment 53 Daniel Walsh 2011-02-25 10:24:31 EST
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=
Comment 54 Tomas Hoger 2011-02-25 11:16:46 EST
(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));
Comment 55 Daniel Walsh 2011-02-25 15:32:39 EST
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.
Comment 56 Tomas Hoger 2011-02-26 17:27:07 EST
(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.
Comment 57 Daniel Walsh 2011-03-03 12:21:05 EST
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.
Comment 58 Daniel Walsh 2011-03-03 13:34:13 EST
Tomas has suggested on line that I change the tmpdir to be 

root,dwalsh,1770.

Tavis do you think there is risk in this?
Comment 59 Daniel Walsh 2011-03-03 14:00:49 EST
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.
Comment 60 Tavis Ormandy 2011-03-03 15:04:25 EST
I think that sounds okay, but as Tomas says 1770 is definitely the right permissions.
Comment 61 Daniel Walsh 2011-03-03 15:38:54 EST
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)
Comment 62 Tavis Ormandy 2011-03-03 16:26:57 EST
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.
Comment 63 Daniel Walsh 2011-03-03 17:23:21 EST
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
Comment 64 Tomas Hoger 2011-03-04 09:04:33 EST
(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.
Comment 65 Tomas Hoger 2011-03-07 09:25:29 EST
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.
Comment 67 Tomas Hoger 2011-04-04 05:47:13 EDT
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.
Comment 68 Daniel Walsh 2011-04-04 15:44:47 EDT
Correct
Comment 69 errata-xmlrpc 2011-04-04 17:13:01 EDT
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

Note You need to log in before you can comment on or make changes to this bug.