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

Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> · Mon, 5 Feb 2024 15:11:17 -0800

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=?"

Thanks,
Fan







