Re: git: 6ae8d57652fa - main - mac_veriexec: add mac_priv_grant check for NODEV

From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Sun, 16 Apr 2023 23:40:07 UTC
On 17 Apr 2023, at 00:14, Stephen J. Kiernan <stevek@FreeBSD.org> wrote:
> 
> The branch main has been updated by stevek:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=6ae8d57652faf3bb8532ed627676c65eecd94a31
> 
> commit 6ae8d57652faf3bb8532ed627676c65eecd94a31
> Author:     Simon J. Gerraty <sjg@juniper.net>
> AuthorDate: 2019-07-29 22:38:16 +0000
> Commit:     Stephen J. Kiernan <stevek@FreeBSD.org>
> CommitDate: 2023-04-16 23:14:40 +0000
> 
>    mac_veriexec: add mac_priv_grant check for NODEV
> 
>    Allow other MAC modules to override some veriexec checks.
> 
>    We need two new privileges:
>    PRIV_VERIEXEC_DIRECT    process wants to override 'indirect' flag
>                            on interpreter
>    PRIV_VERIEXEC_NOVERIFY  typically associated with PRIV_VERIEXEC_DIRECT
>                            allow override of O_VERIFY
> 
>    We also need to check for PRIV_VERIEXEC_NOVERIFY override
>    for FINGERPRINT_NODEV and FINGERPRINT_NOENTRY.
>    This will only happen if parent had PRIV_VERIEXEC_DIRECT override.
> 
>    This allows for MAC modules to selectively allow some applications to
>    run without verification.
> 
>    Needless to say, this is extremely dangerous and should only be used
>    sparingly and carefully.
> 
>    Obtained from:  Juniper Networks, Inc.
> 
>    Reviewers: sjg
>    Subscribers: imp, dab
> 
>    Differential Revision: https://reviews.freebsd.org/D39537

Hi Steve,
I see you’ve made a bunch of commits over the past few days that suffer
from not following the proper commit message formatting outlined in the
Committer’s Guide and templated in
tools/tools/git/hooks/prepare-commit-msg; can you please take care and
do so in future?

Jess

> ---
> sys/security/mac_veriexec/mac_veriexec.c         | 16 ++++++++++++++++
> sys/security/mac_veriexec/veriexec_fingerprint.c | 23 ++++++++++++++++++++++-
> sys/sys/priv.h                                   |  8 +++++++-
> 3 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/sys/security/mac_veriexec/mac_veriexec.c b/sys/security/mac_veriexec/mac_veriexec.c
> index e377f61ad21c..b20df7d694ef 100644
> --- a/sys/security/mac_veriexec/mac_veriexec.c
> +++ b/sys/security/mac_veriexec/mac_veriexec.c
> @@ -51,6 +51,7 @@
> #include <sys/sysctl.h>
> #include <sys/vnode.h>
> #include <fs/nullfs/null.h>
> +#include <security/mac/mac_framework.h>
> #include <security/mac/mac_policy.h>
> 
> #include "mac_veriexec.h"
> @@ -430,6 +431,18 @@ mac_veriexec_priv_check(struct ucred *cred, int priv)
> 	return (0);
> }
> 
> +/**
> + * @internal
> + * @brief Check if the requested sysctl should be allowed
> + *
> + * @param cred         credentials to use
> + * @param oidp         sysctl OID
> + * @param arg1         first sysctl argument
> + * @param arg2         second sysctl argument
> + * @param req          sysctl request information
> + *
> + * @return 0 if the sysctl should be allowed, otherwise an error code.
> + */
> static int
> mac_veriexec_sysctl_check(struct ucred *cred, struct sysctl_oid *oidp,
>     void *arg1, int arg2, struct sysctl_req *req)
> @@ -533,6 +546,9 @@ mac_veriexec_check_vp(struct ucred *cred, struct vnode *vp, accmode_t accmode)
> 				return (error);
> 			break;
> 		default:
> +			/* Allow for overriding verification requirement */
> +			if (mac_priv_grant(cred, PRIV_VERIEXEC_NOVERIFY) == 0)
> +				return (0);
> 			/*
> 			 * Caller wants open to fail unless there is a valid
> 			 * fingerprint registered.
> diff --git a/sys/security/mac_veriexec/veriexec_fingerprint.c b/sys/security/mac_veriexec/veriexec_fingerprint.c
> index 29b5c19eed1e..500842cbd5ab 100644
> --- a/sys/security/mac_veriexec/veriexec_fingerprint.c
> +++ b/sys/security/mac_veriexec/veriexec_fingerprint.c
> @@ -42,11 +42,14 @@
> #include <sys/malloc.h>
> #include <sys/mount.h> 
> #include <sys/mutex.h>
> +#include <sys/priv.h>
> #include <sys/proc.h>
> #include <sys/sbuf.h>
> #include <sys/syslog.h>
> #include <sys/vnode.h>
> 
> +#include <security/mac/mac_framework.h>
> +
> #include "mac_veriexec.h"
> #include "mac_veriexec_internal.h"
> 
> @@ -292,7 +295,8 @@ mac_veriexec_fingerprint_check_image(struct image_params *imgp,
> 
> 	case FINGERPRINT_INDIRECT: /* fingerprint ok but need to check
> 				      for direct execution */
> -		if (!imgp->interpreted) {
> +		if (!imgp->interpreted &&
> +		    mac_priv_grant(td->td_ucred, PRIV_VERIEXEC_DIRECT) != 0) {
> 			identify_error(imgp, td, "attempted direct execution");
> 			if (prison0.pr_securelevel > 1 ||
> 			    mac_veriexec_in_state(VERIEXEC_STATE_ENFORCE))
> @@ -326,6 +330,23 @@ mac_veriexec_fingerprint_check_image(struct image_params *imgp,
> 		identify_error(imgp, td, "invalid status field for vnode");
> 		error = EPERM;
> 	}
> +	switch (status) {
> +	case FINGERPRINT_NODEV:
> +	case FINGERPRINT_NOENTRY:
> +		/*
> +		 * Check if this process has override allowed.
> +		 * This will only be true if PRIV_VERIEXEC_DIRECT
> +		 * already succeeded.
> +		 */
> +		if (error == EAUTH &&
> +		    mac_priv_grant(td->td_ucred, PRIV_VERIEXEC_NOVERIFY) == 0) {
> +			error = 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> 	return error; 
> }
> 
> diff --git a/sys/sys/priv.h b/sys/sys/priv.h
> index cb4dcecea4aa..6574d8c42599 100644
> --- a/sys/sys/priv.h
> +++ b/sys/sys/priv.h
> @@ -520,10 +520,16 @@
>  */
> #define	PRIV_KDB_SET_BACKEND	690	/* Allow setting KDB backend. */
> 
> +/*
> + * veriexec override privileges - very rare!
> + */
> +#define	PRIV_VERIEXEC_DIRECT	700	/* Can override 'indirect' */
> +#define	PRIV_VERIEXEC_NOVERIFY	701	/* Can override O_VERIFY */
> +
> /*
>  * Track end of privilege list.
>  */
> -#define	_PRIV_HIGHEST		691
> +#define	_PRIV_HIGHEST		702
> 
> /*
>  * Validate that a named privilege is known by the privilege system.  Invalid