Harden create_temp_filename()

PATCH In review David Sommerseth 3 participants 8 messages Apr 16, 2010 – Apr 18, 2010

lore

Share & tools

lore

David Sommerseth

author

2010-04-16 20:21 UTC #0 lore raw

v2 cover Harden create_temp_filename()

From: David Sommerseth <[email protected]...>

In a Debian bug report [1] there were worries that the --client-connect script hook was prune to a "symlink" attack. Even though this can be recognised if --tmp-dir is set to a world writable directory, it is not considered standard practice to do so.

This patch-set replaces the previous suggested patch, with an enhancement suggested by Fabian Knittel. In addition create_temp_filename() is renamed to create_temp_file() to reflect the behvioural change in the function.

kind regards,

David Sommerseth

[1] <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=534908>

*** BLURB HERE ***

Fabian Knittel and David Sommerseth (1):
Harden create_temp_filename() (version 2)

David Sommerseth (2):
Renamed all calls to create_temp_filename() Updated the man page to reflect the behavioural change of
create_temp_file()

 init.c    |    2 +-
 misc.c    |   60 ++++++++++++++++++++++++++++++++++++++++++++++--------------
 misc.h    |    4 ++--
 multi.c   |   19 ++++++++++++++-----
 openvpn.8 |    2 +-
 pf.c      |   33 +++++++++++++++++----------------
 ssl.c     |   36 +++++++++++++++++++++---------------
 7 files changed, 102 insertions(+), 54 deletions(-)

David Sommerseth

author

2010-04-16 20:21 UTC #1 lore raw

PATCH v2 1/3 Harden create_temp_filename() (version 2)

From: David Sommerseth <[email protected]...>

By hardening the create_temp_filename() function to check if the generated filename exists and to create the temp file with only S_IRUSR|S_IWUSR bit files set before calling the script, it should become even more difficult to exploit such a scenario.

After a discussion on the mailing list, Fabian Knittel provided an enhanced version of the inital patch which is added to this patch.

This patch also renames create_temp_filename() to create_temp_file(), as this patch also creates the temporary file. The function returns the filename of the created file, or NULL on error.

Signed-off-by David Sommerseth <[email protected]...>

Signed-off-by Fabian Knittel <[email protected]...>

---
 misc.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++--------------
 misc.h |    4 ++--
 2 files changed, 48 insertions(+), 16 deletions(-)

misc.c +46 ~~-14~~

index b52a042..643ffad 100644
@@ -1165,25 +1165,57 @@ test_file (const char *filename)
 
 /* create a temporary filename in directory */
 const char *
-create_temp_filename (const char *directory, const char *prefix, struct gc_arena *gc)
+create_temp_file (const char *directory, const char *prefix, struct gc_arena *gc)
 {
   static unsigned int counter;
   struct buffer fname = alloc_buf_gc (256, gc);
+  int fd;
+  const char *retfname = NULL;
+  unsigned int attempts = 0;
 
-  mutex_lock_static (L_CREATE_TEMP);
-  ++counter;
-  mutex_unlock_static (L_CREATE_TEMP);
-
-  {
-    uint8_t rndbytes[16];
-    const char *rndstr;
-
-    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);
-  }
+  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);
 
-  return gen_path (directory, BSTR (&fname), gc);
+  msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+  return NULL;
 }
 
 /*

misc.h +2 -2

index bf51e89..e3ce19e 100644
@@ -218,8 +218,8 @@ long int get_random(void);
 /* return true if filename can be opened for read */
 bool test_file (const char *filename);
 
-/* create a temporary filename in directory */
-const char *create_temp_filename (const char *directory, const char *prefix, struct gc_arena *gc);
+/* create a temporary file in directory, returns the filename of the created file */
+const char *create_temp_file (const char *directory, const char *prefix, struct gc_arena *gc);
 
 /* put a directory and filename together */
 const char *gen_path (const char *directory, const char *filename, struct gc_arena *gc);

Signature · 1 line

1.6.6.1

Fabian Knittel

2010-04-16 21:42 UTC #2 lore raw

Re: [Openvpn-devel] [PATCHv2 1/3] Harden create_temp_filename() (version 2)

Hi David,

misc.c test_file (const char *filename) · line 1219

Quoted 7 lines · David Sommerseth

misc.c +4 -1

+    }
+  while (attempts < 6);
 
-  return gen_path (directory, BSTR (&fname), gc);
+  msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+  return NULL;
 }

I noticed something else ... if - hypothetically - someone manages to guess our file names 5 times in a row, we abort the OpenVPN process. Maybe that's a bit drastic?

