Harden create_temp_filename()

As promised in the meeting today, a patch for hardening create_temp_filename().

I've added more checks to what create_temp_filename() returns where it is called in addition, to make it even safer.

misc.c +12 -0

+  do {
     uint8_t rndbytes[16];
     const char *rndstr;
 
+    attempts++;
+    mutex_lock_static (L_CREATE_TEMP);
+    ++counter;
+    mutex_unlock_static (L_CREATE_TEMP);
+
     prng_bytes (rndbytes, sizeof (rndbytes));
     rndstr = format_hex_ex (rndbytes, sizeof (rndbytes), 40, 0, NULL, gc);
     buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);
+
+    retfname = gen_path (directory, BSTR (&fname), gc);
+    if( !retfname ) {
+      msg (M_FATAL, "Failed to create temporary filename and path");
+      return NULL;
+    }

Hi David,

Quoted 2 lines · David Sommerseth

As promised in the meeting today, a patch for hardening create_temp_filename().

Great! :)

Quoted 21 lines

I've added more checks to what create_temp_filename() returns where it is called in addition, to make it even safer.

misc.c +12 -0

+  do {
     uint8_t rndbytes[16];
     const char *rndstr;
 
+    attempts++;
+    mutex_lock_static (L_CREATE_TEMP);
+    ++counter;
+    mutex_unlock_static (L_CREATE_TEMP);
+
     prng_bytes (rndbytes, sizeof (rndbytes));
     rndstr = format_hex_ex (rndbytes, sizeof (rndbytes), 40, 0, NULL, gc);
     buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);
+
+    retfname = gen_path (directory, BSTR (&fname), gc);
+    if( !retfname ) {
+      msg (M_FATAL, "Failed to create temporary filename and path");
+      return NULL;
+    }

misc.c create_temp_filename (const char *directory, const char *prefix, struct gc_arena · line 1203

Quoted 10 lines

misc.c +10 -0

+  } while ( test_file (retfname) && (attempts < 6) );
+
+  if( attempts > 5 ) {
+    msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+    return NULL;
+  }
+
+  /* Create the tempfile to avoid a race condition */
+  fd = creat (retfname, S_IRUSR | S_IWUSR);
+  if( fd == -1 ) {

You'll probably want to add O_EXCL to creat()'s mode flags:

| O_EXCL Ensure that this call creates the file: if this flag is
speci‐
| fied in conjunction with O_CREAT, and pathname already
exists,
| then open() will fail. The behavior of O_EXCL is
undefined if
| O_CREAT is not specified.

Otherwise, there would be a race-condition between test_file() and creat() and someone could create a symlink under you feet.

And as the important "does it exist?" test thereby moves to creat(), I would like to suggest that you move the creat() call into the loop. The
exit condition would then be "successful creat()" or "max attempts".

PS: Is there some kind of code style guide for OpenVPN? I notice that
you're using a different style than I expected to see...

Quoted 49 lines · Fabian Knittel

Hi David,

Quoted 2 lines · David Sommerseth

As promised in the meeting today, a patch for hardening create_temp_filename().

Great! :)

Quoted 21 lines

I've added more checks to what create_temp_filename() returns where it is called in addition, to make it even safer.

misc.c +12 -0

+  do {
     uint8_t rndbytes[16];
     const char *rndstr;
 
+    attempts++;
+    mutex_lock_static (L_CREATE_TEMP);
+    ++counter;
+    mutex_unlock_static (L_CREATE_TEMP);
+
     prng_bytes (rndbytes, sizeof (rndbytes));
     rndstr = format_hex_ex (rndbytes, sizeof (rndbytes), 40, 0, NULL, gc);
     buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);
+
+    retfname = gen_path (directory, BSTR (&fname), gc);
+    if( !retfname ) {
+      msg (M_FATAL, "Failed to create temporary filename and path");
+      return NULL;
+    }

