uses unsafe /tmp files

[Kees Cook]

Split from bug 436977; in some places, eucalyptus uses unsafe /tmp files:

./gatherlog/GLclient.c: env = axutil_env_create_all("/tmp/fooh", AXIS2_LOG_LEVEL_TRACE);
./install-sh: tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
./storage/storage.c:#define F1 "/tmp/improbable-cache-file-1"
./storage/storage.c:#define F2 "/tmp/improbable-cache-file-2"
./storage/storage.c:#define F3 "/tmp/improbable-cache-file-3"
./storage/storage.c:#define F4 "/tmp/improbable-cache-file-4"
./storage/storage.c:#define F5 "/tmp/improbable-cache-file-5"
./storage/storage.c:#define RM_CMD "rm -rf /tmp/improbable-cache-file-?"
./cluster/CCclient.c: env = axutil_env_create_all("/tmp/fofo", AXIS2_LOG_LEVEL_TRACE);
./tools/detach.pl:system("cp $virshxmlfile /tmp/wtf");
./tools/httpd.conf:ServerRoot "/tmp"
./tools/euca_watchdog.pl:our $chkpt_file = "/tmp/euca_watchdog.checkpoint";
./node/handlers_xen.c: snprintf(filename, 1024, "/tmp/consoleOutput.%s", instanceId);
./debian/patches/var_lib_eucalyptus.diff: HTTPD_HOME="/tmp/"

ProblemType: Bug
Architecture: amd64
Date: Tue Oct 6 20:09:05 2009
DistroRelease: Ubuntu 9.10
Package: eucalyptus-common (not installed)
ProcEnviron:
 LANGUAGE=en_US.UTF-8
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
ProcVersionSignature: Ubuntu 2.6.31-11.36-generic
SourcePackage: eucalyptus
Uname: Linux 2.6.31-11-generic x86_64

[Daniel Nurmi (nurmi)]

Greetings,

We've analyzed the points in the source tree that are referenced, and have found that none of them are actually exercised in a Karmic default eucalyptus running system. More detail on each follows:

GLclient is a testing utility that doesn't get installed by the package (or a 'make install' for that matter)
./gatherlog/GLclient.c: env = axutil_env_create_all("/tmp/fooh", AXIS2_LOG_LEVEL_TRACE);

install-sh is only used for building eucalyptus
./install-sh: tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$

the function in 'storage.c' that uses these files is only used by 'test.c' in 'storage/', which is a utility that is never installed
./storage/storage.c:#define F1 "/tmp/improbable-cache-file-1"
./storage/storage.c:#define F2 "/tmp/improbable-cache-file-2"
./storage/storage.c:#define F3 "/tmp/improbable-cache-file-3"
./storage/storage.c:#define F4 "/tmp/improbable-cache-file-4"
./storage/storage.c:#define F5 "/tmp/improbable-cache-file-5"
./storage/storage.c:#define RM_CMD "rm -rf /tmp/improbable-cache-file-?"

CCclient is a debugging utility that is not installed
./cluster/CCclient.c: env = axutil_env_create_all("/tmp/fofo", AXIS2_LOG_LEVEL_TRACE);

These two are never used for Karmic by default (which uses the handlers_kvm.c), unless a user installs Xen on their node controllers. The first can be entirely removed (is just there for debugging). The second is actually, I believe, as unpredictable or more so than a file created with mktemp(). Eucalyptus instanceIds are random unique ids of 8 hex characters ("i-ABCDEFGH" where A-H are hex values).
./tools/detach.pl:system("cp $virshxmlfile /tmp/wtf");
./node/handlers_xen.c: snprintf(filename, 1024, "/tmp/consoleOutput.%s", instanceId);

The httpd*.conf that ends up actually being used has this value set to '/' instead of '/tmp'. The init script(s) actually replace this with '/'.
./debian/patches/var_lib_eucalyptus.diff: HTTPD_HOME="/tmp/"
./tools/httpd.conf:ServerRoot "/tmp"

This tool is never installed.
./tools/euca_watchdog.pl:our $chkpt_file = "/tmp/euca_watchdog.checkpoint";

[Dustin Kirkland  (kirkland)]

That's great news, Dan.

Could you commit a single revision that cleans these up? I'll merge
into our tree and upload immediately...

:-Dustin

[Kees Cook (kees)]

On Thu, Oct 15, 2009 at 06:01:44PM -0000, Daniel Nurmi wrote:
> We've analyzed the points in the source tree that are referenced, and
> have found that none of them are actually exercised in a Karmic default
> eucalyptus running system. More detail on each follows:

Excellent, the bulk of these can be ignored then. :)

> These two are never used for Karmic by default (which uses the handlers_kvm.c), unless a user installs Xen on their node controllers. The first can be entirely removed (is just there for debugging). The second is actually, I believe, as unpredictable or more so than a file created with mktemp(). Eucalyptus instanceIds are random unique ids of 8 hex characters ("i-ABCDEFGH" where A-H are hex values).
> ./tools/detach.pl:system("cp $virshxmlfile /tmp/wtf");

Yeah, this line should just be removed.

> ./node/handlers_xen.c: snprintf(filename, 1024, "/tmp/consoleOutput.%s", instanceId);

Would it make sense to use some other location instead of /tmp? While
it's not predictable, the file isn't handled safely on creation. At the
least, I would recommend adding O_EXCL to the flags. This may require
an unlink() call to blow away old files, but that's symlink-attack safe.
With the O_EXCL, worst case is a DoS on the console output.

Speaking of that code, I noticed this on the side that waits for the file
to appear:
    while(count < 10000 && stat(filename, &statbuf) < 0) {count++;}
    fd = open(filename, O_RDONLY);

That loop is going to go by _very_ quickly. e.g. on my system, that takes
0.017 seconds to execute. I would recommend (not for security, but for
possible race bugs) that you use nanosleep() and time() for choosing an
actual timeout to wait with. :)

Thanks for checking!

-Kees

--
Kees Cook
Ubuntu Security Team

[Kees Cook (kees)]

http://bazaar.launchpad.net/~ubuntu-core-dev/eucalyptus/ubuntu/revision/697?remember=695&compare_revid=695

[Launchpad Janitor (janitor)]

This bug was fixed in the package eucalyptus - 1.6~bzr931-0ubuntu4

---------------
eucalyptus (1.6~bzr931-0ubuntu4) karmic; urgency=low

  [ Kees Cook ]
  * Fix unsafe /tmp file uses (LP: #445105):
    - tools/detach.pl: remove debugging XML dump call.
    - node/handlers_xen.c: use mkstemp().

  [ Mathias Gug ]
  * debian/eucalyptus-common.eucalyptus.upstart: Start eucalyptus-cloud
    with all the relevant options (LP: #452665).

  [ Dustin Kirkland ]
  * cluster/handlers.c, net/vnetwork.c: label the 169.254.169.254 link
    local address with :metadata, the public vm ip addresses with :pub,
    and the private vm ip addresses with :priv, to separate it appropriately
    (LP: #452754)
  * debian/eucalyptus-ipaddr.conf: and now that we're separating the weird
    addresses to their own labels, we can show just the device label, much
    cleaner output, (LP: #451607)
  * debian/copyright: Eucalyptus licensed 1.6 under the GPLv3, update
    our copyright file accordingly, (LP: #453129)
  * debian/control, debian/eucalyptus-java-common.links: the CLC absolutely
    needs the ecj.jar installed and linked for euca-* and ec2-* API calls
    to work (LP: #453177)

 -- Dustin Kirkland <email address hidden> Fri, 16 Oct 2009 13:28:32 -0500