If I understand M_FATAL correctly, msg() doesn't even return. So effectively, the create_temp_file() function never returns NULL, because all error cases are currently fatal.

As you've apparently prepared the code for the NULL case in your 2/3 patch, I would suggest a non-fatal error code for at least the last case.

Cheers
Fabian

David Sommerseth

author

2010-04-18 13:21 UTC #3 lore raw

Re: [Openvpn-devel] [PATCHv2 1/3] Harden create_temp_filename() (version 2)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 15 lines · Fabian Knittel

Hi David,
misc.c test_file (const char *filename) · line 1219
Quoted 7 lines · David Sommerseth
misc.c +4 -1
+    }
+  while (attempts < 6);
 
-  return gen_path (directory, BSTR (&fname), gc);
+  msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+  return NULL;
 }
I noticed something else ... if - hypothetically - someone manages to guess our file names 5 times in a row, we abort the OpenVPN process. Maybe that's a bit drastic?

Quoted 6 lines

If I understand M_FATAL correctly, msg() doesn't even return. So effectively, the create_temp_file() function never returns NULL, because all error cases are currently fatal.
As you've apparently prepared the code for the NULL case in your 2/3 patch, I would suggest a non-fatal error code for at least the last case.

(whoops, I seem to have overseen this one yesterday)

Good points, I actually didn't think about it, that the M_FATAL causes a shutdown of OpenVPN ... but I'm just wondering what would be the really proper solution though. And there are three places needs to be considered separately.

* msg (M_FATAL, "Failed to create temporary filename and path"); This indicates that putting together the pieces for a full path failed. The reason of failure could be due to wrong input data, basically.
This one should be a normal warning message (M_WARN) and not M_FATAL.

* msg (M_FATAL, "Could not create temporary file '%s': %s",
* retfname, strerror_ts (errno, &gcerr));
This one kicks in on filesystem errors only, when the open() call fails.
This could be both M_FATAL (if you consider the features which depends
on temp files) and M_WARN or M_NONFATAL (because the calling function should do decide what to do when this fails). I'm leaning towards dropping M_FATAL here. Not sure if M_WARN or M_NONFATAL is appropriate here, but leaning towards M_NONFATAL.

* msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
This is in the very end of the function. Doing M_FATAL with shutdown, is the safest situation - as it closes down a service which obviously struggles with some sever issues.

By not doing M_FATAL, but M_WARN or M_NONFATAL instead, it is up to the calling function to decide what to do. I would still think that in this case, should still be M_FATAL. Just because this is a very extra ordinary situation which indicates that something is crucially wrong on the system.

It could indicate that someone is able to read the memory and try to "hijack" the process by creating its own file. But a more reasonable situation could be thay the random string is not random any more, and it provides the same file name over and over again.

Any other thoughts?

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvLB1wACgkQDC186MBRfrrNDwCfTM4tb206N0idGrcAurqibkwf 7RQAnj8Jey99RvuGvTJKi72d38vZv0MN
=/qq6
-----END PGP SIGNATURE-----

David Sommerseth

author

2010-04-16 20:21 UTC #4 lore raw

PATCH v2 2/3 Renamed all calls to create_temp_filename()

In reply to David Sommerseth

From: David Sommerseth <[email protected]...>

All places where create_temp_filename() was called are now calling create_temp_file(). Extra checks on the result of create_temp_file() is added in addition.

Signed-off-by David Sommerseth <[email protected]...>

---
 init.c  |    2 +-
 multi.c |   19 ++++++++++++++-----
 pf.c    |   33 +++++++++++++++++----------------
 ssl.c   |   36 +++++++++++++++++++++---------------
 4 files changed, 53 insertions(+), 37 deletions(-)

init.c +1 -1