misc.c create_temp_filename (const char *directory, const char *prefix, struct gc_arena · line 1203

Quoted 10 lines

misc.c +10 -0

+  } while ( test_file (retfname) && (attempts < 6) );
+
+  if( attempts > 5 ) {
+    msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+    return NULL;
+  }
+
+  /* Create the tempfile to avoid a race condition */
+  fd = creat (retfname, S_IRUSR | S_IWUSR);
+  if( fd == -1 ) {

You'll probably want to add O_EXCL to creat()'s mode flags:

| O_EXCL Ensure that this call creates the file: if this flag is
speci
| fied in conjunction with O_CREAT, and pathname already
exists,
| then open() will fail. The behavior of O_EXCL is
undefined if
| O_CREAT is not specified.

Quoted 2 lines

Otherwise, there would be a race-condition between test_file() and creat() and someone could create a symlink under you feet.

I honestly doubt that O_EXCL would help much, as the file will be closed immediately.

I'll look more into this, as the only advantage is that if open() with O_EXCL|O_CREAT fails if the file exists, it should be used instead.

Quoted 2 lines · David Sommerseth

I'll look more into this, as the only advantage is that if open() with O_EXCL|O_CREAT fails if the file exists, it should be used instead.

Unfortunately, this won't help against symlink attacks directed to non-existant files (like "-> /etc/nologin").

It *will* protect against symlink attacks to existing files (overwriting /etc/passwd or something similarily nasty).

Quoted 2 lines

Unfortunately, this won't help against symlink attacks directed to non-existant files (like "-> /etc/nologin").

"If O_EXCL and O_CREAT are set, and path names a symbolic link, open() shall fail and set errno to [EEXIST], regardless of the contents of the symbolic link."

Hi,

Quoted 2 lines · David Sommerseth

I'll look more into this, as the only advantage is that if open() with O_EXCL|O_CREAT fails if the file exists, it should be used instead.

Unfortunately, this won't help against symlink attacks directed to non-existant files (like "-> /etc/nologin").

It *will* protect against symlink attacks to existing files (overwriting /etc/passwd or something similarily nasty).

Quoted 3 lines · Gert Doering

Hi,

On Fri, Apr 16, 2010 at 11:16:32AM +0200, David Sommerseth wrote:

Quoted 4 lines

Quoted 2 lines

I'll look more into this, as the only advantage is that if open() with O_EXCL|O_CREAT fails if the file exists, it should be used instead.

Unfortunately, this won't help against symlink attacks directed to non-existant files (like "-> /etc/nologin").

That's right, this could create a local DoS. I'm going to have a more careful look at test_file() afterwards. Considering to make it use stat() instead of just trying to open the file for reading.

do

  {
    uint8_t rndbytes[16];
    const char *rndstr;

    ++attempts;
    mutex_lock_static (L_CREATE_TEMP);
    ++counter;
    mutex_unlock_static (L_CREATE_TEMP);

    prng_bytes (rndbytes, sizeof rndbytes);
    rndstr = format_hex_ex (rndbytes, sizeof rndbytes, 40, 0, NULL, gc);
    buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);

    retfname = gen_path (directory, BSTR (&fname), gc);
    if (!retfname)
      {
        msg (M_FATAL, "Failed to create temporary filename and path");
        return NULL;
      }

    /* Atomically create the file.  Errors out if the file already
       exists.  */
    fd = open (retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
    if (fd != -1)
      {
        close (fd);
        return retfname;
      }
    else if (fd == -1 && errno != EEXIST)
      {
        /* Something else went wrong, no need to retry.  */
        struct gc_arena gcerr = gc_new ();
        msg (M_FATAL, "Could not create temporary file '%s': %s",
             retfname, strerror_ts (errno, &gcerr));
        gc_free (&gcerr);
        return NULL;
      }

}
while (attempts < 6);

msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts); return NULL;

I've dived into the kernel code to see what it *really* does (when the man page are so unclear), and it should behave as those other Unices does as well. So, O_EXCL do make sense to avoid overwriting existing files if it is a symlink to an existing file.

O_EXCL Ensure that this call creates the file: if this flag is speci‐
fied in conjunction with O_CREAT, and pathname already exists, then open() will fail. The behavior of O_EXCL is undefined if O_CREAT is not specified.

When these two flags are specified, symbolic links are not fol‐
lowed: if pathname is a symbolic link, then open() fails regard‐ less of where the symbolic link points to.

