Comment 3 for bug 445105

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