index 3748c2e..e2f7729 100644
@@ -457,7 +457,7 @@ init_static (void)
 #ifdef STATUS_PRINTF_TEST
   {
     struct gc_arena gc = gc_new ();
-    const char *tmp_file = create_temp_filename ("/tmp", "foo", &gc);
+    const char *tmp_file = create_temp_file ("/tmp", "foo", &gc);
     struct status_output *so = status_open (tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
     status_printf (so, "%s", "foo");
     status_printf (so, "%s", "bar");

multi.c +14 -5

index 2b04428..826a113 100644
@@ -1530,7 +1530,13 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
       if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
 	{
 	  struct argv argv = argv_new ();
-	  const char *dc_file = create_temp_filename (mi->context.options.tmp_dir, "cc", &gc);
+	  const char *dc_file = create_temp_file (mi->context.options.tmp_dir, "cc", &gc);
+
+          if( !dc_file ) {
+            cc_succeeded = false;
+            goto script_depr_failed;
+          }
+
 	  argv_printf (&argv, "%s", dc_file);
 	  delete_file (dc_file);
 	  if (plugin_call (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT, &argv, NULL, mi->context.c2.es) != OPENVPN_PLUGIN_FUNC_SUCCESS)

@@ -1543,6 +1549,7 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
 	      multi_client_connect_post (m, mi, dc_file, option_permissions_mask, &option_types_found);
 	      ++cc_succeeded_count;
 	    }
+        script_depr_failed:
 	  argv_reset (&argv);
 	}

@@ -1578,9 +1585,11 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
 
 	  setenv_str (mi->context.c2.es, "script_type", "client-connect");
 
-	  dc_file = create_temp_filename (mi->context.options.tmp_dir, "cc", &gc);
-
-	  delete_file (dc_file);
+	  dc_file = create_temp_file (mi->context.options.tmp_dir, "cc", &gc);
+          if( !dc_file ) {
+            cc_succeeded = false;
+            goto script_failed;
+          }
 
 	  argv_printf (&argv, "%sc %s",
 		       mi->context.options.client_connect_script,

@@ -1593,7 +1602,7 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
 	    }
 	  else
 	    cc_succeeded = false;
-
+        script_failed:
 	  argv_reset (&argv);
 	}

pf.c +17 ~~-16~~

