[Openvpn-devel,v3] Don't throw fatal errors from verify_cert_export_cert()

Message ID 1514933571-4592-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers show
Series
  • [Openvpn-devel,v3] Don't throw fatal errors from verify_cert_export_cert()
Related show

Commit Message

Steffan Karger Jan. 2, 2018, 10:52 p.m.
As with create_temp_file(), this function is called on client connects
and should not cause fatal errors when I/O (possibly temporarily) fails.
Fix this and the openssl backend implementation of x509_write_pem() to
no longer throw fatal errors.

The callers of this function are already fixed in the commit that does
the same for create_temp_file().

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
v2: Use M_NONFATAL (instead of M_WARN/M_ERRNO), as suggested by Selva.
v3: Also fix x509_write_pem and unlink file on write error, per Selva too.

 src/openvpn/ssl_verify.c         | 9 ++++++---
 src/openvpn/ssl_verify_openssl.c | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Selva Nair Jan. 2, 2018, 11:41 p.m. | #1
Hi,

Thanks for v3. Looks good now.

On Tue, Jan 2, 2018 at 5:52 PM, Steffan Karger
<steffan.karger@fox-it.com> wrote:
> As with create_temp_file(), this function is called on client connects
> and should not cause fatal errors when I/O (possibly temporarily) fails.
> Fix this and the openssl backend implementation of x509_write_pem() to
> no longer throw fatal errors.
>
> The callers of this function are already fixed in the commit that does
> the same for create_temp_file().
>
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
> ---
> v2: Use M_NONFATAL (instead of M_WARN/M_ERRNO), as suggested by Selva.
> v3: Also fix x509_write_pem and unlink file on write error, per Selva too.
>
>  src/openvpn/ssl_verify.c         | 9 ++++++---
>  src/openvpn/ssl_verify_openssl.c | 2 +-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index ebb1da2..5ae4fbb 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -549,7 +549,7 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
>      if (!tmp_dir
>          || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
>      {
> -        msg (M_WARN, "Failed to create peer cert file");
> +        msg(M_NONFATAL, "Failed to create peer cert file");
>          return NULL;
>      }
>
> @@ -557,13 +557,16 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
>      peercert_file = fopen(peercert_filename, "w+");
>      if (!peercert_file)
>      {
> -        msg(M_ERR, "Failed to open temporary file : %s", peercert_filename);
> +        msg(M_NONFATAL|M_ERRNO, "Failed to open temporary file: %s",
> +            peercert_filename);
>          return NULL;
>      }
>
>      if (SUCCESS != x509_write_pem(peercert_file, peercert))
>      {
> -        msg(M_ERR, "Error writing PEM file containing certificate");
> +        msg(M_NONFATAL, "Error writing PEM file containing certificate");
> +        (void) platform_unlink(peercert_filename);
> +        peercert_filename = NULL;
>      }
>
>      fclose(peercert_file);
> diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
> index 02850fc..238292f 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -767,7 +767,7 @@ x509_write_pem(FILE *peercert_file, X509 *peercert)
>  {
>      if (PEM_write_X509(peercert_file, peercert) < 0)
>      {
> -        msg(M_ERR, "Failed to write peer certificate in PEM format");
> +        msg(M_NONFATAL, "Failed to write peer certificate in PEM format");
>          return FAILURE;
>      }
>      return SUCCESS;

Acked-by: Selva Nair <selva.nair@gmail.com>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Jan. 9, 2018, 3:28 p.m. | #2
Your patch has been applied to the master branch.

commit fb515a831dd530b9fb0c1581418d82bc74da6f8e
Author: Steffan Karger
Date:   Tue Jan 2 23:52:51 2018 +0100

     Don't throw fatal errors from verify_cert_export_cert()

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1514933571-4592-1-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16136.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


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

Patch

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index ebb1da2..5ae4fbb 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -549,7 +549,7 @@  verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
     if (!tmp_dir
         || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
     {
-        msg (M_WARN, "Failed to create peer cert file");
+        msg(M_NONFATAL, "Failed to create peer cert file");
         return NULL;
     }
 
@@ -557,13 +557,16 @@  verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
     peercert_file = fopen(peercert_filename, "w+");
     if (!peercert_file)
     {
-        msg(M_ERR, "Failed to open temporary file : %s", peercert_filename);
+        msg(M_NONFATAL|M_ERRNO, "Failed to open temporary file: %s",
+            peercert_filename);
         return NULL;
     }
 
     if (SUCCESS != x509_write_pem(peercert_file, peercert))
     {
-        msg(M_ERR, "Error writing PEM file containing certificate");
+        msg(M_NONFATAL, "Error writing PEM file containing certificate");
+        (void) platform_unlink(peercert_filename);
+        peercert_filename = NULL;
     }
 
     fclose(peercert_file);
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 02850fc..238292f 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -767,7 +767,7 @@  x509_write_pem(FILE *peercert_file, X509 *peercert)
 {
     if (PEM_write_X509(peercert_file, peercert) < 0)
     {
-        msg(M_ERR, "Failed to write peer certificate in PEM format");
+        msg(M_NONFATAL, "Failed to write peer certificate in PEM format");
         return FAILURE;
     }
     return SUCCESS;