Don't throw fatal errors from create_temp_file()

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/pf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/openvpn/pf.c +0 -0

index 5fe1734..3faaebd 100644

src/openvpn/pf.c +1 -1

         if (pf_file)
         {
+            c->c2.pf.filename = string_alloc(pf_file, &c->c2.gc);
             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, &c->c2.gc);
                 c->c2.pf.enabled = true;
 #ifdef ENABLE_DEBUG
                 if (check_debug_level(D_PF_DEBUG))

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.

 src/openvpn/pf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

src/openvpn/pf.c +0 -0

index 5fe1734..7479347 100644
@@ -618,19 +618,18 @@ pf_load_from_buffer_list(struct context *c, const struct buffer_list *config)
 void

Quoted 17 lines · Steffan Karger

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.

 src/openvpn/pf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

src/openvpn/pf.c +0 -0

index 5fe1734..7479347 100644
@@ -618,19 +618,18 @@ pf_load_from_buffer_list(struct context *c, const struct buffer_list *config)
 void

src/openvpn/pf.c pf_load_from_buffer_list(struct context *c, const struct buffer_list *config) · line 625

Quoted 10 lines

src/openvpn/pf.c +2 -3

 pf_init_context(struct context *c)
 {
-    struct gc_arena gc = gc_new();
 #ifdef PLUGIN_PF
     if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
     {
-        const char *pf_file = create_temp_file(c->options.tmp_dir, "pf", &gc);
-        if (pf_file)
+        c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
+                                             &c->c2.gc);

Patch looks good. But it introduces a new compile warning.

pf.c: In function ‘pf_init_context’:
pf.c:624:27: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]

         c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",

I'm pondering if we need create_temp_file() to actually return a const char * - wouldn't just a plain char * be enough?

The alternative is to cast the const away here; but that just feels too hacky in this code path.

Hi,

Quoted 31 lines · David Sommerseth

Quoted 17 lines · Steffan Karger

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: As suggested by Antionio, get rid of local 'gc' and 'file' vars.

 src/openvpn/pf.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

src/openvpn/pf.c +0 -0

index 5fe1734..7479347 100644
@@ -618,19 +618,18 @@ pf_load_from_buffer_list(struct context *c, const struct buffer_list *config)
 void

src/openvpn/pf.c pf_load_from_buffer_list(struct context *c, const struct buffer_list *config) · line 625

Quoted 10 lines

src/openvpn/pf.c +2 -3

 pf_init_context(struct context *c)
 {
-    struct gc_arena gc = gc_new();
 #ifdef PLUGIN_PF
     if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
     {
-        const char *pf_file = create_temp_file(c->options.tmp_dir, "pf", &gc);
-        if (pf_file)
+        c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
+                                             &c->c2.gc);

Patch looks good. But it introduces a new compile warning.

Quoted 6 lines

pf.c: In function ‘pf_init_context’:
pf.c:624:27: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]

         c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",

I'm pondering if we need create_temp_file() to actually return a const char * - wouldn't just a plain char * be enough?

Good point. But I think it's best to return a const char * here, because the memory is allocated through a gc and therefore should also be free'd through the gc. The const modifier prevents freeing the data without a warning (as long as you keep smacking me for discarding const qualifiers, or we add a -Werror compile flag ;-) ).

Quoted 2 lines

The alternative is to cast the const away here; but that just feels too hacky in this code path.

Or change c->c2.filename to be const char *. I think that's the way to go here. And possibly reject a NULL gc argument in create_temp_file(), because that would make the function leak memory...

What do you think?

If gc == NULL, the data allocated in the alloc_gc_buf() call would never be free'd. The function is never called that way, but let's prevent future problems.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/misc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

src/openvpn/misc.c +0 -0

index 90632706..86f80a6a 100644
@@ -724,10 +724,13 @@ const char *
 create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)

If gc == NULL, the data allocated in the alloc_gc_buf() call in create_temp_file or the string_mod_const call in gen_path would never be free'd.

These functions are currently never called that way, but let's prevent future problems.

While touching create_temp_file, also remove the counter variable, which is never read.

Signed-off-by Steffan Karger <[email protected]...>

---
v2:
- change create_temp_file to avoid using a struct buffer (simpler)
- add gc != NULL check for gen_path (avoid similar memleak pitfall)

 src/openvpn/misc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

