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.