index 027eb69..5075164 100644
@@ -554,24 +554,25 @@ pf_init_context (struct context *c)
 #ifdef PLUGIN_PF
   if (plugin_defined (c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
     {
-      const char *pf_file = create_temp_filename (c->options.tmp_dir, "pf", &gc);
-      delete_file (pf_file);
-      setenv_str (c->c2.es, "pf_file", pf_file);
-
-      if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
-	{
-	  event_timeout_init (&c->c2.pf.reload, 1, now);
-	  c->c2.pf.filename = string_alloc (pf_file, NULL);
-	  c->c2.pf.enabled = true;
+      const char *pf_file = create_temp_file (c->options.tmp_dir, "pf", &gc);
+      if( pf_file ) {
+        setenv_str (c->c2.es, "pf_file", pf_file);
+
+        if (plugin_call (c->plugins, OPENVPN_PLUGIN_ENABLE_PF, NULL, NULL, c->c2.es) == OPENVPN_PLUGIN_FUNC_SUCCESS)
+          {
+            event_timeout_init (&c->c2.pf.reload, 1, now);
+            c->c2.pf.filename = string_alloc (pf_file, NULL);
+            c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
-	  if (check_debug_level (D_PF_DEBUG))
-	    pf_context_print (&c->c2.pf, "pf_init_context#1", D_PF_DEBUG);
+            if (check_debug_level (D_PF_DEBUG))
+              pf_context_print (&c->c2.pf, "pf_init_context#1", D_PF_DEBUG);
 #endif
-	}
-      else
-	{
-	  msg (M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-	}
+          }
+        else
+          {
+            msg (M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
+          }
+      }
     }
 #endif
 #ifdef MANAGEMENT_PF

ssl.c +21 ~~-15~~

index ddd5ee7..552bcbe 100644
@@ -1094,10 +1094,11 @@ key_state_gen_auth_control_file (struct key_state *ks, const struct tls_options
   const char *acf;
 
   key_state_rm_auth_control_file (ks);
-  acf = create_temp_filename (opt->tmp_dir, "acf", &gc);
-  ks->auth_control_file = string_alloc (acf, NULL);
-  setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
-
+  acf = create_temp_file (opt->tmp_dir, "acf", &gc);
+  if( acf ) {
+    ks->auth_control_file = string_alloc (acf, NULL);
+    setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
+  } /* FIXME: Should have better error handling? */
   gc_free (&gc);					  
 }

@@ -3144,17 +3145,22 @@ verify_user_pass_script (struct tls_session *session, const struct user_pass *up
 	{
 	  struct status_output *so;
 
-	  tmp_file = create_temp_filename (session->opt->tmp_dir, "up", &gc);
-	  so = status_open (tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
-	  status_printf (so, "%s", up->username);
-	  status_printf (so, "%s", up->password);
-	  if (!status_close (so))
-	    {
-	      msg (D_TLS_ERRORS, "TLS Auth Error: could not write username/password to file: %s",
-		   tmp_file);
-	      goto done;
-	    }
-	}
+	  tmp_file = create_temp_file (session->opt->tmp_dir, "up", &gc);
+          if( tmp_file ) {
+            so = status_open (tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
+            status_printf (so, "%s", up->username);
+            status_printf (so, "%s", up->password);
+            if (!status_close (so))
+              {
+                msg (D_TLS_ERRORS, "TLS Auth Error: could not write username/password to file: %s",
+                     tmp_file);
+                goto done;
+              }
+          } else {
+            msg (D_TLS_ERRORS, "TLS Auth Error: could not create write "
+                 "username/password to temp file");
+          }
+        }
       else
 	{
 	  setenv_str (session->opt->es, "username", up->username);

Signature · 1 line

1.6.6.1

Fabian Knittel

2010-04-16 21:37 UTC #5 lore raw

Re: [Openvpn-devel] [PATCHv2 2/3] Renamed all calls to create_temp_filename()

Hi David,

Quoted 9 lines · David Sommerseth

multi.c +0 -1

index 2b04428..826a113 100644
@@ -1530,7 +1530,13 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
       if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
 	{
 	  struct argv argv = argv_new ();
-	  const char *dc_file = create_temp_filename (mi->context.options.tmp_dir, "cc", &gc);

multi.c multi_connection_established (struct multi_context *m, struct multi_instance *mi · line 1542

Quoted 10 lines

multi.c +7 -0

+	  const char *dc_file = create_temp_file (mi->context.options.tmp_dir, "cc", &gc);
+
+          if( !dc_file ) {
+            cc_succeeded = false;
+            goto script_depr_failed;
+          }
+
 	  argv_printf (&argv, "%s", dc_file);
 	  delete_file (dc_file);
 	  if (plugin_call (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT, &argv, NULL, mi->context.c2.es) != OPENVPN_PLUGIN_FUNC_SUCCESS)

You'll probably want to remove the remaining delete_file, right? :)

[...]

Quoted 10 lines

ssl.c +0 -2

index ddd5ee7..552bcbe 100644
@@ -1094,10 +1094,11 @@ key_state_gen_auth_control_file (struct key_state *ks, const struct tls_options
   const char *acf;
 
   key_state_rm_auth_control_file (ks);
-  acf = create_temp_filename (opt->tmp_dir, "acf", &gc);
-  ks->auth_control_file = string_alloc (acf, NULL);

ssl.c key_state_gen_auth_control_file (struct key_state *ks, const struct tls_options · line 1104

Quoted 10 lines

ssl.c +5 -2

-  setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
-
+  acf = create_temp_file (opt->tmp_dir, "acf", &gc);
+  if( acf ) {
+    ks->auth_control_file = string_alloc (acf, NULL);
+    setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
+  } /* FIXME: Should have better error handling? */
   gc_free (&gc);					  
 }

I just noticed your FIXME. With the above change, the client auth plugin would have the chance to notice that there's no "auth_control_file" environment variable and error out. It was already logged that the temp file creation failed, so it should be clear to the admin why it happened.

An improvement might be to ensure that verify_user_pass_plugin() disallows the plugin from doing OPENVPN_PLUGIN_FUNC_DEFERRED if there's no auth_control_file. (The function could simply check the return value and force FAILURE.)

Cheers
Fabian

David Sommerseth

author

2010-04-16 20:21 UTC #6 lore raw

PATCH v2 3/3 Updated the man page to reflect the behavioural change of create_temp_file()

In reply to David Sommerseth

From: David Sommerseth <[email protected]...>

As this function now creates the temp file, it is no longer 'not-yet-created', but 'freshly created'.

Signed-off-by David Sommerseth <[email protected]...>

---
 openvpn.8 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

openvpn.8 +1 -1

index 82e7d24..301cd37 100644
@@ -2814,7 +2814,7 @@ on client connection.  The script is passed the common name
 and IP address of the just-authenticated client
 as environmental variables (see environmental variable section
 below).  The script is also passed
-the pathname of a not-yet-created temporary file as $1
+the pathname of a freshly created temporary file as $1
 (i.e. the first command line argument), to be used by the script
 to pass dynamically generated config file directives back to OpenVPN.

Signature · 1 line

1.6.6.1

Gert Doering

2010-04-18 14:07 UTC #7 lore raw

In reply to David Sommerseth

Hi,

Quoted 8 lines · David Sommerseth

In a Debian bug report [1] there were worries that the --client-connect script hook was prune to a "symlink" attack. Even though this can be recognised if --tmp-dir is set to a world writable directory, it is not considered standard practice to do so.
This patch-set replaces the previous suggested patch, with an enhancement suggested by Fabian Knittel. In addition create_temp_filename() is renamed to create_temp_file() to reflect the behvioural change in the function.

Overall, this looks good to me (with the additional changes by Fabian and you). So ACK.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...