src/openvpn/misc.c +1 -2

index 90632706..b6f92526 100644
@@ -723,21 +723,20 @@ test_file(const char *filename)
 const char *
 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;
+    char fname[256] = { 0 };
 

src/openvpn/misc.c +7 -1

-        retfname = gen_path(directory, BSTR(&fname), gc);
+        retfname = gen_path(directory, fname, gc);
         if (!retfname)
         {
             msg(M_WARN, "Failed to create temporary filename and path");

@@ -812,6 +811,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
 #else
     const int CC_PATH_RESERVED = CC_SLASH;
 #endif
+
+    if (!gc)
+    {
+        return NULL; /* Would leak memory otherwise */
+    }
+
     const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
 
     if (safe_filename

This function is called in response to connecting clients, and can fail when I/O fails for some (possibly temporary) reason. In such cases we should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and (where needed) changes the callers to check for and handle errors.

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/misc.c       |  6 +++---
 src/openvpn/pf.c         |  8 ++++----
 src/openvpn/ssl_verify.c | 32 +++++++++++++++++++++-----------
 3 files changed, 28 insertions(+), 18 deletions(-)

src/openvpn/misc.c +3 -3

index 8c7f611..25f3800 100644
@@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
         retfname = gen_path(directory, BSTR(&fname), gc);
         if (!retfname)
         {
-            msg(M_FATAL, "Failed to create temporary filename and path");
+            msg(M_WARN, "Failed to create temporary filename and path");
             return NULL;
         }
 

@@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
         else if (fd == -1 && errno != EEXIST)
         {
             /* Something else went wrong, no need to retry.  */
-            msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
+            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
                 retfname);
             return NULL;
         }
     }
     while (attempts < 6);
 
-    msg(M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
     return NULL;
 }
 

src/openvpn/pf.c +4 -4

index 5cb002b..5fe1734 100644
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
                 }
 #endif
             }
-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }
+        }
+        if (!c->c2.pf.enabled)
+        {
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }
     }
 #endif /* ifdef PLUGIN_PF */

src/openvpn/ssl_verify.c +0 -0

index 9cd36d7..df2736c 100644

src/openvpn/ssl_verify.c +18 -10

     {
+        msg (M_WARN, "Failed to create peer cert file");
         return NULL;
     }
 
-    /* create tmp file to store peer cert */
-    peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
-
     /* write peer-cert in tmp-file */
     peercert_file = fopen(peercert_filename, "w+");
     if (!peercert_file)

@@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
 
     if (verify_export_cert)
     {
-        if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc)))
+        tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc);
+        if (!tmp_file)
         {
-            setenv_str(es, "peer_cert", tmp_file);
+            ret = false;
+            goto cleanup;
         }
+        setenv_str(es, "peer_cert", tmp_file);
     }
 
     argv_parse_cmd(&argv, verify_command);

@@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
         }
     }
 
+cleanup:
     gc_free(&gc);
     argv_reset(&argv);
 

@@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
     }
 }
 
-static void
+static bool
 key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options *opt)
 {
     struct gc_arena gc = gc_new();
-    const char *acf;
 
     key_state_rm_auth_control_file(ks);
-    acf = create_temp_file(opt->tmp_dir, "acf", &gc);
+    const char *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);
+    return acf;
 }
 
 static unsigned int

@@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
 
 #ifdef PLUGIN_DEF_AUTH
         /* generate filename for deferred auth control file */
-        key_state_gen_auth_control_file(ks, session->opt);
+        if (!key_state_gen_auth_control_file(ks, session->opt))
+        {
+            msg (D_TLS_ERRORS, "TLS Auth Error (%s): "
+                 "could not create deferred auth control file", __func__);
+            goto cleanup;
+        }
 #endif
 
         /* call command */

@@ -1209,6 +1218,7 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
         msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username");
     }
 
+cleanup:
     return retval;
 }
 

This function is called in response to connecting clients, and can fail when I/O fails for some (possibly temporary) reason. In such cases we should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and (where needed) changes the callers to check for and handle errors.

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/misc.c       |  6 +++---
 src/openvpn/pf.c         |  8 ++++----
 src/openvpn/ssl_verify.c | 32 +++++++++++++++++++++-----------
 3 files changed, 28 insertions(+), 18 deletions(-)

