Re: [PATCH RFC v12 15/20] ipe: add support for dm-verity as a trust provider

[Paul Moore]

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