Browse code

Move private file access checks to options_postprocess_filechecks()

This removes the dependency of crypto.c on misc.c, which makes testing
(stuff that needs) crypto.c functionality easier.

Apart from that, testing file access really belongs in
options_postprocess_filechecks(), and moving it there enables us to
perform the same check for other private files too.

v2: change indenting, remove remaining warn_if_group_others_accessible()
calls and move function to options.c.

[ DS: This patch is a slightly modified version of the one sent to the
mailing list. It removes all references to --tls-crypt, so it
can be applied eariler to the tree as it contains a good clean-up
as well ]

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <1479045751-22297-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13019.html
Signed-off-by: David Sommerseth <davids@openvpn.net>

Steffan Karger authored on 2016/11/13 23:02:31
Showing 6 changed files
... ...
@@ -36,7 +36,7 @@
36 36
 #include "crypto.h"
37 37
 #include "error.h"
38 38
 #include "integer.h"
39
-#include "misc.h"
39
+#include "platform.h"
40 40
 
41 41
 #include "memdbg.h"
42 42
 
... ...
@@ -1320,9 +1320,6 @@ read_key_file (struct key2 *key2, const char *file, const unsigned int flags)
1320 1320
   if (!(flags & RKF_INLINE))
1321 1321
     buf_clear (&in);
1322 1322
 
1323
-  if (key2->n)
1324
-    warn_if_group_others_accessible (error_filename);
1325
-
1326 1323
 #if 0
1327 1324
   /* DEBUGGING */
1328 1325
   {
... ...
@@ -190,31 +190,6 @@ save_inetd_socket_descriptor (void)
190 190
 }
191 191
 
192 192
 /*
193
- * Warn if a given file is group/others accessible.
194
- */
195
-void
196
-warn_if_group_others_accessible (const char* filename)
197
-{
198
-#ifndef WIN32
199
-#ifdef HAVE_STAT
200
-  if (strcmp (filename, INLINE_FILE_TAG))
201
-    {
202
-      struct stat st;
203
-      if (stat (filename, &st))
204
-	{
205
-	  msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
206
-	}
207
-      else
208
-	{
209
-	  if (st.st_mode & (S_IRWXG|S_IRWXO))
210
-	    msg (M_WARN, "WARNING: file '%s' is group or others accessible", filename);
211
-	}
212
-    }
213
-#endif
214
-#endif
215
-}
216
-
217
-/*
218 193
  * Print an error message based on the status code returned by system().
219 194
  */
220 195
 const char *
... ...
@@ -1111,8 +1086,6 @@ get_user_pass_cr (struct user_pass *up,
1111 1111
           FILE *fp;
1112 1112
           char password_buf[USER_PASS_LEN] = { '\0' };
1113 1113
 
1114
-          warn_if_group_others_accessible (auth_file);
1115
-
1116 1114
           fp = platform_fopen (auth_file, "r");
1117 1115
           if (!fp)
1118 1116
             msg (M_ERR, "Error opening '%s' auth file: %s", prefix, auth_file);
... ...
@@ -71,9 +71,6 @@ void run_up_down (const char *command,
71 71
 
72 72
 void write_pid (const char *filename);
73 73
 
74
-/* check file protections */
75
-void warn_if_group_others_accessible(const char* filename);
76
-
77 74
 /* system flags */
78 75
 #define S_SCRIPT (1<<0)
79 76
 #define S_FATAL  (1<<1)
... ...
@@ -2676,11 +2676,37 @@ options_postprocess_mutate (struct options *o)
2676 2676
  */
2677 2677
 #ifndef ENABLE_SMALL  /** Expect people using the stripped down version to know what they do */
2678 2678
 
2679
+/*
2680
+ * Warn if a given file is group/others accessible.
2681
+ */
2682
+static void
2683
+warn_if_group_others_accessible (const char* filename)
2684
+{
2685
+#ifndef WIN32
2686
+#ifdef HAVE_STAT
2687
+  if (strcmp (filename, INLINE_FILE_TAG))
2688
+    {
2689
+      struct stat st;
2690
+      if (stat (filename, &st))
2691
+	{
2692
+	  msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
2693
+	}
2694
+      else
2695
+	{
2696
+	  if (st.st_mode & (S_IRWXG|S_IRWXO))
2697
+	    msg (M_WARN, "WARNING: file '%s' is group or others accessible", filename);
2698
+	}
2699
+    }
2700
+#endif
2701
+#endif
2702
+}
2703
+
2679 2704
 #define CHKACC_FILE (1<<0)       /** Check for a file/directory precense */
2680 2705
 #define CHKACC_DIRPATH (1<<1)    /** Check for directory precense where a file should reside */
2681 2706
 #define CHKACC_FILEXSTWR (1<<2)  /** If file exists, is it writable? */
2682 2707
 #define CHKACC_INLINE (1<<3)     /** File is present if it's an inline file */
2683 2708
 #define CHKACC_ACPTSTDIN (1<<4)  /** If filename is stdin, it's allowed and "exists" */
2709
+#define CHKACC_PRIVATE (1<<5)	 /** Warn if this (private) file is group/others accessible */
2684 2710
 
2685 2711
 static bool
2686 2712
 check_file_access(const int type, const char *file, const int mode, const char *opt)
... ...
@@ -2721,6 +2747,11 @@ check_file_access(const int type, const char *file, const int mode, const char *
2721 2721
     if (platform_access (file, W_OK) != 0)
2722 2722
       errcode = errno;
2723 2723
 
2724
+  if (type & CHKACC_PRIVATE)
2725
+    {
2726
+      warn_if_group_others_accessible (file);
2727
+    }
2728
+
2724 2729
   /* Scream if an error is found */
2725 2730
   if( errcode > 0 )
2726 2731
     msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': %s",
... ...
@@ -2837,10 +2868,12 @@ options_postprocess_filechecks (struct options *options)
2837 2837
 #ifdef MANAGMENT_EXTERNAL_KEY
2838 2838
   if(!(options->management_flags & MF_EXTERNAL_KEY))
2839 2839
 #endif
2840
-     errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->priv_key_file, R_OK,
2841
-                             "--key");
2842
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->pkcs12_file, R_OK,
2843
-                             "--pkcs12");
2840
+    {
2841
+      errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
2842
+				 options->priv_key_file, R_OK, "--key");
2843
+    }
2844
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
2845
+			     options->pkcs12_file, R_OK, "--pkcs12");
2844 2846
 