src/openvpn/misc.c +3 -3

index 8c7f611..25f3800 100644
@@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
         retfname = gen_path(directory, BSTR(&fname), gc);
         if (!retfname)
         {
-            msg(M_FATAL, "Failed to create temporary filename and path");
+            msg(M_WARN, "Failed to create temporary filename and path");
             return NULL;
         }
 

@@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
         else if (fd == -1 && errno != EEXIST)
         {
             /* Something else went wrong, no need to retry.  */
-            msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
+            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
                 retfname);
             return NULL;
         }
     }
     while (attempts < 6);
 
-    msg(M_FATAL, "Failed to create temporary file after %i attempts", attempts);
+    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
     return NULL;
 }
 

src/openvpn/pf.c +0 -0

index 5cb002b..5fe1734 100644

src/openvpn/pf.c +0 -0

     }
 #endif /* ifdef PLUGIN_PF */

src/openvpn/ssl_verify.c +0 -0

index 9cd36d7..df2736c 100644

src/openvpn/ssl_verify.c +17 -10

     {
+        msg (M_WARN, "Failed to create peer cert file");
         return NULL;
     }
 
-    /* create tmp file to store peer cert */
-    peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
-
     /* write peer-cert in tmp-file */
     peercert_file = fopen(peercert_filename, "w+");
     if (!peercert_file)

@@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
 
     if (verify_export_cert)
     {
-        if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc)))
+        tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc);
+        if (!tmp_file)
         {
-            setenv_str(es, "peer_cert", tmp_file);
+            ret = false;
+            goto cleanup;
         }
+        setenv_str(es, "peer_cert", tmp_file);
     }
 
     argv_parse_cmd(&argv, verify_command);

@@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
         }
     }
 
+cleanup:
     gc_free(&gc);
     argv_reset(&argv);
 

@@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
     }
 }
 
-static void
+static bool
 key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options *opt)
 {
     struct gc_arena gc = gc_new();
-    const char *acf;
 
     key_state_rm_auth_control_file(ks);
-    acf = create_temp_file(opt->tmp_dir, "acf", &gc);
+    const char *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);
+    return acf;
 }
 
 static unsigned int

@@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
 
 #ifdef PLUGIN_DEF_AUTH
         /* generate filename for deferred auth control file */
-        key_state_gen_auth_control_file(ks, session->opt);
+        if (!key_state_gen_auth_control_file(ks, session->opt))
+        {
+            msg (D_TLS_ERRORS, "TLS Auth Error (%s): "
+                 "could not create deferred auth control file", __func__);
+            goto cleanup;
+        }
 #endif
 
         /* call command */

src/openvpn/pf.c pf_init_context(struct context *c) · line 645

Quoted 12 lines · Steffan Karger

src/openvpn/pf.c +0 -4

index 5cb002b..5fe1734 100644
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
                 }
 #endif
             }
-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }

Why removing this chunk above? Isn't that supposed to be printed when the init function of the plugin fails?

Quoted 3 lines

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

src/openvpn/pf.c pf_init_context(struct context *c) · line 646

Quoted 5 lines

src/openvpn/pf.c +4 -0

+        }
+        if (!c->c2.pf.enabled)
+        {
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }

Maybe it's just me missing how the signaling part really works, but what happens on the server when the SIGUSR1 is delivered? And why registering the signal when PF is disabled? I am missing something I think...

Quoted 10 lines

     }

#endif /* ifdef PLUGIN_PF */

src/openvpn/ssl_verify.c +0 -0

index 9cd36d7..df2736c 100644
@@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
     FILE *peercert_file;
     const char *peercert_filename = "";
 

src/openvpn/ssl_verify.c verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru · line 552

Quoted 4 lines

src/openvpn/ssl_verify.c +3 -1

-    if (!tmp_dir)
+    /* create tmp file to store peer cert */
+    if (!tmp_dir ||
+        !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))

|| at the beginning of the next line (as mentioned in my previous email)

Hi,

Quoted 16 lines · Antonio Quartulli

src/openvpn/pf.c pf_init_context(struct context *c) · line 645

Quoted 12 lines · Steffan Karger

src/openvpn/pf.c +0 -4

index 5cb002b..5fe1734 100644
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
                 }
 #endif
             }
-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }

