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");
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. :)
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). detach. pl:system( "cp $virshxmlfile /tmp/wtf");
> ./tools/
Yeah, this line should just be removed.
> ./node/ handlers_ xen.c: snprintf(filename, 1024, "/tmp/consoleOu tput.%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