Bugzilla Bug 203414

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 an attachment (id=144400) [details]
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 an attachment (id=144695) [details]
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 an attachment (id=188633) [details]
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)?