Why removing this chunk above? Isn't that supposed to be printed when the init function of the plugin fails?

Because it's no longer true; instead of disabling the firewall, we will now reject the connection. I tried to explain that in the commit message:

Quoted 4 lines · Steffan Karger

Quoted 3 lines

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

I think rejecting a client is the sensible thing to do when initializing the firewall failed. (Rather than accepting the client and not enforcing the firewall rules.)

Quoted 10 lines

src/openvpn/pf.c pf_init_context(struct context *c) · line 646

Quoted 6 lines

src/openvpn/pf.c +4 -0

+        }
+        if (!c->c2.pf.enabled)
+        {
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }

Maybe it's just me missing how the signaling part really works, but what happens on the server when the SIGUSR1 is delivered? And why registering the signal when PF is disabled? I am missing something I think...

This registers a SIGUSR1 on the instance context, not the top-level context. This leads to an instance reset. See e.g. check_tls_errors(), which does the same in case of an error (or even uses SIGTERM, if --tls-exit is used).

Registering a signal is how error reporting is this part of the code works.

Quoted 11 lines

Quoted 11 lines

     }

#endif /* ifdef PLUGIN_PF */

src/openvpn/ssl_verify.c +0 -0

index 9cd36d7..df2736c 100644
@@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
     FILE *peercert_file;
     const char *peercert_filename = "";
 

Quoted 6 lines

src/openvpn/ssl_verify.c verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru · line 552

Quoted 4 lines

src/openvpn/ssl_verify.c +3 -1

-    if (!tmp_dir)
+    /* create tmp file to store peer cert */
+    if (!tmp_dir ||
+        !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))

|| at the beginning of the next line (as mentioned in my previous email)

Will do, once we agree on the above discussion points.

Hope this clarifies things.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

This changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/pf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

src/openvpn/pf.c +0 -0

index 5cb002bf..29231b67 100644
@@ -639,10 +639,11 @@ pf_init_context(struct context *c)
                 }
 #endif
             }

src/openvpn/pf.c +0 -0

     }
 #endif /* ifdef PLUGIN_PF */

Quoted 17 lines · Steffan Karger

This changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/pf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

src/openvpn/pf.c +0 -0

index 5cb002bf..29231b67 100644
@@ -639,10 +639,11 @@ pf_init_context(struct context *c)
                 }
 #endif
             }

src/openvpn/pf.c pf_init_context(struct context *c) · line 647

Quoted 10 lines

src/openvpn/pf.c +5 -4

-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }
+        }
+        if (!c->c2.pf.enabled)
+        {
+            msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client.");
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }

after this part there is some code dealing with the management
interface: wouldn't it be better to have a return after registering the signal so that we don't attempt any interaction with the management interface? If the plugin initialization failed, we should exit right away no?

(although this PF thing with the plugin being optional makes everything more complicated every time :/)

Another thought: if we abort every time we can't initialize the pf context, do you think we need the c->c2.pf.enabled member at all?

It looks to me that after this patch if PF is enabled, either a client is connected and has a PF context, or its connection was rejected entirely. Thus a overall state should be enough rather than a per-client one. (Can probably be adjusted with another patch if you don't feel like doing all this in here?)

This function is called in response to connecting clients, and can fail when I/O fails for some (possibly temporary) reason. In such cases we should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and (where needed) changes the callers to check for and handle errors.

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/misc.c       |  6 +++---
 src/openvpn/pf.c         |  8 ++++----
 src/openvpn/ssl_verify.c | 32 +++++++++++++++++++++-----------
 3 files changed, 28 insertions(+), 18 deletions(-)

src/openvpn/misc.c +3 -3

index 8c7f611..25f3800 100644
@@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char
 *prefix, struct gc_arena *gc)
         retfname = gen_path(directory, BSTR(&fname), gc);
         if (!retfname)
         {
-            msg(M_FATAL, "Failed to create temporary filename and path");
+            msg(M_WARN, "Failed to create temporary filename and path");
             return NULL;
         }

@@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char
 *prefix, struct gc_arena *gc)
         else if (fd == -1 && errno != EEXIST)
         {
             /* Something else went wrong, no need to retry.  */
-            msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
+            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
                 retfname);
             return NULL;
         }
     }
     while (attempts < 6);
 
-    msg(M_FATAL, "Failed to create temporary file after %i attempts",
 attempts);
