security hole in /etc/cron.daily/apport

[Stephane Chazelas]

That script has:

[ -d /var/crash ] || exit 0
find /var/crash -mindepth 1 -mtime +7 -print0 | xargs -0 rm -f
find /var/crash -mindepth 1 -empty -print0 | xargs -0 rm -f

/var/crash is world writable, so one can create directories in there.

One can for instance create a /var/crash/foo/passwd file in there that is more than 7 days old, and also mount a fuse filesystem as /var/crash/xxx where directory listing is very slow, or simply create a directory that contains millions of files, so that find will take several minutes to list them. During that time, our "evil" user can change /var/crash/foo to a symlink to /etc so that when xargs eventually spawns rm, it removes /etc/passwd.

I beleive -maxdepth was intended instead of -mindepth. Note that -empty, -mindepth and -maxdepth are not standard (GNU extension also found in some BSD finds).

Maybe a better way to write it:

cd /var/crash &&
find . ! -name . -prune \( -type f -mtime +7 -o -size 0 \) -exec rm -f {} +

(the + above is standard but implies a recent enough version of GNU find)

[Stephane Chazelas (stephane-chazelas)]

2009-04-07 13:33:01 -0000, Stephane Chazelas:
[...]
> Maybe a better way to write it:
>
> cd /var/crash &&
> find . ! -name . -prune \( -type f -mtime +7 -o -size 0 \) -exec rm -f {} +
[...]

Sorry, misplaced braces above:

find . ! -name . -prune -type f \( -mtime +7 -o -size 0 \) -exec rm -f {} +

Best regards,
Stephane

[Martin Pitt (pitti)]

Thank you for discovering this. I'll handle this as soon as possible, but since it was not yet annouced publicly I might stall it until next week (I am on a conference this week). I also subscribed the Ubuntu Security Team to alert them, perhaps you can assign a CVE to this?

[Martin Pitt (pitti)]

I am also subscribing Jan Blunck, who packages Apport for OpenSUSE. Does this affect you, or do you have a different cron job?

I propose that I'll work on a patch for this, test it thoroughly, attach it here, and then we agree on a release date (both upstream and distro security updates).

[Martin Pitt (pitti)]

Tentatively opening an OpenSUSE task. Jan, if that does not affect you, please just set to invalid.

[Martin Pitt (pitti)]

Stephane, I originally added -mindepth 1 to avoid deleting /var/crash/ itself in the cronjob. However, I think -type f -mindepth 1 -maxdepth 1 is a safer way.

So for the security update I propose to change

  find /var/crash -mindepth 1 -mtime +7 -print0 | xargs -0 rm -f
  find /var/crash -mindepth 1 -empty -print0 | xargs -0 rm -f

to

  find /var/crash -mindepth 1 -maxdepth 1 -type f -mtime +7 -print0 | xargs -0 rm -f
  find /var/crash -mindepth 1 -maxdepth 1 -type f -empty -print0 | xargs -0 rm -f

for the sake of minimal and well-understood changes. I'll do the more standards friendly version you proposed upstream.

(Disclaimer: completely untested yet, I just confirmed that -type f does not catch symlinks)

[Jan Blunck (jblunck)]

On Wed, Apr 08, Martin Pitt wrote:

> I am also subscribing Jan Blunck, who packages Apport for OpenSUSE. Does
> this affect you, or do you have a different cron job?
>
> I propose that I'll work on a patch for this, test it thoroughly, attach
> it here, and then we agree on a release date (both upstream and distro
> security updates).

That should work. Thanks for keeping me in the loop.

Cheers,
Jan

[Stephane Chazelas (stephane-chazelas)]

2009-04-08 23:11:34 -0000, Martin Pitt:
> Stephane, I originally added -mindepth 1 to avoid deleting /var/crash/

Hi Martin, thanks for looking into that

note that rm -f wont remove directories, so it's only useful in
case /var/crash has been changed to a file or to avoid an error
message.

> itself in the cronjob. However, I think -type f -mindepth 1 -maxdepth 1
> is a safer way.

Yes, that's the non-portable equivalent of

