I'm Jeremy Bae at STG Security, Inc.
(swbae@stgsecurity.com).
main.c 332 line
/* create the temporary file names */
pid = getpid();
(void) sprintf(temp1, "%s/cscope%d.1", tmpdir, pid);
(void) sprintf(temp2, "%s/cscope%d.2", tmpdir, pid);
temporary files created with predictable names.
/tmp/cscope[pid].1
/tmp/cscope[pid].2
If root uses cscope, and malicious user make symbolic
link to other file (/etc/shadow?), he can corrupt system
file.
if /tmp/cscope[pid].1 -> /etc/shadow
/etc/shadow would be corrupted.
Logged In: YES
user_id=27517
Two replies:
1) root is definitely not supposed to be running this
program. A person running C development tools under the
root account deserves whatever he'll get.
2) While I understand the problem, I'm not aware of any
truly good solution that would work well across a range of
platforms as wide as cscope (pretty much all existing
Unices, plus Cygwin and DJGPP). Care to help out on that end?
Logged In: YES
user_id=1155367
I agree 1).
2) This bug is the issue of multi user unix(cygwin, djgpp are
not).
How about create temp file on user home directory?
$HOME/.cscope/cscope[pid].1
$HOME/.cscope/cscope[pid].2
if you create /home/[foo]/.cscope/ with permission 700,
it will be secure enough.
cf. http://www.dwheeler.com/secure-programs/Secure-
Programs-HOWTO/avoid-race.html#TEMPORARY-FILES
Logged In: YES
user_id=27517
While the bug may only affect true multi-user platforms,
what I was getting at was that the solution, whatever it is,
must be usable on all of them.
I'm against putting temporary files inside users' home
directories --- only "important" stuff should really ever go
there (not to mention some platforms don't even have a $HOME
or user name in the first place).
I'm afraid all most none of the solutions suggested on that
very helful web page can be applied for cscope, because of
the way it shares temp files by name between subprocesses
and cscope itself (e.g. see how sortcommand is constructed
in build(), build.c:446).
Logged In: YES
user_id=125727
The trivial fix here is to open() the temporary file with
O_EXCL, so that it fails if the file already exists. This
gets rid of the /etc/shadow problem.
Any files we open in /tmp should be opened this way, to
avoid the security problem described. A quick glance at the
source reveals that we're using such files in more places
than just here.
More generally, to be "polite" we ought to use mkstemp(), if
it's available, in order to honor whatever mechanism is used
for temp files on a system. There's also tmpnam(), if
mkstemp isn't available, though you've got to call it in a
loop (it gives you a filename that's not in use, but there's
a race between calling it and creating the file) and with
the O_EXCL flag passed manually. Finally, there's
tmpfile(), but it doesn't let you know the name of the file
it gives you.
Note that some of these functions return int file
descriptors rather than FILE *'s. We can convert the int
file descriptor to FILEs via fdopen().
I recommend we implement a 'mymkstemp()' function and audit
the code to see where we need to be calling it instead of
our current strategy of slapping together a filename on the
fly and then calling myfopen().
Logged In: YES
user_id=27517
That trivial fix won't do us any good. As I already pointed
out, we construct at least one shell command line using
redirection to temp1, the name of one of cscope's two temp
files. We also repeatedly close and reopen the file internally.
In short, changing the way we open the file would solve only
one small aspect of the problem, leaving the majority of
them untouched. There's quite a lot more to this problem
than that we myfopen() these temp files instead of using
open() with an O_EXCL flag.
Logged In: YES
user_id=827328
what if we simply stat the files before we create them? If
they exist, and they're a symlink, we remove the link before
creating the file.
Logged In: YES
user_id=27517
stat()-before-creat() doesn't help. It just shortens the
time span an attacker has to win the race, but doesn't make
it go away. O_EXCL is, essentially, the only working answer
to that aspect of the problem. Our whistleblowers from
'rexotec' believe fopen() mode "w+bx" would fix that --- but
I don't think this 'x' mode bit exists in any other fopen()
than glibc's.
And it doesn't even begin to address the implementation of
the ^ command, which passes a temp file to a sub-shell by
name, for redirection. There's no way that one can be fixed
--- it has to be torn out and re-implemented from scratch.
Logged In: YES
user_id=827328
What about a fix like this?
=============================
--- cscope-15.5/src/mypopen.c.orig 2004-11-25
12:02:18.000000000 -0500
+++ cscope-15.5/src/mypopen.c 2004-11-26
11:41:05.362541248 -0500
@@ -99,8 +99,27 @@
{
/* opens a file pointer and then sets close-on-exec
for the file */
FILE *fp;
+ char *tmpdir;
- fp = fopen(path, mode);
+ /*
+ *NH: Check here if we're opening either of the per
process
+ *temp files. if we are, check to see if the file
exists. If
+ *it does, remove it to avoid the security implications
+ */
+ tmpdir = mygetenv("TMPDIR",NULL);
+ if((tmpdir != NULL) && (strstr(path,tmpdir) != NULL)) {
+ /*
+ *we are trying to open a file in the tmp
directory
+ *we should make sure the file doesn't already
+ *exist
+ */
+ int fd = open(path,O_CREAT|O_EXCL,O_RDWR);
+ if(fd < 0 )
+ return NULL;
+ fp = fdopen(fd,mode);
+ } else {
+ fp = fopen(path, mode);
+ }
#ifdef SETMODE
if (fp && ! strchr(mode, 'b')) {
==========================================
It lets us use myfopen on the temp files, doesn't appear to
have any races, and I think it works on all the currently
tested platoforms.
Logged In: YES
user_id=27517
That patch would solve the fopen() aspect of the problem,
but in a way I'm not completely happy with:
* Testing for equality with TMPDIR feels fragile.
* I think we really shouldn't try to re-invent mkstemp() for
ourselves, here.
* Or if we do, don't do it in myfopen(), but in a
(to-be-written) mymkstemp().
* And what are we to do in case of a failure of a creat() in
O_EXCL mode? Is a simple failure message enough, or should
we raise more of a fuzz, flagging a possible attempted attack?
And we still don't have even a first hint at a solution for
the shell redirection issue (to file temp2, used by the '^'
command).
Logged In: YES
user_id=827328
Heres a better idea. How about instead of just creating two
well known names for the cscope temp files, we create a
directory in $TMPDIR, and create it with permissions such
that no other user can access the files. That keeps access
to the temp files safe, regardless of our access semantics.
Then we just store the directory name, and remove it when
we shut down. Heres, the patch (also attached on the
patches page). It keeps a consistent directory name (based
on the pid of the process), but that could be randomized if
need be.
=======================
--- cscope-15.5/src/main.c.orig 2004-11-30
10:47:56.000000000 -0500
+++ cscope-15.5/src/main.c 2004-11-30
12:58:13.000000000 -0500
@@ -102,6 +102,7 @@
#endif
char temp1[PATHLEN + 1]; /* temporary file name */
char temp2[PATHLEN + 1]; /* temporary file name */
+char tempdirpv[PATHLEN +1]; /* private temp directory */
long totalterms; /* total inverted index terms */
BOOL trun_syms; /* truncate symbols to 8
characters */
char tempstring[8192]; /* use this as a buffer,
instead of 'yytext',
@@ -138,6 +139,7 @@
pid_t pid;
struct stat stat_buf;
struct sigaction winch_action;
+ mode_t orig_umask;
yyin = stdin;
yyout = stdout;
@@ -344,9 +346,18 @@
}
/* create the temporary file names */
+ orig_umask = umask(S_IRWXG|S_IRWXO);
pid = getpid();
- (void) sprintf(temp1, "%s/cscope%d.1", tmpdir, pid);
- (void) sprintf(temp2, "%s/cscope%d.2", tmpdir, pid);
+ (void) sprintf(tempdirpv, "%s/cscope.%d", tmpdir, pid);
+ if(mkdir(tempdirpv,S_IRWXU))
+ {
+ fprintf(stderr, "cscope: Could not create
private temp dir %s\n",tempdirpv);
+ myexit(1);
+ }
+ umask(orig_umask);
+
+ (void) sprintf(temp1, "%s/cscope.1", tempdirpv, pid);
+ (void) sprintf(temp2, "%s/cscope.2", tempdirpv, pid);
/* if running in the foreground */
if (signal(SIGINT, SIG_IGN) != SIG_IGN) {
@@ -848,6 +859,7 @@
if (temp1[0] != '\0') {
(void) unlink(temp1);
(void) unlink(temp2);
+ (void) rmdir(tempdirpv);
}
/* restore the terminal to its original mode */
if (incurses == YES) {
=====================================
Logged In: YES
user_id=827328
The patch which fixes this bug, supplied in tracker number
1074268 on the patches page is checked into CVS.