+    msg(M_WARN, "Failed to create temporary file after %i attempts",
 attempts);

     return NULL;

}

src/openvpn/pf.c +4 -4

index 5cb002b..5fe1734 100644
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
                 }
 #endif
             }
-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }
+        }
+        if (!c->c2.pf.enabled)
+        {
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }
     }
 #endif /* ifdef PLUGIN_PF */

src/openvpn/ssl_verify.c +4 -4

index 9cd36d7..df2736c 100644
@@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t
 *peercert, const char *tmp_dir, stru
     FILE *peercert_file;
     const char *peercert_filename = "";
 
-    if (!tmp_dir)
+    /* create tmp file to store peer cert */
+    if (!tmp_dir ||
+        !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
     {
+        msg (M_WARN, "Failed to create peer cert file");
         return NULL;
     }
 
-    /* create tmp file to store peer cert */
-    peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
-
     /* write peer-cert in tmp-file */
     peercert_file = fopen(peercert_filename, "w+");

if (!peercert_file)

src/openvpn/ssl_verify.c +5 -2

@@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command,
 struct env_set *es,
 
     if (verify_export_cert)
     {
-        if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert,
 &gc)))
+        tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc);
+        if (!tmp_file)
         {
-            setenv_str(es, "peer_cert", tmp_file);
+            ret = false;
+            goto cleanup;
         }
+        setenv_str(es, "peer_cert", tmp_file);
     }

     argv_parse_cmd(&argv, verify_command);

src/openvpn/ssl_verify.c +5 -4

@@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command,
 struct env_set *es,
         }
     }
 
+cleanup:
     gc_free(&gc);
     argv_reset(&argv);

@@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
     }
 }
 
-static void
+static bool
 key_state_gen_auth_control_file(struct key_state *ks, const struct
 tls_options *opt)
 {
     struct gc_arena gc = gc_new();
-    const char *acf;
 
     key_state_rm_auth_control_file(ks);
-    acf = create_temp_file(opt->tmp_dir, "acf", &gc);
+    const char *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);
+    return acf;
 }
 

static unsigned int

src/openvpn/ssl_verify.c +6 -1

@@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session
 *session, const struct user_pass *up,
 
 #ifdef PLUGIN_DEF_AUTH
         /* generate filename for deferred auth control file */
-        key_state_gen_auth_control_file(ks, session->opt);
+        if (!key_state_gen_auth_control_file(ks, session->opt))
+        {
+            msg (D_TLS_ERRORS, "TLS Auth Error (%s): "
+                 "could not create deferred auth control file", __func__);
+            goto cleanup;
+        }
 #endif
 

         /* call command */

src/openvpn/ssl_verify.c +1 -0

@@ -1209,6 +1218,7 @@ verify_user_pass_plugin(struct tls_session *session,
 const struct user_pass *up,
         msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer
 provided a blank username");
     }
 
+cleanup:
     return retval;

}

--
2.7.4

------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

This function is called in response to connecting clients, and can fail when I/O fails for some (possibly temporary) reason. In such cases we should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and (where needed) changes the callers to check for and handle errors.

Note that this changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/misc.c       |  6 +++---
 src/openvpn/pf.c         |  8 ++++----
 src/openvpn/ssl_verify.c | 32 +++++++++++++++++++++-----------
 3 files changed, 28 insertions(+), 18 deletions(-)

src/openvpn/misc.c +0 -0

index 8c7f611..25f3800 100644

src/openvpn/pf.c +0 -0

index 5cb002b..5fe1734 100644
@@ -639,10 +639,10 @@ pf_init_context(struct context *c)
                 }
 #endif

From: Steffan Karger <[email protected]...>

close_instance() tries to remove the file in c2.pf.filename, but that only works if we actually set that if we fail. So, set that filename as soon as we know we've created the file.

Signed-off-by Steffan Karger <[email protected]...>

This changes the behavior for pf plugins: instead of just not initializing the firewall rules and happily continuing, this now rejects the client in the case of an (unlikely) failure to initialize the pf.

Signed-off-by Steffan Karger <[email protected]...>

From: Steffan Karger <[email protected]...>

This function is called in response to connecting clients, and can fail when I/O fails for some (possibly temporary) reason. In such cases we should not exit the process, but just reject the connecting client.

