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