find /var/crash/. ! -name . -prune -type f

> So for the security update I propose to change
>
> find /var/crash -mindepth 1 -mtime +7 -print0 | xargs -0 rm -f
> find /var/crash -mindepth 1 -empty -print0 | xargs -0 rm -f
>
> to
>
> find /var/crash -mindepth 1 -maxdepth 1 -type f -mtime +7 -print0 | xargs -0 rm -f
> find /var/crash -mindepth 1 -maxdepth 1 -type f -empty -print0 | xargs -0 rm -f
>
> for the sake of minimal and well-understood changes. I'll do the more
> standards friendly version you proposed upstream.

I forgot to mention that -print0 and xargs -0 were not standard
as well. To have the equivalent of -exec ... {} +, you also need
the (non standard) -r option to xargs to avoid running rm if no
file is found (though it's harmless)

Another advantage of

cd /var/crash &&
  find . ! -name . -prune -type f \( -size 0 -o -mtime +7 \) \
    -exec rm -f {} +

is that it makes smaller rm command lines (so potentially calls
fewer rm commands), also it avoid problems if /var/crash is
renamed or removed in the mean time (though, if that happens,
you're in serious trouble already I suppose).

> (Disclaimer: completely untested yet, I just confirmed that -type f does
> not catch symlinks)
[...]

Even if it did, rm -f would not remove the target of the link
but the link itself and -empty doesn't select symlinks and
-mtime checks the modification time of the link, not its target
(unless the -L option (former -follow predicate) is provided).

Another concern one may have (but that may just be nitpicking):
that script may remove files that are not /owned/ by apport,
which may become a problem if another package is using that
directory. So you may want to add a -name '*.crash' or maybe use
a directory named after the package name.

Best regards,
Stephane

[Martin Pitt (pitti)]

So this is indeed quite hard to reproduce. A simple ln -s /etc /var/crash/etc will not work, since find will just report the /var/crash/etc symlink and not descend. So it indeed needs the much more complex fuse/ultra-large dir trick, but the explanation makes sense to me.

I discussed this with Jamie, and did some local testing. Adding -maxdepth 1 -type f works, and I consider that a safe security update. Rewriting the statement entirely using the -prune way you described will be done in trunk.

[Martin Pitt (pitti)]

Subscribing Will Woods as well, just in case this is packaged in some fedora extras repository where it needs to be fixed as well. Will, this is still embargoed, so please don't commit/upload anything yet.

I am preparing patches now. Would it work for everyone to release the update next Friday, 2009-04-24? Or do you need more time?

[Martin Pitt (pitti)]

Debdiff for jaunty.

This is against the apport 1.0 upstream tarball, and thus not applicable for the OpenSUSE/Fedora branches.

[Martin Pitt (pitti)]

Jamie said that I should attach debdiffs, and security team would apply/upload them.

[Martin Pitt (pitti)]

debdiff for intrepid

this is also suitable for the OpenSUSE branch, since the .spec file installs debian/apport.cron.daily.

[Martin Pitt (pitti)]

hardy debdiff

[Jamie Strandboge (jdstrand)]

I played with this and using fuse to have apport remove arbitrary files in this manner does not seem possible on Ubuntu. Specifically:
http://apps.sourceforge.net/mediawiki/fuse/index.php?title=FAQ#Why_don.27t_other_users_have_access_to_the_mounted_filesystem.3F

By default, other users, including root, cannot access a fuse mounted filesystem mounted by a user. One can give access to others by specifying '-o allow_other' (or allow_root) as a fuse option, but only if /etc/fuse.conf has 'user_allow_other'. On Ubuntu, this option is commented out. Furthermore, the regular DAC permissions appear to be in effect as the mounting user, so removing /var/crash/etc is not possible when a non-root user mounts /var/crash/etc to /etc.

Eg:
$ sudo apt-get install libfuse-dev
$ apt-get source fuse
$ cp ./fuse-*/example/fusexmp.c
$ cd /tmp
$ gcc -Wall `pkg-config fuse --cflags --libs` fusexmp.c -o fusexmp
$ mkdir /var/crash/foo
$ ./fusexmp -o allow_root -o nonempty /var/crash/foo
$ ls -l /var/crash/foo/etc/shadow
-rw-r----- 1 root shadow 1310 2009-04-06 07:46 /var/crash/foo/etc/shadow
$ cat /var/crash/foo/etc/shadow
cat: /var/crash/foo/etc/shadow: Permission denied
$ sudo cat /var/crash/foo/etc/shadow
cat: /var/crash/foo/etc/shadow: Permission denied
$ sudo su -
# cat /var/crash/foo/etc/shadow
cat: /var/crash/foo/etc/shadow: Permission denied

Fuse behavior will need to be reviewed on earlier releases of course, but please let me know if there this analysis is incorrect (I'm new to fuse).

[Martin Pitt (pitti)]

Indeed, thanks Jamie. So that leaves the "huge directory" race, which is still real (although hard to hit). Please feel free to remove the "fuse" bits from the changelogs when you apply/upload the updates.

[Martin Pitt (pitti)]

and finally, the patch for trunk. I am using the classical \; instead of +, since old GNU find does not yet support +.

I am going to release a new upstream release apport 1.1 on the coordinated release date with that patch applied.

[Jamie Strandboge (jdstrand)]

Marked as 'Low' based on the attack vector being limited to a timing-based symlink attack. Will update as needed as the impact is further evaluated.

[Stephane Chazelas (stephane-chazelas)]

2009-04-15 19:20:50 -0000, Martin Pitt:
> So this is indeed quite hard to reproduce. A simple ln -s /etc
> /var/crash/etc will not work, since find will just report the
> /var/crash/etc symlink and not descend. So it indeed needs the much more
> complex fuse/ultra-large dir trick, but the explanation makes sense to
> me.

That's a very common trick though, described in a lot of
resources about security.

It's a race condition of the type:
if (condition) action
where the condition can be changed by someone else in between
the time the condition is evaluated and the action with the
addition that the attacker may introduce as large a delay as he
likes (creating a huge dir is one example, there may exist
more). The changing of a dir to a symlink is a very well know
trick as well.

I agree you can forget about the fuse one, if an "evil" user is
allowed to mount fuse filesystem with allow_root, you're in big
trouble anyway. It just happened I has fuse in mind when I wrote
the report.

> I discussed this with Jamie, and did some local testing. Adding
> -maxdepth 1 -type f works, and I consider that a safe security update.
> Rewriting the statement entirely using the -prune way you described will
> be done in trunk.

Yes, that seems OK to me as well AFAICT.

Best regards,
Stephane

[Stephane Chazelas (stephane-chazelas)]

2009-04-15 20:39:32 -0000, Jamie Strandboge:
> Marked as 'Low' based on the attack vector being limited to a timing-
> based symlink attack. Will update as needed as the impact is further
> evaluated.
[...]

It's a:

if (condition) action

race condition where there's an easy and obvious way (there
might be even simpler ways I've not thought of) for an attacker
to make the window for the race condition as large as he wishes,
so I wouldn't really call it a "timing based" one.

It's really a classical one which looks just like the poor
"cleaning /tmp scripts" found on some systems. I just found out
there's even a lengthy section about that in GNU find
documentation:

info find 'Security Considerations'

--
Stephane

[Martin Pitt (pitti)]

Stephane Chazelas [2009-04-16 6:21 -0000]:
> info find 'Security Considerations'

Indeed I read this last night, and it explains it quite well. It even
points out that -exec rm {} has the very same problem (which is really
quite obvious), but limiting the search depth to 1 should close that
hole AFAICS. Using the -delete action is an interesting option, but
this is again BSD find specific.

[Stephane Chazelas (stephane-chazelas)]

2009-04-15 20:27:14 -0000, Martin Pitt:
> and finally, the patch for trunk. I am using the classical \; instead of
> +, since old GNU find does not yet support +.
[...]
PATCH>+find /var/crash/. ! -name . -prune -type f \( -size 0 -o -mtime +7 \) -exec rm -fv '{}' \;

FYI, the -v option to rm is not standard either.

Standardly:
-exec rm -f '{}' \; -exec printf "removed \`%s'\n" {} \;

(though it will output the "removed..." line even if the file
was not there in the first place (not really a problem as it was
there at the time tests were done, so it has been removed
even if not by rm), and the message will not be localised).

Best regards,
Stephane

[Martin Pitt (pitti)]

Stephane Chazelas [2009-04-16 14:21 -0000]:
> FYI, the -v option to rm is not standard either.

Oops, it shouldn't be there in the first place. I removed the -v from
the trunk patch.

[Jamie Strandboge (jdstrand)]

This has been assigned CVE-2009-1295. It is not public yet. Is the CRD finalized (so I can notify vendor-sec)?

[Martin Pitt (pitti)]

Jamie Strandboge [2009-04-20 17:28 -0000]:
> This has been assigned CVE-2009-1295. It is not public yet.

Thanks!

> Is the CRD finalized (so I can notify vendor-sec)?

Still waiting for Jan's ack, but if he doesn't answer, I'd just go
with it (this Friday).

[Jan Blunck (jblunck)]

Sorry, I was on vacation and forgot to CC our security team on this one (Marcus is subscribed by now).

[Marcus Meissner (meissner)]

Talked to Jan,

The patches not for "trunk" look good to me, the patch "patch for trunk" should get -maxdepth too.

[Martin Pitt (pitti)]

Marcus Meissner [2009-04-22 9:55 -0000]:
> The patches not for "trunk" look good to me, the patch "patch for trunk"
> should get -maxdepth too.

Getting rid of the GNU-only -maxdepth was part of the exercise. It
behaves like -maxdepth 1 now, due to the pruning.

[Stephane Chazelas (stephane-chazelas)]

2009-04-22 10:13:36 -0000, Martin Pitt:
> Marcus Meissner [2009-04-22 9:55 -0000]:
> > The patches not for "trunk" look good to me, the patch "patch for trunk"
> > should get -maxdepth too.
>
> Getting rid of the GNU-only -maxdepth was part of the exercise. It
> behaves like -maxdepth 1 now, due to the pruning.

More exactly,

find . ! -name . -prune ...

Is the standard/portable equivalent of GNU's

gfind . -mindepth 1 -maxdepth 1

The standard equivalent of gfind -maxdepth 1 would be:

find . \( -name . -o -prune \) ...

[...]
> cd /var/crash &&
> find . ! -name . -prune -type f \( -mtime +7 -o -size 0 \) -exec rm -f {} +
[...]

Best regards,
Stephane

[Jan Blunck (jblunck)]

Marcus: is the CRD this Friday doable for us as well?

[Marcus Meissner (meissner)]

due to high load I don't think we can make friday.

but I have no problem if this is disclosed on friday, we will just be late.

[Marcus Meissner (meissner)]

due to high load I don't think we can make friday.

Perhaps mid of next week?

[Martin Pitt (pitti)]

Marcus Meissner [2009-04-22 11:10 -0000]:
> Perhaps mid of next week?

No problem, that's why I asked in the first place. Jamie, Marcus,
let's do next Wednesday then (April 29).

Martin

[Marcus Meissner (meissner)]

Ok for april 29 crd.

[Jamie Strandboge (jdstrand)]

Reported to vendor-sec with CRD of 2009-04-29.

[Jamie Strandboge (jdstrand)]

Gutsy is EOL.

[Jamie Strandboge (jdstrand)]

Feedback from vendor-sec is that the patch is probably not good enough, specifically:

"At the very least, you need to add " --" after "rm -f" to prevent
option-passing to "rm" via filenames starting with dashes."

It was then suggested that /var/crash could be cleaned out with a program specifically designed to clean out temporary directories (http://shlang.com/stmpclean/ plus http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/stmpclean/).

[Stephane Chazelas (stephane-chazelas)]

2009-04-27 16:18:02 -0000, Jamie Strandboge:
> Feedback from vendor-sec is that the patch is probably not good enough,
> specifically:
>
> "At the very least, you need to add " --" after "rm -f" to prevent
> option-passing to "rm" via filenames starting with dashes."

No,

The patch has

find /var/crash/. ... -exec ...
or
find . ... -exec ...
in my suggestion

So all the file paths will start with "/var/crash" or "./", not
dash so the "--" is not necessary.

[...]
> Maybe a better way to write it:
>
> cd /var/crash &&
> find . ! -name . -prune -type f \( -mtime +7 -o -size 0 \) -exec rm -f {} +
>
> (the + above is standard but implies a recent enough version of GNU find)
[...]

Best regards,
Stephane

[Martin Pitt (pitti)]

Indeed that's the same case for the stable patches I proposed. All paths delivered by find start with /var/crash/, so the -- isn't strictly needed. OTOH it does not really hurt to have it, so if you upload the debdiffs, please feel free to add them.

As for "use an existing program", /lib/init.d/bootclean.sh does

        find . -depth -xdev $TEXPR $EXCEPT ! -type d \
                -print0 | xargs -0r rm -f -- \
                || { report_err ; return 1 ; }

(with TEXPR being something like -mtime, thus not affecting result structure, and EXCEPT being something like "! -path ...", thus only some filters and also not affecting result structure).

Thus my updated find expression

  find /var/crash -mindepth 1 -maxdepth 1 \( -type f -o -type l \) -mtime +7 -print0 | xargs -0 rm -f

is much stricter than the bootclean.sh one, except for the --.

I still think it is sufficient.

Thanks!

[Stephane Chazelas (stephane-chazelas)]

2009-04-28 07:38:42 -0000, Martin Pitt:
> Indeed that's the same case for the stable patches I proposed. All paths
> delivered by find start with /var/crash/, so the -- isn't strictly
> needed. OTOH it does not really hurt to have it, so if you upload the
> debdiffs, please feel free to add them.

Yes, that -- is not necessary, it makes the command line longer,
with no additional benefit but appart from that doesn't hurt.

> As for "use an existing program", /lib/init.d/bootclean.sh does
>
> find . -depth -xdev $TEXPR $EXCEPT ! -type d \
> -print0 | xargs -0r rm -f -- \
> || { report_err ; return 1 ; }

I hope that bootclean stuff is not called after a user may log
in the system as it has the same problem as that reported in
this bug.

It's also got a bug:

On intrepid at least, EXCEPT is defined as:

        EXCEPT='! -name .
                ! ( -path ./lost+found -uid 0 )
                ! ( -path ./quota.user -uid 0 )
                ! ( -path ./aquota.user -uid 0 )
                ! ( -path ./quota.group -uid 0 )
                ! ( -path ./aquota.group -uid 0 )
                ! ( -path ./.journal -uid 0 )
                ! ( -path ./.clean -uid 0 )
                ! ( -path './...security*' -uid 0 )'

Those inner "'" have no effect, but upon expansion of $EXCEPT,
filename generation will be performed on the "./...security*"
word resulting for word splitting. If there is more than one
file matching that pattern, that will cause find to fail.
The fix would be to do:

(set -f; find ... $EXCEPT ...)

to prevent filename generation upon expansion of $EXCEPT.

Also, it doesn't stop find from descending into lost+found
(which is a directory anyway so wouldn't be selected).

The fix would be:

        EXCEPT='! -name .
                ! ( -path ./lost+found -uid 0 -prune )
                ! ( -path ./quota.user -uid 0 )
                ! ( -path ./aquota.user -uid 0 )
                ! ( -path ./quota.group -uid 0 )
                ! ( -path ./aquota.group -uid 0 )
                ! ( -path ./.journal -uid 0 )
                ! ( -path ./.clean -uid 0 )
                ! ( -path ./...security* -uid 0 )'

And remove "-depth"

Note that -path is a very recent addition to the POSIX
specification (SUSv4). -print0/-0r are not standard.

[...]
> Thus my updated find expression
>
> find /var/crash -mindepth 1 -maxdepth 1 \( -type f -o -type l \)
> -mtime +7 -print0 | xargs -0 rm -f
>
> is much stricter than the bootclean.sh one, except for the --.

Agreed.

> I still think it is sufficient.

Agreed.

Cheers,
Stephane

[Martin Pitt (pitti)]

Stephane Chazelas [2009-04-28 8:18 -0000]:
> It's also got a bug:

Should be filed in a separate report against sysvinit, since it's
unrelated to this one.

> Note that -path is a very recent addition to the POSIX
> specification (SUSv4). -print0/-0r are not standard.

It doesn't need to be, since those init scripts are Debian/Ubuntu
specific.

Thanks,

Martin

[Jamie Strandboge (jdstrand)]

While the use of '--' is not required because all paths will start with '/var/crash' and because it uses -print0 with 'xargs -0', it isn't susceptible to files with spaces in them (as mentioned), I plan to add '--' to the end in the -security updates, to help future-proof the command.

[Launchpad Janitor (janitor)]

This bug was fixed in the package apport - 0.108.4

---------------
apport (0.108.4) hardy-security; urgency=low

  * etc/cron.daily/apport: Only attempt to remove files and symlinks, do not
    descend into subdirectories of /var/crash/. Doing so might be exploited by
    a race condition between find traversing a huge directory tree, changing
    an existing subdir into a symlink to e. g. /etc/, and finally getting that
    piped to rm. Patch based on work from Martin Pitt. Thanks to Stephane
    Chazelas for discovering this!
    - LP: #357024
    - CVE-2009-1295

 -- Jamie Strandboge <email address hidden> Wed, 29 Apr 2009 08:32:35 -0500

[Launchpad Janitor (janitor)]

This bug was fixed in the package apport - 0.119.2

---------------
apport (0.119.2) intrepid-security; urgency=low

  [ Martin Pitt ]
  * etc/cron.daily/apport: Only attempt to remove files and symlinks, do not
    descend into subdirectories of /var/crash/. Doing so might be exploited by
    a race condition between find traversing a huge directory tree, changing
    an existing subdir into a symlink to e. g. /etc/, and finally getting that
    piped to rm. Patch based on work by Martin Pitt. Thanks to Stephane
    Chazelas for discovering this!
    - LP: #357024
    - CVE-2009-1295

 -- Jamie Strandboge <email address hidden> Wed, 29 Apr 2009 08:34:14 -0500

[Launchpad Janitor (janitor)]

This bug was fixed in the package apport - 1.0-0ubuntu5.2

---------------
apport (1.0-0ubuntu5.2) jaunty-security; urgency=low

  [ Martin Pitt ]
  * etc/cron.daily/apport: Only attempt to remove files and symlinks, do not
    descend into subdirectories of /var/crash/. Doing so might be exploited by
    a race condition between find traversing a huge directory tree, changing
    an existing subdir into a symlink to e. g. /etc/, and finally getting that
    piped to rm. Patch based on work by Martin Pitt. Thanks to Stephane
    Chazelas for discovering this!
    - LP: #357024
    - CVE-2009-1295

 -- Jamie Strandboge <email address hidden> Wed, 29 Apr 2009 08:33:09 -0500

[Martin Pitt (pitti)]

Fixed upstream in version 1.1.1. See the announcement: https://launchpad.net/apport/+announcement/2601

[Launchpad Janitor (janitor)]

This bug was fixed in the package apport - 1.1.1-0ubuntu1

---------------
apport (1.1.1-0ubuntu1) karmic; urgency=low

  [ Martin Pitt ]
  * New upstream security update:
    - etc/cron.daily/apport: Only attempt to remove files and symlinks, do not
      descend into subdirectories of /var/crash/. Doing so might be exploited by
      a race condition between find traversing a huge directory tree, changing
      an existing subdir into a symlink to e. g. /etc/, and finally getting
      that piped to rm. This also changes the find command to not use GNU
      extensions. Thanks to Stephane Chazelas for discovering this!
      (LP: #357024, CVE-2009-1295)
    - Other fixes were already cherrypicked in the previous upload.

  [ Matt Zimmerman ]
  * package-hooks/source_linux.py: Attach info for linux-restricted-modules
    and linux-backports-modules

 -- Martin Pitt <email address hidden> Thu, 30 Apr 2009 09:08:29 +0200