This commit changes the function to actually return NULL on errors, and (where needed) changes the callers to check for and handle errors.

Since the tls-crypt-v2 metadata code also calls create_temp_file() when clients connect, I consider this a prerequisite for tls-crypt-v2.

Signed-off-by Steffan Karger <[email protected]...>

Your patch has been applied to the master branch.

commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916

Author: Steffan Karger
Date:   Wed Nov 1 23:03:41 2017 +0100

     Don't throw fatal errors from create_temp_file()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg15701.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

If gc == NULL, the data allocated in the alloc_gc_buf() call in create_temp_file or the string_mod_const call in gen_path would never be free'd.

These functions are currently never called that way, but let's prevent future problems.

While touching create_temp_file, also remove the counter variable, which is never read, simplify the do-while to a while loop, and truncate the prefix (if needed) to preserve the random and extension of the created filename.

Signed-off-by Steffan Karger <[email protected]...>

---
v2:
- change create_temp_file to avoid using a struct buffer (simpler)
- add gc != NULL check for gen_path (avoid similar memleak pitfall)
v3:
- Check the return value of openvpn_snprintf()

 src/openvpn/misc.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

src/openvpn/misc.c +12 -8

index 25f38003..67011169 100644
@@ -723,21 +723,26 @@ test_file(const char *filename)
 const char *
 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;
+    char fname[256] = { 0 };
+    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
+    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
 
-    do
+    while (attempts < 6)
     {
         ++attempts;
-        ++counter;
 
-        buf_printf(&fname, PACKAGE "_%s_%08lx%08lx.tmp", prefix,
-                   (unsigned long) get_random(), (unsigned long) get_random());
+        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
+                              prefix, (unsigned long) get_random(),
+                              (unsigned long) get_random()))
+        {
+            msg(M_WARN, "ERROR: temporary filename too long");
+            return NULL;
+        }
 
-        retfname = gen_path(directory, BSTR(&fname), gc);
+        retfname = gen_path(directory, fname, gc);
         if (!retfname)
         {
             msg(M_WARN, "Failed to create temporary filename and path");

@@ -760,7 +765,6 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
             return NULL;
         }
     }
-    while (attempts < 6);
 
     msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
     return NULL;

src/openvpn/misc.c +2 -0

+    }
+
     const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
 
     if (safe_filename

src/openvpn/misc.c gen_path(const char *directory, const char *filename, struct gc_arena *gc) · line 822

Quoted 8 lines

src/openvpn/misc.c +4 -0

@@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
 #else
     const int CC_PATH_RESERVED = CC_SLASH;
 #endif
+
+    if (!gc)
+    {
+        return NULL; /* Would leak memory otherwise */

As far as I understood the ratio behind this change, we are basically saying that passing gc == NULL is a real programming error and not a runtime problem. If so, wouldn't it be better to assert(gc) directly? I am saying so because we should make this failure louder, so that it can be recognized in an early test.

Or can we have cases when this function is called with NULL because of a temporary failure (my understanding is that this should not be possible)?

Hi,

Quoted 15 lines · Antonio Quartulli

src/openvpn/misc.c gen_path(const char *directory, const char *filename, struct gc_arena *gc) · line 822

Quoted 8 lines

src/openvpn/misc.c +4 -0

@@ -812,6 +816,12 @@ gen_path(const char *directory, const char *filename, struct gc_arena *gc)
 #else
     const int CC_PATH_RESERVED = CC_SLASH;
 #endif
+
+    if (!gc)
+    {
+        return NULL; /* Would leak memory otherwise */

As far as I understood the ratio behind this change, we are basically saying that passing gc == NULL is a real programming error and not a runtime problem. If so, wouldn't it be better to assert(gc) directly? I am saying so because we should make this failure louder, so that it can be recognized in an early test.

Quoted 2 lines

Or can we have cases when this function is called with NULL because of a temporary failure (my understanding is that this should not be possible)?

I'm a bit in doubt here - if this parameter is NULL, that's a programming error. But we've seen before that programming errors sometimes can be triggered by external input. In this case, this function can be called when a client connects. It *should* never hit this case, but if it somehow does, that becomes a DoS vuln. So I think I prefer 'graceful error handling' in paths that are exposed like this, unless the error indicates a system integrity error such as an out-of-bounds read or write.