Bugzilla Bug 96782

the sandbox utility is full of toctou races, these are unacceptable and should
be fixed.

the toctous are of the nature:

is_file_secure(foo);
continue_being_insecure();

(note: i'll work on a patch soon).

While you're at that, please review Bug #47167. That bug being invalid was
unacceptable last year but the situation has unimproved. Nowadays success rate 
increased from 5/8 to nearly 100%.

Ughh

do you care to provide any real information ?  simply saying 'hey, sandbox
sucks' isnt useful

For example:

main():
...
if (file_exist(sandbox_pids_file, 1) < 0) {
...
} else {
 pids_file = file_open(sandbox_pids_file, "r+", 1, 0664, "portage");
...
}
...

file_open():
...
/* file_security_check() runs some tests then returns wether it's safe to 
proceed, of course, there is no guarantee the filesystem hasnt changed since the 
stat() was performed by f_s_c(), the results may not still be true */
file_security_check(filename);
...
 fd = open(filename, file_getmode(mode));
 file_security_check(filename);
...

where file_getmode(mode) resolves to O_RDWR | O_CREAT in this case, which is a 
TOCTOU race, vulnerable to linking attacks. A solution in this case could be to 
check if the file exists, if it doesnt, use O_EXCL, which would fail if anyone 
tried to win the race.

Why do you need /tmp/sandboxpids.tmp, anyway? IIRC, I think it was the reporter
of bug 21923 (though I'll have to check) who first discovered in may 2003 that
sandboxpids.tmp is only 'needed' when sandbox is injected via 
/etc/ld.so.preload using sandboxpid's.tmp as some sort of reference count... 
Sandbox adds itself to that file at startup (but only if it's the first 
instance) and removes itself when it's finished (but only if it's the last 
instance). Nowadays (even two years ago) sandbox does not use /etc/ld.so.
preload, 
so sandboxpids.tmp does not serve any useful purpose (checked it myself with 
today's sandbox, all sandbox does is add and delete a single number to this 
file but does not use it in any way for anything useful). Logfiles, I think 
nobody I talked to could think of any reason why having those in /tmp would be
a good idea.

Ill check the pids file - I am slowly cleaning out the dead code (as you noted,
all the ld.so.preload stuff is out), but I have not really given that much
attention, so cannot say for sure.

As for the logs ... Might move them to /var/log/sandbox/, but just having them
in /var/log/ will bloat things a bit.  I am not a FHS expert, so somebody else
might want to comment.

FHS doesnt say much about /var/log except that it's for logfiles ;)

/var/log/sandbox/ sounds good to me

Ok, please check the latest code, and let me know if there are issues.

 $ svn co svn+ssh://svn.gentoo.org/var/svnroot/path-sandbox

There's something else fishy going on since sandbox-1.2.9 - paths longer than
PATH_MAX don't work at all and sandbox is caught in an endless loop (100% cpu).
After studying Bug #21766 (comment  and 7) I think portage's getcwd is broken
again. CC'ing (with permission) writer of Bug #21766 Comment #7 (you are the 
one who came up with the idea for portage's own getcwd, seems like the older
one can handle unlimited paths and the newer one does not. opinion valued)

  Well, it can't handle "unlimited" length paths... you still need to 
`stat()' every path component either relative to `/' or `.'... nice 
thing about the glibc version is that it goes backwards from `.' and 
needs only two characters (`..') to go back one level as opposed to 
the full directory name in the uclibc version.  In the particular 
scenario used in Bug 21766 comment #4, ratio is 9/3, which means 
glibc `getcwd()' can handle close to (PATH_MAX * 3) while uclib's 
version is approx. (PATH_MAX * 1)...  no real gain here.


  After closer inspection, other reasons why the version in sandbox-1.2.9+
can't work are...

(1) in `__syscall_egetcwd()', members of `struct stat st' are passed 
uninitialized to `recurser()'... `st' should be initialized to "/".

(2) line #131 overwrites `stat_buf' with "/.." ... instead it should be 
appending it (`strcat()' like in uclibs is perfectly save... previous 
statement already makes sure there's enough room left).

(3) this particular implementation requires that the passed in buffer 
points to an existing directory... use "." for best results...

  However, since this is not related to tempfile races, see bug #98419

Well, even the glibc one fails with the specific test (handles a bit longer
paths, but not the depth that coreutils test requires), is rather ugly and at
some point redo the whole path with 'confdir3':

  http://bugs.gentoo.org/show_bug.cgi?id=21766#c39

As for the recursion .. cannot say I can get it here.  Will look at the new bug
though.

  See Bug #98419 for more info on `getcwd'.

Right, besides getcwd() .. can somebody from security comment on the original
problem?

Looks good, the logs are all moved into /var/log?

seems home_dir can be set to /tmp if HOME has been unset? is that ever likely to 
happen? and if so, does it read or write anything in there? (sorry, unfamiliar 
with it's operation :)

If that's not an issue, everything looks fine to me.

Can do.  It should not anymore .. its more to make sure we have a sane TMPDIR
and its writable for ./configure scripts, etc ...

Actually, it already sets, home_dir to /tmp if all else fails.

Martin: do you have any idea of the release date for the fix ?

Thierry, that should be already in 1.2.11.

Ah ok. Great :)

Any reason why we should not ask arch teams to test and stable-ize ? Or do you
prefer to wait a little in ~ to catch bugs ?

Arches, please test sandbox-1.2.11 carefully and mark stable if it is.

amd64 stable

sparc stable.

stable on ppc64

Tested and marked ppc stable.

Tested and marked x86 stable

Stable on hppa. Sorry for the delay.

Alpha and IA64 were done by agriffis

This one is ready for GLSA. 

GLSA 200507-22 
 
arm and s390 don't forget to mark stable.