search

iamgreaser / iceball

Open-source rewrite of the VOXLAP version of Ace of Spades.
http://iceball.build
GNU General Public License v3.0
114 stars 32 forks source link

Possible security concerns #183

Closed fkaa closed 9 years ago

fkaa commented 9 years ago

According to clang's infinite wisdom, tempnam is deprecated because of security issues and advices you to use mkstemp instead. Is this something we should be concerned about? Are there any downsides to switching to mkstemp instead?

Building C object src/CMakeFiles/iceball-dedi.dir/lua.c.o
In file included from /Users/fkaaman/Dev/iceball/src/lua.c:184:
/Users/fkaaman/Dev/iceball/src/lua_fetch.h:342:20: warning: 'tempnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tempnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]
                                char *tfname = tempnam(NULL, "ibsit");
                                               ^
/usr/include/stdio.h:391:7: note: 'tempnam' has been explicitly marked deprecated here
char    *tempnam(const char *, const char *) __DARWIN_ALIAS(tempnam);
         ^
iamgreaser commented 9 years ago

From the FreeBSD man page tempnam(3):

SECURITY CONSIDERATIONS

The tmpnam() and tempnam() functions are susceptible to a race condition occurring between the selection of the file name and the creation of the file, which allows malicious users to potentially overwrite arbitrary files in the system, depending on the level of privilege of the running program. Additionally, there is no means by which file permissions may be specified. It is strongly suggested that mkstemp(3) be used in place of these functions.

I actually know how such an exploit would be pulled off (it's a confused deputy attack), and basically it's really only an issue if you're trying to run Iceball as root on a system where you've given a user account to some dodgy bastard. From my understanding it's a rather hard exploit to trigger unless you can predict what the next temp filename will be, and then again you kinda have to spam the filesystem with symlinks at just the right moment.

Basically, if someone can get into your system to use Iceball to exploit this issue, they can probably fuck you over without using Iceball.

To be blunt, it's not worth the effort to work around such an obscure exploit. The only case I can think of would be in a dedicated server, but that should eventually be tweaked to not depend on sackit, and thus we won't need this tempnam() thing for the dedi anyway - we'd just load .it files as if they were general binary files (hidden behind a userdata thing, of course).

iamgreaser commented 9 years ago

This is no longer used in the dedi version, and thus really isn't an issue anymore. Closing.