Browse code

Don't throw fatal errors from create_temp_file()

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 <steffan.karger@fox-it.com>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20171101220342.14648-4-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15701.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Steffan Karger authored on 2017/11/02 07:03:41
Showing 2 changed files
... ...
@@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
740 740
         retfname = gen_path(directory, BSTR(&fname), gc);
741 741
         if (!retfname)
742 742
         {
743
-            msg(M_FATAL, "Failed to create temporary filename and path");
743
+            msg(M_WARN, "Failed to create temporary filename and path");
744 744
             return NULL;
745 745
         }
746 746
 
... ...
@@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
755 755
         else if (fd == -1 && errno != EEXIST)
756 756
         {
757 757
             /* Something else went wrong, no need to retry.  */
758
-            msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
758
+            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
759 759
                 retfname);
760 760
             return NULL;
761 761
         }
762 762
     }
763 763
     while (attempts < 6);
764 764
 
765
-    msg(M_FATAL, "Failed to create temporary file after %i attempts", attempts);
765
+    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
766 766
     return NULL;
767 767
 }
768 768
 
... ...
@@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
547 547
     FILE *peercert_file;
548 548
     const char *peercert_filename = "";
549 549
 
550
-    if (!tmp_dir)
550
+    /* create tmp file to store peer cert */
551
+    if (!tmp_dir
552
+        || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
551 553
     {
554
+        msg (M_WARN, "Failed to create peer cert file");
552 555
         return NULL;
553 556
     }
554 557
 
555
-    /* create tmp file to store peer cert */
556
-    peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
557
-
558 558
     /* write peer-cert in tmp-file */
559 559
     peercert_file = fopen(peercert_filename, "w+");
560 560
     if (!peercert_file)
... ...
@@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
589 589
 
590 590
     if (verify_export_cert)
591 591
     {
592
-        if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc)))
592
+        tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc);
593
+        if (!tmp_file)
593 594
         {
594
-            setenv_str(es, "peer_cert", tmp_file);
595
+            ret = false;
596
+            goto cleanup;
595 597
         }
598
+        setenv_str(es, "peer_cert", tmp_file);
596 599
     }
597 600
 
598 601
     argv_parse_cmd(&argv, verify_command);
... ...
@@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
609 609
         }
610 610
     }
611 611
 
612
+cleanup:
612 613
     gc_free(&gc);
613 614
     argv_reset(&argv);
614 615
 
... ...
@@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
879 879
     }
880 880
 }
881 881
 
882
-static void
882
+static bool
883 883
 key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options *opt)
884 884
 {
885 885
     struct gc_arena gc = gc_new();
886
-    const char *acf;
887 886
 
888 887
     key_state_rm_auth_control_file(ks);
889
-    acf = create_temp_file(opt->tmp_dir, "acf", &gc);
888
+    const char *acf = create_temp_file(opt->tmp_dir, "acf", &gc);
890 889
     if (acf)
891 890
     {
892 891
         ks->auth_control_file = string_alloc(acf, NULL);
893 892
         setenv_str(opt->es, "auth_control_file", ks->auth_control_file);
894
-    } /* FIXME: Should have better error handling? */
893
+    }
895 894
 
896 895
     gc_free(&gc);
896
+    return acf;
897 897
 }
898 898
 
899 899
 static unsigned int
... ...
@@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
1184 1184
 
1185 1185
 #ifdef PLUGIN_DEF_AUTH
1186 1186
         /* generate filename for deferred auth control file */
1187
-        key_state_gen_auth_control_file(ks, session->opt);
1187
+        if (!key_state_gen_auth_control_file(ks, session->opt))
1188
+        {
1189
+            msg (D_TLS_ERRORS, "TLS Auth Error (%s): "
1190
+                 "could not create deferred auth control file", __func__);
1191
+            goto cleanup;
1192
+        }
1188 1193
 #endif
1189 1194
 
1190 1195
         /* call command */
... ...
@@ -1209,6 +1218,7 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
1209 1209
         msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username");
1210 1210
     }
1211 1211
 
1212
+cleanup:
1212 1213
     return retval;
1213 1214
 }
1214 1215