O_EXCL is only supported on NFS when using NFSv3 or later on kernel 2.6 or later. In environments where NFS O_EXCL support is not provided, programs that rely on it for performing locking
tasks will contain a race condition. Portable programs that
want to perform atomic file locking using a lockfile, and need to avoid reliance on NFS support for O_EXCL, can create a unique file on the same file system (e.g., incorporating hostname and
PID), and use link(2) to make a link to the lockfile. If
link(2) returns 0, the lock is successful. Otherwise, use stat(2) on the unique file to check if its link count has increased to 2, in which case the lock is also successful.

Btw ... When diving into the kernel code, I stumbled upon this comment in fs/namei.c:1872:

	/* Does someone understand code flow here? Or it is only
	 * me so stupid? Anathema to whoever designed this non-sense
	 * with "intent.open".
	 */

Thought that one was worth sharing ;-)

Hi David,

David Sommerseth wrote:
(BTW, I thought creat() took a flags parameter, but it only takes a mode param. My mistake. So you're correct in wanting to use open() instead of creat().)

To let the code speak, here's my suggestion (not compile-tested):

Quoted 43 lines

do

  {
    uint8_t rndbytes[16];
    const char *rndstr;

    ++attempts;
    mutex_lock_static (L_CREATE_TEMP);
    ++counter;
    mutex_unlock_static (L_CREATE_TEMP);

    prng_bytes (rndbytes, sizeof rndbytes);
    rndstr = format_hex_ex (rndbytes, sizeof rndbytes, 40, 0, NULL, gc);
    buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);

    retfname = gen_path (directory, BSTR (&fname), gc);
    if (!retfname)
      {
        msg (M_FATAL, "Failed to create temporary filename and path");
        return NULL;
      }

    /* Atomically create the file.  Errors out if the file already
       exists.  */
    fd = open (retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
    if (fd != -1)
      {
        close (fd);
        return retfname;
      }
    else if (fd == -1 && errno != EEXIST)
      {
        /* Something else went wrong, no need to retry.  */
        struct gc_arena gcerr = gc_new ();
        msg (M_FATAL, "Could not create temporary file '%s': %s",
             retfname, strerror_ts (errno, &gcerr));
        gc_free (&gcerr);
        return NULL;
      }

}
while (attempts < 6);

msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts); return NULL;

(Regarding the code style: The above uses what I had thought would be the current style, but as I've only read parts of the OpenVPN code, I'm likely to be wrong. Are there any code-parts / newer code-parts which should be considered reference material?)

Quoted 4 lines

I've dived into the kernel code to see what it *really* does (when the man page are so unclear), and it should behave as those other Unices does as well. So, O_EXCL do make sense to avoid overwriting existing files if it is a symlink to an existing file.

IMHO the Linux man-page ist quite clear (I'm using version 3.24):

Quoted 11 lines

O_EXCL Ensure that this call creates the file: if this flag is speci‐
fied in conjunction with O_CREAT, and pathname already exists, then open() will fail. The behavior of O_EXCL is undefined if O_CREAT is not specified.

When these two flags are specified, symbolic links are not fol‐
lowed: if pathname is a symbolic link, then open() fails regard‐ less of where the symbolic link points to.

O_EXCL is only supported on NFS when using NFSv3 or later on kernel 2.6 or later.

Quoted 10 lines

In environments where NFS O_EXCL support
is not provided, programs that rely on it for performing locking
tasks will contain a race condition. Portable programs that
want to perform atomic file locking using a lockfile, and need to avoid reliance on NFS support for O_EXCL, can create a unique file on the same file system (e.g., incorporating hostname and
PID), and use link(2) to make a link to the lockfile. If
link(2) returns 0, the lock is successful. Otherwise, use stat(2) on the unique file to check if its link count has increased to 2, in which case the lock is also successful.

Using open with O_CREAT|O_EXCL:
- makes the atomic guarantee, that the file was created by us (or the function call fails)
- does not follow symlinks (if a symlink exists, it fails)
- works over NFS for NFS implementations that support O_EXCL. (This should apply to all current Linux NFS implementations.)

So, anything remaining that you'd like cleared up?