On 08/11/16 21:18, Steffan Karger wrote: > From: Steffan Karger <stef...@karger.me> > > This removes the dependency of crypto.c on misc.c, which makes testing > (stuff that needs) crypto.c functionality easier. In particular, this > simplifies the --tls-crypt tests in one of the follow-up patches. > > 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. > > Signed-off-by: Steffan Karger <steffan.kar...@fox-it.com> > --- > src/openvpn/crypto.c | 5 +---- > src/openvpn/options.c | 36 ++++++++++++++++++++++-------------- > 2 files changed, 23 insertions(+), 18 deletions(-)
These are code paths I know fairly well :) > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index e9dc17e..c576e6e 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c [...snip...] > @@ -2850,10 +2856,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"); I'd prefer to have the arguments continuation aligned with the CHKACC_FILE, but this is code style bike-shedding. However that is what we do in the rest of this scope. > 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, > @@ -2862,26 +2870,26 @@ 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->tls_crypt_file, R_OK, > - "--tls-crypt"); > - 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->tls_crypt_file, R_OK, "--tls-crypt"); > + 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"); Here the indent is as expected though :) > #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 */ > Patch looks good to me. Good clean-up moving warn_if_group_others_accessible() to these code paths. But I have one comment. I see that warn_if_group_others_accessible() is still called from ssl_{mbedtls,openssl}.c ... seems to be used in tls_ctx_load_priv_file() and tls_ctx_load_pkcs12(). Are those checks needed now? If not, maybe move warn_if_group_others_accessible() to options.c as well? With these issues resolved, it's an ACK from me. -- kind regards, David Sommerseth OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel