Don't throw fatal errors from verify_cert_export_cert()

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

As with create_temp_file(), this function is called on client connects and should not cause fatal errors when I/O (possibly temporarily) fails.

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

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

---
 src/openvpn/ssl_verify.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

src/openvpn/ssl_verify.c +0 -0

index ebb1da2..12cff9a 100644

As with create_temp_file(), this function is called on client connects and should not cause fatal errors when I/O (possibly temporarily) fails.

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

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

---
v2: Use M_NONFATAL (instead of M_WARN/M_ERRNO), as suggested by Selva.

 src/openvpn/ssl_verify.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

src/openvpn/ssl_verify.c +1 -1

index ebb1da2..0ba9f41 100644
@@ -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;
     }
 

Quoted 10 lines · Steffan Karger

src/openvpn/ssl_verify.c +2 -1

@@ -557,13 +557,15 @@ 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;
     }
 

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

Quoted 1 line

src/openvpn/ssl_verify.c +0 -0

     if (SUCCESS != x509_write_pem(peercert_file, peercert))

The openssl version of x509_write_pem() called here could fail with M_ERR --- is that already fixed in one of the pending patches? If not, why not make that one too non-fatal?

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

Quoted 3 lines

src/openvpn/ssl_verify.c +1 -1

     {
-        msg(M_ERR, "Error writing PEM file containing certificate");
+        msg(M_NONFATAL, "Error writing PEM file containing certificate");

Yeah, not including M_ERRNO looks like the right thing to do here.

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

Quoted 1 line

src/openvpn/ssl_verify.c +1 -0

+        peercert_filename = NULL;

This could potentially lead to a stale tempfile left behind. Could be fixed by unlinking here? Successfully exported cert file does get unlinked after the verify script returns.

Sorry, earlier I only made a hasty remark about the error flag and did not do a proper review...

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 <[email protected]...>

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

src/openvpn/ssl_verify.c +6 -3

index ebb1da2..5ae4fbb 100644
@@ -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);

src/openvpn/ssl_verify_openssl.c +0 -0

index 02850fc..238292f 100644