diff mbox series

[RFC,v9,12/16] fsverity: consume builtin signature via LSM hook

Message ID 1675119451-23180-13-git-send-email-wufan@linux.microsoft.com (mailing list archive)
State Changes Requested
Delegated to: Paul Moore
Headers show
Series Integrity Policy Enforcement LSM (IPE) | expand

Commit Message

Fan Wu Jan. 30, 2023, 10:57 p.m. UTC 
fsverity represents a mechanism to support both integrity and
authenticity protection of a file, supporting both signed and unsigned
digests.

An LSM which controls access to a resource based on authenticity and
integrity of said resource, can then use this data to make an informed
decision on the authorization (provided by the LSM's policy) of said
claim.

This effectively allows the extension of a policy enforcement layer in
LSM for fsverity, allowing for more granular control of how a
particular authenticity claim can be used. For example, "all (built-in)
signed fsverity files should be allowed to execute, but only these
hashes are allowed to be loaded as kernel modules".

This enforcement must be done in kernel space, as a userspace only
solution would fail a simple litmus test: Download a self-contained
malicious binary that never touches the userspace stack. This
binary would still be able to execute.

Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
---
v1-v6:
  + Not present

v7:
  Introduced

v8:
  + Split fs/verity/ changes and security/ changes into separate patches
  + Change signature of fsverity_create_info to accept non-const inode
  + Change signature of fsverity_verify_signature to accept non-const inode
  + Don't cast-away const from inode.
  + Digest functionality dropped in favor of:
    ("fs-verity: define a function to return the integrity protected
      file digest")
  + Reworded commit description and title to match changes.
  + Fix a bug wherein no LSM implements the particular fsverity @name
    (or LSM is disabled), and returns -EOPNOTSUPP, causing errors.

v9:
  + No changes
---
 fs/verity/fsverity_private.h |  2 +-
 fs/verity/open.c             | 13 ++++++++++++-
 fs/verity/signature.c        |  1 +
 include/linux/fsverity.h     |  2 ++
 4 files changed, 16 insertions(+), 2 deletions(-)

Comments

Eric Biggers Feb. 9, 2023, 3:30 a.m. UTC | #1 
So disregarding the fact that using the fsverity builtin signatures still seems
like a bad idea to me, here's a few comments on the diff itself:

On Mon, Jan 30, 2023 at 02:57:27PM -0800, Fan Wu wrote:
> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index 81ff94442f7b..7e6fa52c0e9c 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -7,7 +7,9 @@
>  
>  #include "fsverity_private.h"
>  
> +#include <linux/security.h>
>  #include <linux/slab.h>
> +#include <crypto/public_key.h>

There's no need to include <crypto/public_key.h>.

>  
>  static struct kmem_cache *fsverity_info_cachep;
>  
> @@ -146,7 +148,7 @@ static int compute_file_digest(struct fsverity_hash_alg *hash_alg,
>   * appended signature), and check the signature if present.  The
>   * fsverity_descriptor must have already undergone basic validation.
>   */
> -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> +struct fsverity_info *fsverity_create_info(struct inode *inode,
>  					   struct fsverity_descriptor *desc)
>  {
>  	struct fsverity_info *vi;
> @@ -182,6 +184,15 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
>  
>  	err = fsverity_verify_signature(vi, desc->signature,
>  					le32_to_cpu(desc->sig_size));
> +	if (err) {
> +		fsverity_err(inode, "Error %d verifying signature", err);
> +		goto out;
> +	}

The above error message is unnecessary because fsverity_verify_signature()
already prints an error message on failure.

> +
> +	err = security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME, desc->signature,
> +					 le32_to_cpu(desc->sig_size), 0);

This runs even if CONFIG_FS_VERITY_BUILTIN_SIGNATURES is disabled.  Is that
really the right behavior?

Also a nit: please stick to the preferred line length of 80 characters.
See Documentation/process/coding-style.rst

> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index 143a530a8008..5d7b9496f9c4 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/cred.h>
>  #include <linux/key.h>
> +#include <linux/security.h>
>  #include <linux/slab.h>
>  #include <linux/verification.h>

This change is unnecessary.

> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index 40f14e5fed9d..29e9888287ba 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -254,4 +254,6 @@ static inline bool fsverity_active(const struct inode *inode)
>  	return fsverity_get_info(inode) != NULL;
>  }
>  
> +#define FS_VERITY_INODE_SEC_NAME "fsverity.inode-info"

"inode-info" is very vague.  Shouldn't it be named "builtin-sig" or something?

- Eric
Fan Wu Feb. 9, 2023, 10:21 p.m. UTC | #2 
On Wed, Feb 08, 2023 at 07:30:33PM -0800, Eric Biggers wrote:
> So disregarding the fact that using the fsverity builtin signatures still seems
> like a bad idea to me, here's a few comments on the diff itself:
> 
Thanks for the review. I have verified the headers are indeed unnecessary,
I will remove them in the next version.

> On Mon, Jan 30, 2023 at 02:57:27PM -0800, Fan Wu wrote:
> > diff --git a/fs/verity/open.c b/fs/verity/open.c
> > index 81ff94442f7b..7e6fa52c0e9c 100644
> > --- a/fs/verity/open.c
> > +++ b/fs/verity/open.c
> > @@ -7,7 +7,9 @@
> >  
> >  #include "fsverity_private.h"
> >  
> > +#include <linux/security.h>
> >  #include <linux/slab.h>
> > +#include <crypto/public_key.h>
> 
> There's no need to include <crypto/public_key.h>.
> 
> >  
> > +	if (err) {
> > +		fsverity_err(inode, "Error %d verifying signature", err);
> > +		goto out;
> > +	}
> 
> The above error message is unnecessary because fsverity_verify_signature()
> already prints an error message on failure.
> 
> > +
> > +	err = security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME, desc->signature,
> > +					 le32_to_cpu(desc->sig_size), 0);
> 
> This runs even if CONFIG_FS_VERITY_BUILTIN_SIGNATURES is disabled.  Is that
> really the right behavior?
> 
Yes the hook call should better depend on a KCONFIG. After second thought I think it
should depend on CONFIG_IPE_PROP_FS_VERITY, which also indirectly introduces the
dependency on CONFIG_FS_VERITY_BUILTIN_SIGNATURES.

