Currently, revdep-rebuild creates a slew of files under root's $HOME. Aside from being slightly annoying because short-term temp files really don't belong in one's home directory, this is a pretty big issue for me. I administer about 25 Gentoo servers, which all share a /home NFS mount, and root's $HOME is /home/root. I update them all at once from a screen session on an infrequent, periodical basis. After compiling and depcleaning, I always like to run revdep-rebuild to go through everything because portage breaks something, usually pretty important, 99% of the time (ahh, I'm so glad for revdep-rebuild!). But, especially on the older <1GHz machines with slow disks, revdep-rebuild takes a good while. I found out the hard way that if I fired it up on all 25 machines at once, it didn't work at all. Okay, I can do one machine at a time, but inevitably, I end up firing it up on one forgetting that I already started it somewhere else, and usually get quite confused when this happens. But even if I'm patient, and carefully do one machine at a time, it takes a lot longer due to having to wait for each machine one by one. So, that's my frustration. Now, after a while, it occured to me to do `$HOME=/tmp revdep-rebuild`, and this works great! But I'd still think this should be fixed even though a workaround exists. Ideally, I feel that revdep-rebuild's temp files should go into $PORTAGE_TMPDIR/revdep-rebuild/ and be named things like 0_env, 1_files, etc.
I tend to agree, and I even wrote code to this effect in one of my iterations of rewriting rr, but dealing with more pressing bugs has kept it off the table for now... I'll make a note in the TODO list.
Created attachment 144400 [details, diff] revdep-rebuild_tmpdir.patch Patch against r474 in SVN. This patch makes some fairly drastic changes, but in my opinion they're the best way to go about handling tempfiles in a reasonably secure way. My intent was as follows: 1) Create a directory called "${TMPDIR}/revdep-rebuild", where TMPDIR defaults to /tmp if not set by the user. 2) chmod directory 770 and chown it to the portage group (I wasn't sure if I should set the sticky bit or not, but since non-portage users shouldn't be able to enter the TMPDIR, I hope it's moot.) 3) Create individual directories for library searches. For example, if you're searching for packages depending on libxslt.so you'll find the revdep-rebuild files in "$TMPDIR/revdep-rebuild/libxslt.so" 4) Create files in the directory named foo.rr, such as 0_env.rr, 1_files.rr, 2_ldpath.rr and so on. 5) Cleanly hardcode the list of files r-r uses. Revdep-rebuild only uses about 10 files, so by manually listing them at the beginning of the script it's easier to keep them managed and not accidentally delete stuff. There's no more hairy rm -f * voodoo in revdep-rebuild.
Security people: Since the attachment in comment #2 introduces some fairly drastic changes to how revdep-rebuild handles tempfiles, I'd appreciate any input you can give. Thanks, Mike
(In reply to comment #3) > Security people: Since the attachment in comment #2 introduces some fairly > drastic changes to how revdep-rebuild handles tempfiles, I'd appreciate any > input you can give. I just discussed this with kojiro on IRC and from a fast look at the code, it patch moves the default directory from $HOME to $TMPDIR without adding any sanity checks, which would result in the vulnerability to symlink attacks. /tmp/revdep-rebuild or any directory in there could be symlinks to other root-writable directories, and the script would make them chmod 770 for portage group. It might also allow for data loss if files in there are overwritten. IMO since the data revdep-rebuild generates is machine-specific state that should be available between restarts of revdep-rebuild (and the system), using /var/lib/revdep-rebuild seems more appropriate.
After speaking with rbu on IRC about the security issues inherent with using tmpfiles in publicly writable directories like /tmp, and taking some of his suggestions into account, this is how I would like to go forward: 1. Have the gentoolkit ebuild create: drwxrwx--- 2 root portage 4096 Feb 25 14:04 /var/lib/revdep-rebuild 2. Use that path for the root of all of revdep-rebuild's state files 3. Don't check for or worry about other directories. Rationale: - It doesn't make sense for users not in the portage group to run revdep-rebuild. It would fail anyway. - Users in the portage group ought to be trustworthy, so to prove a vulnerability you would have to attack the files from lower access than portage-group - It may be possible to exploit the patch in comment #2 by a non-portage user creating a symlink at /tmp/revdep-rebuild before revdep-rebuild is run. - Such an attack would not be possible by a user not in the portage group if /var/lib/revdep-rebuild were only accessible by root and portage-group members. I'll get working on this right away.
giving revdep-rebuild its own /var/lib directory is crazy. use $tmpdir/revdep-rebuild and give it proper permissions.
Created attachment 144695 [details, diff] revdep-rebuild_tmpdir2.patch OK, this does more strict checking to make sure $TMPDIR/revdep-rebuild (or whatever) is not a symlink and is owned by root:portage and has at least 02770 permissions. Please try to break it. Thanks :)
Michael, the temp code looks good to me. Removing security, please readd if you need additional comments.
After applying the patch, if you run revdep-rebuild as a user, you get the following permissions: drwxrws--- 2 pvarner portage 4096 Mar 14 17:50 . drwxrwxrwt 19 root root 36864 Mar 14 18:00 .. -rw-r--r-- 1 pvarner portage 285047 Mar 14 17:50 1_files.rr -rw-r--r-- 1 pvarner portage 12290 Mar 14 17:50 2_ldpath.rr -rw-r--r-- 1 pvarner portage 3910 Mar 14 17:55 3_errors.rr Subsequent runs as root then produce: # ./revdep-rebuild --ignore --keep-temp --verbose --pretend * Configuring search environment for revdep-rebuild * Incorrect permissions on /tmp/revdep-rebuild * or at least one file in /tmp/revdep-rebuild. * Please make sure it's not a symlink and then remove it. I'm going to commit what I have, but it needs a little bit further testing and tweaking before we release it. $ svn commit -m 'Change revdep-rebuild to not use /root for temp files. It now uses $TMPDIR/revdep-rebuild. (Bug 203414)' Sending revdep-rebuild/revdep-rebuild Transmitting file data . Committed revision 483.
This patch kills the usability of revdep-rebuild as a user in the portage group. We need to either rethink the logic, or restrict revdep-rebuild to the root user only. For now, I am reverting this patch.
$ svn commit -m "Fix revdep-rebuild to use TMPDIR instead of HOME for temporary files. (Bug 203414)" src/revdep-rebuild/revdep-rebuild ChangeLog Sending ChangeLog Sending src/revdep-rebuild/revdep-rebuild Transmitting file data .. Committed revision 492.
Released in gentoolkit-0.2.4_rc5.
[12:30] <hoffie> FuzzyRay: ping wrt bug 203414 [12:32] <hoffie> FuzzyRay: i think the fix you committed is incomplete. there is still a race between the checks, the mkdir call and the fixing of ownership. i think you have to use mktemp
I've moved the temporary cache files from /tmp to /var/tmp, each user gets their own directory of the name /var/tmp/revdep-rebuild-$USER-cache. I don't see any security problems with how it is currently configured. Here is what I am doing: If the directory already exists, it checks the permissions and aborts if they are incorrect. If it doesn't exist, it creates the directory with mkdir -m 0700 and sets the permissions. Finally, it checks the permissions on the directory and aborts if they are incorrect. The code is in revision 500 of the gentoolkit svn repository for anyone that wants to review it.
(In reply to comment #14) > If the directory already exists, it checks the permissions and aborts > if they are incorrect. If it doesn't exist, it creates the directory with > mkdir -m 0700 and sets the permissions. I didn't check the code, but if it works as you describe, there is a race condition between the "if it doesn't exist" and "it creates ...", i.e. if somebody manages to make a symlink after this check he is able to change the permissions of the destination directory of that symlink. To my knowledge, there is *no* secure way to create a temporary file/dir dynamically with a fixed name - it *must* exist statically (i.e. have been created in advance by portage).
(In reply to comment #15) > (In reply to comment #14) > > If the directory already exists, it checks the permissions and aborts > > if they are incorrect. If it doesn't exist, it creates the directory with > > mkdir -m 0700 and sets the permissions. > > I didn't check the code, but if it works as you describe, there is a > race condition between the "if it doesn't exist" and "it creates ...", > i.e. if somebody manages to make a symlink after this check he is able to > change the permissions of the destination directory of that symlink. I had a quick look at the code some days ago and had the same impression, I just hadn't said something as I wanted to investigate further. > To my knowledge, there is *no* secure way to create a temporary file/dir > dynamically with a fixed name - it *must* exist statically (i.e. have been > created in advance by portage). Yes, but I don't know why we are forced to use a static name here -- imo the code should just switch to mktemp (which even supports prefixes, iirc) and it should be fine.
I just discussed this again with rbu on irc. Results: * symlink attacks are no longer possible * current code allows for permanent DoS (evil user "attacker" can create directory /var/tmp/revdep-rebuild-john-cache and make it impossible for user "john" to use revdep-rebuild; only root can solve this) * as we are talking about "state" data, which is supposed to be preserved between multiple invocations of revdep-rebuild, the data belongs to /var/lib/ (according to FHS) Possible solution: * use dynamic names (mktemp) for the temp files in case of non-root * use /var/lib/revdep-rebuild/ (only writable by root) in case of root This still allows for resuming revdep-rebuild sessions in case of root and should fix any DoS problems. Sadly we couldn't think of a solution which keeps the resuming feature for all users. If you've got ideas, feel free to post/implement them :p
Sincerely I'd say this is at least cache data rather than state data. You shouldn't be needed to back up revdep-rebuild's stuff...
While it is state data when the script is running, after completion it is used as a cache. There is no need to include this data in a backup since it is not used if it is stale. Because of this I don't think /var/lib is approriate. I am willing to create a /var/cache/revdep-rebuild directory (or whatever is deemed the most appropriate) as part of the ebuild with the correct permissions. I will implement the solution in comment #17. I don't think the ability to rerun --pretend as a user with the cache file matters much, so mktemp is probably appropriate when run as a user. Bottom line: what is the best location for the cache/state files when run as root? I'm proposing /var/cache/revdep-rebuild. Is there a more appropriate place?
(In reply to comment #19) > While it is state data when the script is running, after completion it is used > as a cache. There is no need to include this data in a backup since it is not > used if it is stale. Because of this I don't think /var/lib is approriate. agreed. > I'm proposing /var/cache/revdep-rebuild. Is there a more appropriate place? no.
The changes from comments #19 and #20 are in subversion revision 503
Looks good to me. Just for reference: http://sources.gentoo.org/viewcvs.py/gentoolkit/trunk/src/revdep-rebuild/revdep-rebuild?r1=500&r2=503
Released in gentoolkit-0.2.4_rc6
I reopen the bug as it's a security issue, so we might want to publish a GLSA about it. at least, i vote YES
updating summary and reassigning to security@
If we are going to release a GLSA, then let me get gentoolkit-0.2.4 in the tree so it can be marked as stable. The changes in revdep-rebuild are too extensive to backport to the current stable 0.2.3-r1
Also gentoolkit-0.2.4_pre8 is unaffected since it is using a BSD specific version of revdep-rebuild that does not create temporary files and is only keyworded for the Gentoo BSD's
(In reply to comment #26) > If we are going to release a GLSA, then let me get gentoolkit-0.2.4 in the tree > so it can be marked as stable. The changes in revdep-rebuild are too extensive > to backport to the current stable 0.2.3-r1 > of course, sorry for the rush without checking keywords first. You may ask for stabilisation directly on this bug when it's ready.
Now that there's a secure way to create tempdirs, would it be possible to remove the symlink checks introduced due to comment #4? They are breaking perfectly valid uses of symlinks, as per bug #234271 and the forums thread referenced there. Thanks
Versions with the vulnerable code were never stable, but only ~arch _rc versions. I do not agree with the A3 rating, fast-stabling a new version and issuing a GLSA. Or am I mistaken here?
gentoolkit-0.2.4 is in the tree. It is identical to gentoolkit-0.2.4_rc6 except for the version number change. Bug 170220 tracks all fixes and changes for this version of gentoolkit. I have also cleaned up all versions so that the only versions in the tree are the current stable version and the unaffected versions. To answer Robert's question. All versions of gentoolkit prior to gentoolkit-0.2.4_rc5 (except pre8) created the temporary files in the users home directory if possible. It would attempt to use /var/tmp if it could not write to the home directory. None of the versions had a check to see if the location was a symlink and they did not check the permissions of any of the directories in question. So at best the versions were secure only if the script was run as root, root's home directory did not have its default permissions modified from rwx------, and root's home directory was writable by root. gentoolkit-0.2.4_pre8 uses a different version of revdep-rebuild that doesn't create temporary files. gentoolkit-0.2.4_rc5 had the problems noted in comment #17. Security, I'm going to leave it up to you for when to request the stable keywords and the release a GLSA. For a GLSA gentoolkit-0.2.4_pre8 is unaffected as well as gentoolkit-0.2.4_rc6 and gentoolkit-0.2.4 (However, I have removed rc6 from the tree, since 0.2.4 is identical)
(In reply to comment #31) > To answer Robert's question. All versions of gentoolkit prior to > gentoolkit-0.2.4_rc5 (except pre8) created the temporary files in the users > home directory if possible. It would attempt to use /var/tmp if it could not > write to the home directory. None of the versions had a check to see if the > location was a symlink and they did not check the permissions of any of the > directories in question. So at best the versions were secure only if the > script was run as root, root's home directory did not have its default > permissions modified from rwx------, and root's home directory was writable by > root. I don't think that $HOME with u-w is a configuration we support. That is not to say I am unhappy with the implementation right now, but I don't think it warrants a GLSA.
cl(In reply to comment #32) > (In reply to comment #31) > > To answer Robert's question. All versions of gentoolkit prior to > > gentoolkit-0.2.4_rc5 (except pre8) created the temporary files in the users > > home directory if possible. It would attempt to use /var/tmp if it could not > > write to the home directory. None of the versions had a check to see if the > > location was a symlink and they did not check the permissions of any of the > > directories in question. So at best the versions were secure only if the > > script was run as root, root's home directory did not have its default > > permissions modified from rwx------, and root's home directory was writable by > > root. > > I don't think that $HOME with u-w is a configuration we support. That is not to > say I am unhappy with the implementation right now, but I don't think it > warrants a GLSA. > Agreed, closing without GLSA.
(In reply to comment #29) > Now that there's a secure way to create tempdirs, would it be possible to > remove the symlink checks introduced due to comment #4? It seems people are still running into this issue, given all the duplicates. Someone on IRC just asked if the symlink check is still necessary as well, and I read the whole bug here again to check so. I don't think the symlink check is necessary any longer; it was initially created because /var/tmp was used and any system user could perform a symlink attack in there. Later discussion led to the decision to use /var/cache instead, which is writable by root only, and thus symlink attacks are no longer possible. In case of a non-root user invoking revdep-rebuild mktemp is used. Thus I would say that the check is a relict only and it should be safe to remove it. Or am I mistaken?
(In reply to comment #34) > (In reply to comment #29) > > Now that there's a secure way to create tempdirs, would it be possible to > > remove the symlink checks introduced due to comment #4? > > It seems people are still running into this issue, given all the duplicates. > Someone on IRC just asked if the symlink check is still necessary as well, and > I read the whole bug here again to check so. > I don't think the symlink check is necessary any longer; it was initially > created because /var/tmp was used and any system user could perform a symlink > attack in there. Later discussion led to the decision to use /var/cache > instead, which is writable by root only, and thus symlink attacks are no longer > possible. > In case of a non-root user invoking revdep-rebuild mktemp is used. Thus I would > say that the check is a relict only and it should be safe to remove it. Agreed, both this check and moving to /var/cache solve the same problem, so the check can be removed if it's causing people pain.
I'm also fine with removing this check (I guess you were also referring to verify_tmpdir). mktemp already handles creation of safe temporary diectories.
Created attachment 188633 [details, diff] revdep-rebuild_notmpdirchecks.patch Removed the tmpdir checks. This is a patch against @548 in svn
$ svn commit -m "Add patch from kojiro to remove permission check on temporary directory. (Bug 203414)" Sending branches/gentoolkit-0.2.4/src/revdep-rebuild/revdep-rebuild Sending trunk/gentoolkit/bin/revdep-rebuild Transmitting file data .. Committed revision 602.
As of 2009-09-14, both stable 0.2.4.5 and unstable 0.3.0_rc7 versions still have the symlink check. Any idea when an updated version will be available (in non-development packages, that is)?
Patch is included in current gentoolkit.
