On Mon, Feb 5, 2024 at 6:11 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> On 2/3/2024 2:25 PM, Paul Moore wrote:
> > On Jan 30, 2024 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> Allows author of IPE policy to indicate trust for a singular dm-verity
> >> volume, identified by roothash, through "dmverity_roothash" and all
> >> signed dm-verity volumes, through "dmverity_signature".
> >>
> >> Signed-off-by: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >> v2:
> >> + No Changes
> >>
> >> v3:
> >> + No changes
> >>
> >> v4:
> >> + No changes
> >>
> >> v5:
> >> + No changes
> >>
> >> v6:
> >> + Fix an improper cleanup that can result in
> >> a leak
> >>
> >> v7:
> >> + Squash patch 08/12, 10/12 to [11/16]
> >>
> >> v8:
> >> + Undo squash of 08/12, 10/12 - separating drivers/md/ from security/
> >> & block/
> >> + Use common-audit function for dmverity_signature.
> >> + Change implementation for storing the dm-verity digest to use the
> >> newly introduced dm_verity_digest structure introduced in patch
> >> 14/20.
> >>
> >> v9:
> >> + Adapt to the new parser
> >>
> >> v10:
> >> + Select the Kconfig when all dependencies are enabled
> >>
> >> v11:
> >> + No changes
> >>
> >> v12:
> >> + Refactor to use struct digest_info* instead of void*
> >> + Correct audit format
> >> ---
> >> security/ipe/Kconfig | 18 ++++++
> >> security/ipe/Makefile | 1 +
> >> security/ipe/audit.c | 37 ++++++++++-
> >> security/ipe/digest.c | 120 +++++++++++++++++++++++++++++++++++
> >> security/ipe/digest.h | 26 ++++++++
> >> security/ipe/eval.c | 90 +++++++++++++++++++++++++-
> >> security/ipe/eval.h | 10 +++
> >> security/ipe/hooks.c | 67 +++++++++++++++++++
> >> security/ipe/hooks.h | 8 +++
> >> security/ipe/ipe.c | 15 +++++
> >> security/ipe/ipe.h | 4 ++
> >> security/ipe/policy.h | 3 +
> >> security/ipe/policy_parser.c | 26 +++++++-
> >> 13 files changed, 421 insertions(+), 4 deletions(-)
> >> create mode 100644 security/ipe/digest.c
> >> create mode 100644 security/ipe/digest.h
> >>
> >> diff --git a/security/ipe/Kconfig b/security/ipe/Kconfig
> >> index ac4d558e69d5..7afb1ce0cb99 100644
> >> --- a/security/ipe/Kconfig
> >> +++ b/security/ipe/Kconfig
> >> @@ -8,6 +8,7 @@ menuconfig SECURITY_IPE
> >> depends on SECURITY && SECURITYFS && AUDIT && AUDITSYSCALL
> >> select PKCS7_MESSAGE_PARSER
> >> select SYSTEM_DATA_VERIFICATION
> >> + select IPE_PROP_DM_VERITY if DM_VERITY && DM_VERITY_VERIFY_ROOTHASH_SIG
> >> help
> >> This option enables the Integrity Policy Enforcement LSM
> >> allowing users to define a policy to enforce a trust-based access
> >> @@ -15,3 +16,20 @@ menuconfig SECURITY_IPE
> >> admins to reconfigure trust requirements on the fly.
> >>
> >> If unsure, answer N.
> >> +
> >> +if SECURITY_IPE
> >> +menu "IPE Trust Providers"
> >> +
> >> +config IPE_PROP_DM_VERITY
> >> + bool "Enable support for dm-verity volumes"
> >> + depends on DM_VERITY && DM_VERITY_VERIFY_ROOTHASH_SIG
> >> + help
> >> + This option enables the properties 'dmverity_signature' and
> >> + 'dmverity_roothash' in IPE policy. These properties evaluates
> >> + to TRUE when a file is evaluated against a dm-verity volume
> >> + that was mounted with a signed root-hash or the volume's
> >> + root hash matches the supplied value in the policy.
> >> +
> >> +endmenu
> >> +
> >> +endif
> >> diff --git a/security/ipe/Makefile b/security/ipe/Makefile
> >> index 2279eaa3cea3..66de53687d11 100644
> >> --- a/security/ipe/Makefile
> >> +++ b/security/ipe/Makefile
> >> @@ -6,6 +6,7 @@
> >> #
> >>
> >> obj-$(CONFIG_SECURITY_IPE) += \
> >> + digest.o \
> >> eval.o \
> >> hooks.o \
> >> fs.o \
> >> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> >> index ed390d32c641..a4ad8e888df0 100644
> >> --- a/security/ipe/audit.c
> >> +++ b/security/ipe/audit.c
> >> @@ -13,6 +13,7 @@
> >> #include "hooks.h"
> >> #include "policy.h"
> >> #include "audit.h"
> >> +#include "digest.h"
> >>
> >> #define ACTSTR(x) ((x) == IPE_ACTION_ALLOW ? "ALLOW" : "DENY")
> >>
> >> @@ -54,8 +55,30 @@ static const char *const audit_prop_names[__IPE_PROP_MAX] = {
> >> "boot_verified=FALSE",
> >> "boot_verified=TRUE",
> >> #endif /* CONFIG_BLK_DEV_INITRD */
> >> +#ifdef CONFIG_IPE_PROP_DM_VERITY
> >> + "dmverity_roothash=",
> >> + "dmverity_signature=FALSE",
> >> + "dmverity_signature=TRUE",
> >> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
> >> };
> >>
> >> +#ifdef CONFIG_IPE_PROP_DM_VERITY
> >> +/**
> >> + * audit_dmv_roothash - audit a roothash of a dmverity volume.
> >> + * @ab: Supplies a pointer to the audit_buffer to append to.
> >> + * @rh: Supplies a pointer to the digest structure.
> >> + */
> >> +static void audit_dmv_roothash(struct audit_buffer *ab, const void *rh)
> >> +{
> >> + audit_log_format(ab, "%s", audit_prop_names[IPE_PROP_DMV_ROOTHASH]);
> >> + ipe_digest_audit(ab, rh);
> >> +}
> >> +#else
> >> +static void audit_dmv_roothash(struct audit_buffer *ab, const void *rh)
> >> +{
> >> +}
> >> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
> >
> > I talked about this back in my review of the v11 patchset and I'm
> > guessing you may have missed it ... the problem with the above code is
> > that the fields in an audit record should remain constant, even if
> > there is no data for that particular field. In cases where there is no
> > data to record for a given field, a "?" should be used as the field's
> > value, for example:
> >
> > dmverify_roothash=?
> >
> > My guess is that you would want to do something like this:
> >
> > #else /* !CONFIG_IPE_PROP_DM_VERITY */
> > static void audit_dmv_roothash(...)
> > {
> > audit_log_format(ab, "%s=?", audit_prop_names[...]);
> > }
> > #endif /* CONFIG_IPE_PROP_DM_VERITY */
> >
> > --
> > paul-moore.com
>
> These code are used for auditing a policy rule, which the parser will
> guarantee the property will always have a valid value. The comments
> might be misleading which sounds like it's auditing a file's state. I
> will correct them.
>
> Also as we previously discussed, the policy grammar shouldn't depend on
> any kernel switch so these preprocessor statement will be removed.
>
> However, as an audit record should remain constant, I guess we should do
> some special treatment to anonymous files? Like audit record for them
> should include "path=? dev=? ino=?"
Yes, if the record type includes those fields just once, the record
type should *always* include those fields.
--
paul-moore.com