Currently security_inode_setsecurity only allows one LSM to save data with a given name.
In our case IPE will be the only LSM that saves the signature.

I will update this part in the next version.

> Also a nit: please stick to the preferred line length of 80 characters.
> See Documentation/process/coding-style.rst
> 
> > diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> > index 143a530a8008..5d7b9496f9c4 100644
> > --- a/fs/verity/signature.c
> > +++ b/fs/verity/signature.c
> > @@ -9,6 +9,7 @@
> >  
> >  #include <linux/cred.h>
> >  #include <linux/key.h>
> > +#include <linux/security.h>
> >  #include <linux/slab.h>
> >  #include <linux/verification.h>
> 
> This change is unnecessary.
> 
> > diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> > index 40f14e5fed9d..29e9888287ba 100644
> > --- a/include/linux/fsverity.h
> > +++ b/include/linux/fsverity.h
> > @@ -254,4 +254,6 @@ static inline bool fsverity_active(const struct inode *inode)
> >  	return fsverity_get_info(inode) != NULL;
> >  }
> >  
> > +#define FS_VERITY_INODE_SEC_NAME "fsverity.inode-info"
> 
> "inode-info" is very vague.  Shouldn't it be named "builtin-sig" or something?
> 
> - Eric

I agree this name works better, I will change it to "fsverity.builtin-sig".
-Fan
diff mbox series

Patch

diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index c7fcb855e068..3194a1f4a705 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -117,7 +117,7 @@  int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
 				     unsigned int log_blocksize,
 				     const u8 *salt, size_t salt_size);
 
-struct fsverity_info *fsverity_create_info(const struct inode *inode,
+struct fsverity_info *fsverity_create_info(struct inode *inode,
 					   struct fsverity_descriptor *desc);
 
 void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
diff --git a/fs/verity/open.c b/fs/verity/open.c
index 81ff94442f7b..7e6fa52c0e9c 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -7,7 +7,9 @@ 
 
 #include "fsverity_private.h"
 
+#include <linux/security.h>
 #include <linux/slab.h>
+#include <crypto/public_key.h>
 
 static struct kmem_cache *fsverity_info_cachep;
 
@@ -146,7 +148,7 @@  static int compute_file_digest(struct fsverity_hash_alg *hash_alg,
  * appended signature), and check the signature if present.  The
  * fsverity_descriptor must have already undergone basic validation.
  */
-struct fsverity_info *fsverity_create_info(const struct inode *inode,
+struct fsverity_info *fsverity_create_info(struct inode *inode,
 					   struct fsverity_descriptor *desc)
 {
 	struct fsverity_info *vi;
@@ -182,6 +184,15 @@  struct fsverity_info *fsverity_create_info(const struct inode *inode,
 
 	err = fsverity_verify_signature(vi, desc->signature,
 					le32_to_cpu(desc->sig_size));
+	if (err) {
+		fsverity_err(inode, "Error %d verifying signature", err);
+		goto out;
+	}
+
+	err = security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME, desc->signature,
+					 le32_to_cpu(desc->sig_size), 0);
+	if (err == -EOPNOTSUPP)
+		err = 0;
 out:
 	if (err) {
 		fsverity_free_info(vi);
diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index 143a530a8008..5d7b9496f9c4 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -9,6 +9,7 @@ 
 
 #include <linux/cred.h>
 #include <linux/key.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/verification.h>
 
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 40f14e5fed9d..29e9888287ba 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -254,4 +254,6 @@  static inline bool fsverity_active(const struct inode *inode)
 	return fsverity_get_info(inode) != NULL;
 }
 
+#define FS_VERITY_INODE_SEC_NAME "fsverity.inode-info"
+
 #endif	/* _LINUX_FSVERITY_H */