Menu

#153 Insecure temp file creation vulnerability

closed-fixed
None
5
2004-12-06
2004-11-09
Anonymous
No

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.

Discussion

  • Hans-Bernhard Broeker

    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?

     
  • Jeremy Bae

    Jeremy Bae - 2004-11-10

    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

     
  • Hans-Bernhard Broeker

    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).

     
  • Jason Duell

    Jason Duell - 2004-11-11

    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().

     
  • Hans-Bernhard Broeker

    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.

     
  • Neil Horman

    Neil Horman - 2004-11-24

    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.

     
  • Hans-Bernhard Broeker

    • assigned_to: nobody --> broeker
     
  • Hans-Bernhard Broeker

    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.

     
  • Neil Horman

    Neil Horman - 2004-11-27

    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.

     
  • Hans-Bernhard Broeker

    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).

     
  • Neil Horman

    Neil Horman - 2004-11-30

    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) {
    =====================================

     
  • Neil Horman

    Neil Horman - 2004-12-06

    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.

     
  • Hans-Bernhard Broeker

    • status: open --> closed-fixed
     

Log in to post a comment.