2845 2847
   if (options->ssl_flags & SSLF_CRL_VERIFY_DIR)
2846 2848
     errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE, options->crl_file, R_OK|X_OK,
... ...
@@ -2849,24 +2882,24 @@ options_postprocess_filechecks (struct options *options)
2849 2849
     errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE|CHKACC_INLINE,
2850 2850
                                       options->crl_file, R_OK, "--crl-verify");
2851 2851
 
2852
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->tls_auth_file, R_OK,
2853
-                             "--tls-auth");
2854
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->shared_secret_file, R_OK,
2855
-                             "--secret");
2852
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
2853
+			     options->tls_auth_file, R_OK, "--tls-auth");
2854
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
2855
+			     options->shared_secret_file, R_OK, "--secret");
2856 2856
   errs |= check_file_access (CHKACC_DIRPATH|CHKACC_FILEXSTWR,
2857
-                             options->packet_id_file, R_OK|W_OK, "--replay-persist");
2857
+			     options->packet_id_file, R_OK|W_OK, "--replay-persist");
2858 2858
 
2859 2859
   /* ** Password files ** */
2860
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
2860
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
2861 2861
                              options->key_pass_file, R_OK, "--askpass");
2862 2862
 #endif /* ENABLE_CRYPTO */
2863 2863
 #ifdef ENABLE_MANAGEMENT
2864
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
2864
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
2865 2865
                              options->management_user_pass, R_OK,
2866 2866
                              "--management user/password file");
2867 2867
 #endif /* ENABLE_MANAGEMENT */
2868 2868
 #if P2MP
2869
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
2869
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
2870 2870
                              options->auth_user_pass_file, R_OK,
2871 2871
                              "--auth-user-pass");
2872 2872
 #endif /* P2MP */
... ...
@@ -361,8 +361,6 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
361 361
       return 1;
362 362
     }
363 363
 
364
-  warn_if_group_others_accessible (priv_key_file);
365
-
366 364
   if (!mbed_ok(mbedtls_pk_check_pair(&ctx->crt_chain->pk, ctx->priv_key)))
367 365
     {
368 366
       msg (M_WARN, "Private key does not match the certificate");
... ...
@@ -582,7 +582,6 @@ tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
582 582
   /* Load Private Key */
583 583
   if (!SSL_CTX_use_PrivateKey (ctx->ctx, pkey))
584 584
     crypto_msg (M_FATAL, "Cannot use private key");
585
-  warn_if_group_others_accessible (pkcs12_file);
586 585
 
587 586
   /* Check Private Key */
588 587
   if (!SSL_CTX_check_private_key (ctx->ctx))
... ...
@@ -758,7 +757,6 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
758 758
       crypto_msg (M_WARN, "Cannot load private key file %s", priv_key_file);
759 759
       goto end;
760 760
     }
761
-  warn_if_group_others_accessible (priv_key_file);
762 761
 
763 762
   /* Check Private Key */
764 763
   if (!SSL_CTX_check_private_key (ssl_ctx))