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>

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 749b7da8c707ece072dbf9846263bf23789bdc5f..8b2f460e3ace8a34e7a26f0d9459a6c814bb7ce1 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -36,7 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
-#include "misc.h"
+#include "platform.h"
 
 #include "memdbg.h"
 
@@ -1320,9 +1320,6 @@ read_key_file (struct key2 *key2, const char *file, const unsigned int flags)
   if (!(flags & RKF_INLINE))
     buf_clear (&in);
 
-  if (key2->n)
-    warn_if_group_others_accessible (error_filename);
-
 #if 0
   /* DEBUGGING */
   {

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index bc8b33cb7e12aa195dfe1684075741f6b6e62454..fdee663581ded0ce69ccb3ee43588a326ef1322d 100644 (file)
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -189,31 +189,6 @@ save_inetd_socket_descriptor (void)
 #endif
 }
 
-/*
- * Warn if a given file is group/others accessible.
- */
-void
-warn_if_group_others_accessible (const char* filename)
-{
-#ifndef WIN32
-#ifdef HAVE_STAT
-  if (strcmp (filename, INLINE_FILE_TAG))
-    {
-      struct stat st;
-      if (stat (filename, &st))
-       {
-         msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
-       }
-      else
-       {
-         if (st.st_mode & (S_IRWXG|S_IRWXO))
-           msg (M_WARN, "WARNING: file '%s' is group or others accessible", filename);
-       }
-    }
-#endif
-#endif
-}
-
 /*
  * Print an error message based on the status code returned by system().
  */
@@ -1111,8 +1086,6 @@ get_user_pass_cr (struct user_pass *up,
           FILE *fp;
           char password_buf[USER_PASS_LEN] = { '\0' };
 
-          warn_if_group_others_accessible (auth_file);
-
           fp = platform_fopen (auth_file, "r");
           if (!fp)
             msg (M_ERR, "Error opening '%s' auth file: %s", prefix, auth_file);

diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index ceda3235f620b19bc91a4331f83200eecfad5e14..cc0a474dd169378d927501c35d32a0342ee68789 100644 (file)
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -71,9 +71,6 @@ void run_up_down (const char *command,
 
 void write_pid (const char *filename);
 
-/* check file protections */
-void warn_if_group_others_accessible(const char* filename);
-
 /* system flags */
 #define S_SCRIPT (1<<0)
 #define S_FATAL  (1<<1)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 1b9294aff74a5fa9e4cfe56d14077dd4cce84518..32826f509ecebafad836d7277b9d9c0340bab41a 100644 (file)
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2676,11 +2676,37 @@ options_postprocess_mutate (struct options *o)
  */
 #ifndef ENABLE_SMALL  /** Expect people using the stripped down version to know what they do */
 
+/*
+ * Warn if a given file is group/others accessible.
+ */
+static void
+warn_if_group_others_accessible (const char* filename)
+{
+#ifndef WIN32
+#ifdef HAVE_STAT
+  if (strcmp (filename, INLINE_FILE_TAG))
+    {
+      struct stat st;
+      if (stat (filename, &st))
+       {
+         msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
+       }
+      else
+       {
+         if (st.st_mode & (S_IRWXG|S_IRWXO))
+           msg (M_WARN, "WARNING: file '%s' is group or others accessible", filename);
+       }
+    }
+#endif
+#endif
+}
+
 #define CHKACC_FILE (1<<0)       /** Check for a file/directory precense */
 #define CHKACC_DIRPATH (1<<1)    /** Check for directory precense where a file should reside */
 #define CHKACC_FILEXSTWR (1<<2)  /** If file exists, is it writable? */
 #define CHKACC_INLINE (1<<3)     /** File is present if it's an inline file */
 #define CHKACC_ACPTSTDIN (1<<4)  /** If filename is stdin, it's allowed and "exists" */
+#define CHKACC_PRIVATE (1<<5)   /** Warn if this (private) file is group/others accessible */
 
 static bool
 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 *
     if (platform_access (file, W_OK) != 0)
       errcode = errno;
 
+  if (type & CHKACC_PRIVATE)
+    {
+      warn_if_group_others_accessible (file);
+    }
+
   /* Scream if an error is found */
   if( errcode > 0 )
     msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': %s",
@@ -2837,10 +2868,12 @@ options_postprocess_filechecks (struct options *options)
 #ifdef MANAGMENT_EXTERNAL_KEY
   if(!(options->management_flags & MF_EXTERNAL_KEY))
 #endif
-     errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->priv_key_file, R_OK,
-                             "--key");
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->pkcs12_file, R_OK,
-                             "--pkcs12");
+    {
+      errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
+                                options->priv_key_file, R_OK, "--key");
+    }
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
+                            options->pkcs12_file, R_OK, "--pkcs12");
 
   if (options->ssl_flags & SSLF_CRL_VERIFY_DIR)
     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)
     errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE|CHKACC_INLINE,
                                       options->crl_file, R_OK, "--crl-verify");
 
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->tls_auth_file, R_OK,
-                             "--tls-auth");
-  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE, options->shared_secret_file, R_OK,
-                             "--secret");
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
+                            options->tls_auth_file, R_OK, "--tls-auth");
+  errs |= check_file_access (CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE,
+                            options->shared_secret_file, R_OK, "--secret");
   errs |= check_file_access (CHKACC_DIRPATH|CHKACC_FILEXSTWR,
-                             options->packet_id_file, R_OK|W_OK, "--replay-persist");
+                            options->packet_id_file, R_OK|W_OK, "--replay-persist");
 
   /* ** Password files ** */
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
                              options->key_pass_file, R_OK, "--askpass");
 #endif /* ENABLE_CRYPTO */
 #ifdef ENABLE_MANAGEMENT
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
                              options->management_user_pass, R_OK,
                              "--management user/password file");
 #endif /* ENABLE_MANAGEMENT */
 #if P2MP
-  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN,
+  errs |= check_file_access (CHKACC_FILE|CHKACC_ACPTSTDIN|CHKACC_PRIVATE,
                              options->auth_user_pass_file, R_OK,
                              "--auth-user-pass");
 #endif /* P2MP */

diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index a6c90b1f1cc50d4f1cd5f8dfbcfbd2a8477c58c5..c2aff405a921618bc23c8e25e8a46dffd8dc7f60 100644 (file)
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -361,8 +361,6 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
       return 1;
     }
 
-  warn_if_group_others_accessible (priv_key_file);
-
   if (!mbed_ok(mbedtls_pk_check_pair(&ctx->crt_chain->pk, ctx->priv_key)))
     {
       msg (M_WARN, "Private key does not match the certificate");

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 8909ca3b864d46212e3246766605732b969d63c2..171cd02a6eb3be595116beef1e46ff7fea15dc16 100644 (file)
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -582,7 +582,6 @@ tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
   /* Load Private Key */
   if (!SSL_CTX_use_PrivateKey (ctx->ctx, pkey))
     crypto_msg (M_FATAL, "Cannot use private key");
-  warn_if_group_others_accessible (pkcs12_file);
 
   /* Check Private Key */
   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,
       crypto_msg (M_WARN, "Cannot load private key file %s", priv_key_file);
       goto end;
     }
-  warn_if_group_others_accessible (priv_key_file);
 
   /* Check Private Key */
   if (!SSL_CTX_check_private_key (ssl_ctx))