Support for fs-verity file digests in IMA was discussed from the beginning,
prior to fs-verity being upstreamed[1,2]. This patch set adds signature
verification support based on the fs-verity file digest. Both the
file digest and the signature must be included in the IMA measurement list
in order to disambiguate the type of file digest.
[1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
[2] Documentation/filesystems/fsverity.rst
Mimi Zohar (4):
fs-verity: define a function to return the integrity protected file
digest
ima: define a new signature type named IMA_VERITY_DIGSIG
ima: limit including fs-verity's file digest in measurement list
ima: support fs-verity file digest based signatures
fs/verity/fsverity_private.h | 6 ---
fs/verity/measure.c | 49 +++++++++++++++++++++++
include/linux/fsverity.h | 17 ++++++++
security/integrity/ima/ima.h | 3 +-
security/integrity/ima/ima_api.c | 23 ++++++++++-
security/integrity/ima/ima_appraise.c | 9 ++++-
security/integrity/ima/ima_main.c | 7 +++-
security/integrity/ima/ima_template_lib.c | 3 +-
security/integrity/integrity.h | 1 +
9 files changed, 107 insertions(+), 11 deletions(-)
--
2.27.0
Without the file signature included the IMA measurement list, the type
of file digest is unclear. Limit including fs-verity's file digest in
the IMA measurement list based on whether the template name is ima-sig.
In the future, this could be relaxed to include any template format that
includes the file signature.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/ima.h | 3 ++-
security/integrity/ima/ima_api.c | 3 ++-
security/integrity/ima/ima_appraise.c | 3 ++-
security/integrity/ima/ima_main.c | 7 ++++++-
security/integrity/ima/ima_template_lib.c | 3 ++-
5 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index be965a8715e4..ab257e404f8e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -262,7 +262,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
- enum hash_algo algo, struct modsig *modsig);
+ enum hash_algo algo, struct modsig *modsig,
+ bool veritysig);
void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 42c6ff7056e6..179c7f0364c2 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -217,7 +217,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
*/
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
- enum hash_algo algo, struct modsig *modsig)
+ enum hash_algo algo, struct modsig *modsig,
+ bool veritysig)
{
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d43a27a9a9b6..b31be383e668 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -510,7 +510,8 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
!(iint->flags & IMA_HASH))
return;
- rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo, NULL);
+ rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo,
+ NULL, FALSE);
if (rc < 0)
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 465865412100..a73e1e845ea8 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -216,6 +216,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
bool violation_check;
enum hash_algo hash_algo;
unsigned int allowed_algos = 0;
+ int veritysig = FALSE;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -333,8 +334,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
}
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
+ if (xattr_value && xattr_value->type == IMA_VERITY_DIGSIG &&
+ strcmp(template_desc->name, "ima-sig") == 0)
+ veritysig = TRUE;
- rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
+ rc = ima_collect_measurement(iint, file, buf, size, hash_algo,
+ modsig, veritysig);
if (rc != 0 && rc != -EBADF && rc != -EINVAL)
goto out_locked;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index ca017cae73eb..5bad251f3b07 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -478,7 +478,8 @@ int ima_eventsig_init(struct ima_event_data *event_data,
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
- if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+ if ((!xattr_value) || !(xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+ xattr_value->type == IMA_VERITY_DIGSIG))
return ima_eventevmsig_init(event_data, field_data);
return ima_write_template_field_data(xattr_value, event_data->xattr_len,
--
2.27.0
To differentiate between a regular file hash and an fs-verity file digest
based signature stored as security.ima xattr, define a new signature type
named IMA_VERITY_DIGSIG.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/ima_appraise.c | 6 ++++++
security/integrity/integrity.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index dbba51583e7c..d43a27a9a9b6 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -13,7 +13,9 @@
#include <linux/magic.h>
#include <linux/ima.h>
#include <linux/evm.h>
+#include <linux/fsverity.h>
#include <keys/system_keyring.h>
+#include <uapi/linux/fsverity.h>
#include "ima.h"
@@ -183,6 +185,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
return ima_hash_algo;
switch (xattr_value->type) {
+ case IMA_VERITY_DIGSIG:
+ fallthrough;
case EVM_IMA_XATTR_DIGSIG:
sig = (typeof(sig))xattr_value;
if (sig->version != 2 || xattr_len <= sizeof(*sig)
@@ -272,6 +276,8 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
}
*status = INTEGRITY_PASS;
break;
+ case IMA_VERITY_DIGSIG:
+ fallthrough;
case EVM_IMA_XATTR_DIGSIG:
set_bit(IMA_DIGSIG, &iint->atomic_flags);
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..94f9ba55e840 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -77,6 +77,7 @@ enum evm_ima_xattr_type {
EVM_IMA_XATTR_DIGSIG,
IMA_XATTR_DIGEST_NG,
EVM_XATTR_PORTABLE_DIGSIG,
+ IMA_VERITY_DIGSIG,
IMA_XATTR_LAST
};
--
2.27.0
Instead of calculating a file hash and verifying the signature stored
in the security.ima xattr against the calculated file hash, verify the
signature of the fs-verity's file digest. The fs-verity file digest is
a hash that includes the Merkle tree root hash.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/ima_api.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 179c7f0364c2..ee1701f8c0f3 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -16,6 +16,7 @@
#include <linux/xattr.h>
#include <linux/evm.h>
#include <linux/iversion.h>
+#include <linux/fsverity.h>
#include "ima.h"
@@ -205,6 +206,23 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
allowed_algos);
}
+static int ima_collect_verity_measurement(struct integrity_iint_cache *iint,
+ struct ima_digest_data *hash)
+{
+ u8 verity_digest[FS_VERITY_MAX_DIGEST_SIZE];
+ enum hash_algo verity_alg;
+ int rc;
+
+ rc = fsverity_measure(iint->inode, verity_digest, &verity_alg);
+ if (rc)
+ return -EINVAL;
+ if (hash->algo != verity_alg)
+ return -EINVAL;
+ hash->length = hash_digest_size[verity_alg];
+ memcpy(hash->digest, verity_digest, hash->length);
+ return 0;
+}
+
/*
* ima_collect_measurement - collect file measurement
*
@@ -256,6 +274,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
if (buf)
result = ima_calc_buffer_hash(buf, size, &hash.hdr);
+ else if (veritysig)
+ result = ima_collect_verity_measurement(iint, &hash.hdr);
else
result = ima_calc_file_hash(file, &hash.hdr);
--
2.27.0
Define a function named fsverity_measure() to return the verity file digest
and the associated hash algorithm (enum hash_algo).
Signed-off-by: Mimi Zohar <[email protected]>
---
fs/verity/fsverity_private.h | 6 -----
fs/verity/measure.c | 49 ++++++++++++++++++++++++++++++++++++
include/linux/fsverity.h | 17 +++++++++++++
3 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index a7920434bae5..54c5f0993541 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -26,12 +26,6 @@ struct ahash_request;
*/
#define FS_VERITY_MAX_LEVELS 8
-/*
- * Largest digest size among all hash algorithms supported by fs-verity.
- * Currently assumed to be <= size of fsverity_descriptor::root_hash.
- */
-#define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
-
/* A hash algorithm supported by fs-verity */
struct fsverity_hash_alg {
struct crypto_ahash *tfm; /* hash tfm, allocated on demand */
diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index f0d7b30c62db..98d8f6f2a2be 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -57,3 +57,52 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
return 0;
}
EXPORT_SYMBOL_GPL(fsverity_ioctl_measure);
+
+/**
+ * fsverity_measure() - get a verity file's digest
+ * @inode: inode to get digest of
+ * @digest: pointer to the digest
+ * @alg: pointer to the hash algorithm enumeration
+ *
+ * Return the file hash algorithm, digest size, and digest of an fsverity
+ * protected file.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fsverity_measure(struct inode *inode, u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+ enum hash_algo *alg)
+{
+ const struct fsverity_info *vi;
+ const struct fsverity_hash_alg *hash_alg;
+ int i;
+
+ vi = fsverity_get_info(inode);
+ if (!vi)
+ return -ENODATA; /* not a verity file */
+
+ hash_alg = vi->tree_params.hash_alg;
+ memset(digest, 0, FS_VERITY_MAX_DIGEST_SIZE);
+ *alg = HASH_ALGO__LAST;
+
+ /* convert hash algorithm to hash_algo_name */
+ for (i = 0; i < HASH_ALGO__LAST; i++) {
+ pr_debug("name %s hash_algo_name[%d] %s\n",
+ hash_alg->name, i, hash_algo_name[i]);
+
+ if (!strcmp(hash_alg->name, hash_algo_name[i])) {
+ *alg = i;
+ break;
+ }
+ }
+
+ /* Shouldn't happen */
+ if (*alg == HASH_ALGO__LAST)
+ return -EINVAL;
+
+ memcpy(digest, vi->file_digest, hash_alg->digest_size);
+
+ pr_debug("file digest:%s %*phN\n", hash_algo_name[*alg],
+ hash_digest_size[*alg], digest);
+
+ return 0;
+}
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index b568b3c7d095..11006b60713b 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -12,8 +12,16 @@
#define _LINUX_FSVERITY_H
#include <linux/fs.h>
+#include <crypto/hash_info.h>
+#include <crypto/sha2.h>
#include <uapi/linux/fsverity.h>
+/*
+ * Largest digest size among all hash algorithms supported by fs-verity.
+ * Currently assumed to be <= size of fsverity_descriptor::root_hash.
+ */
+#define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
+
/* Verity operations for filesystems */
struct fsverity_operations {
@@ -131,6 +139,8 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
/* measure.c */
int fsverity_ioctl_measure(struct file *filp, void __user *arg);
+int fsverity_measure(struct inode *inode, u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+ enum hash_algo *alg);
/* open.c */
@@ -170,6 +180,13 @@ static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
return -EOPNOTSUPP;
}
+static inline int fsverity_measure(struct inode *inode,
+ u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+ enum hash_algo *alg)
+{
+ return -EOPNOTSUPP;
+}
+
/* open.c */
static inline int fsverity_file_open(struct inode *inode, struct file *filp)
--
2.27.0
Hi Mimi,
I love your patch! Yet something to improve:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on fscrypt/fsverity jmorris-security/next-testing v5.16-rc3 next-20211129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mimi-Zohar/ima-support-fs-verity-signatures-stored-as/20211130-010506
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: i386-randconfig-m021-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/305ad3bb36a5bd51fe30b33f623eaf165e2efb13
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mimi-Zohar/ima-support-fs-verity-signatures-stored-as/20211130-010506
git checkout 305ad3bb36a5bd51fe30b33f623eaf165e2efb13
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: fs/verity/measure.o: in function `fsverity_measure':
fs/verity/measure.c:89: undefined reference to `hash_algo_name'
>> ld: fs/verity/measure.c:104: undefined reference to `hash_digest_size'
ld: fs/verity/measure.c:104: undefined reference to `hash_algo_name'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Mimi,
I love your patch! Yet something to improve:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on v5.16-rc3 next-20211129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mimi-Zohar/ima-support-fs-verity-signatures-stored-as/20211130-010506
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: arm64-randconfig-r032-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/305ad3bb36a5bd51fe30b33f623eaf165e2efb13
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mimi-Zohar/ima-support-fs-verity-signatures-stored-as/20211130-010506
git checkout 305ad3bb36a5bd51fe30b33f623eaf165e2efb13
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
aarch64-linux-ld: fs/verity/measure.o: in function `fsverity_measure':
>> (.text+0x968): undefined reference to `hash_algo_name'
aarch64-linux-ld: fs/verity/measure.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `hash_algo_name' which may bind externally can not be used when making a shared object; recompile with -fPIC
(.text+0x968): dangerous relocation: unsupported relocation
>> aarch64-linux-ld: (.text+0x96c): undefined reference to `hash_algo_name'
>> aarch64-linux-ld: (.text+0xa8c): undefined reference to `hash_digest_size'
aarch64-linux-ld: fs/verity/measure.o: relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol `hash_digest_size' which may bind externally can not be used when making a shared object; recompile with -fPIC
(.text+0xa8c): dangerous relocation: unsupported relocation
aarch64-linux-ld: (.text+0xa9c): undefined reference to `hash_digest_size'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Generally looks fine. A few nits below:
On Mon, Nov 29, 2021 at 12:00:54PM -0500, Mimi Zohar wrote:
> Define a function named fsverity_measure() to return the verity file digest
> and the associated hash algorithm (enum hash_algo).
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> fs/verity/fsverity_private.h | 6 -----
> fs/verity/measure.c | 49 ++++++++++++++++++++++++++++++++++++
> include/linux/fsverity.h | 17 +++++++++++++
> 3 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> index a7920434bae5..54c5f0993541 100644
> --- a/fs/verity/fsverity_private.h
> +++ b/fs/verity/fsverity_private.h
> @@ -26,12 +26,6 @@ struct ahash_request;
> */
> #define FS_VERITY_MAX_LEVELS 8
>
> -/*
> - * Largest digest size among all hash algorithms supported by fs-verity.
> - * Currently assumed to be <= size of fsverity_descriptor::root_hash.
> - */
> -#define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
The include of sha2.h should be removed from this file.
> +/**
> + * fsverity_measure() - get a verity file's digest
> + * @inode: inode to get digest of
> + * @digest: pointer to the digest
> + * @alg: pointer to the hash algorithm enumeration
It should be made clear that @digest and @alg are output, for example:
* @digest: (out) pointer to the digest
* @alg: (out) pointer to the hash algorithm enumeration
> + * Return the file hash algorithm, digest size, and digest of an fsverity
> + * protected file.
The digest size is implied, not returned.
> +
> + if (!strcmp(hash_alg->name, hash_algo_name[i])) {
As the kernel test robot pointed out, this creates a dependency on
CRYPTO_HASH_INFO. So FS_VERITY will need to select CRYPTO_HASH_INFO.
- Eric
On Mon, Nov 29, 2021 at 12:00:55PM -0500, Mimi Zohar wrote:
> To differentiate between a regular file hash and an fs-verity file digest
> based signature stored as security.ima xattr, define a new signature type
> named IMA_VERITY_DIGSIG.
>
> Signed-off-by: Mimi Zohar <[email protected]>
For this new signature type, what bytes are actually signed? It looks like it's
just the raw digest, which isn't sufficient since it is ambiguous. It needs to
include information that makes it clear what the signer is actually signing,
such as "this is an fs-verity SHA-256 file digest". See
'struct fsverity_formatted_digest' for an example of this (but it isn't
necessary to use that exact structure).
I think the existing IMA signatures have the same problem (but it is hard for me
to understand the code). However, a new signature type doesn't have
backwards-compatibility concerns, so it could be done right.
- Eric
On Mon, Nov 29, 2021 at 12:00:56PM -0500, Mimi Zohar wrote:
> Without the file signature included the IMA measurement list, the type
> of file digest is unclear. Limit including fs-verity's file digest in
> the IMA measurement list based on whether the template name is ima-sig.
> In the future, this could be relaxed to include any template format that
> includes the file signature.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> security/integrity/ima/ima.h | 3 ++-
> security/integrity/ima/ima_api.c | 3 ++-
> security/integrity/ima/ima_appraise.c | 3 ++-
> security/integrity/ima/ima_main.c | 7 ++++++-
> security/integrity/ima/ima_template_lib.c | 3 ++-
> 5 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index be965a8715e4..ab257e404f8e 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -262,7 +262,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file, void *buf, loff_t size,
> - enum hash_algo algo, struct modsig *modsig);
> + enum hash_algo algo, struct modsig *modsig,
> + bool veritysig);
> void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> const unsigned char *filename,
> struct evm_ima_xattr_data *xattr_value,
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 42c6ff7056e6..179c7f0364c2 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,7 +217,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> */
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file, void *buf, loff_t size,
> - enum hash_algo algo, struct modsig *modsig)
> + enum hash_algo algo, struct modsig *modsig,
> + bool veritysig)
'veritysig' is being added here but it doesn't actually do anything. It seems
this patchset is not split up correctly.
> + rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo,
> + NULL, FALSE);
> if (rc < 0)
> return;
false should be used instead of FALSE.
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 465865412100..a73e1e845ea8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -216,6 +216,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
> bool violation_check;
> enum hash_algo hash_algo;
> unsigned int allowed_algos = 0;
> + int veritysig = FALSE;
Likewise.
> + if (xattr_value && xattr_value->type == IMA_VERITY_DIGSIG &&
> + strcmp(template_desc->name, "ima-sig") == 0)
> + veritysig = TRUE;
Likewise, true instead of TRUE.
- Eric
On Mon, Nov 29, 2021 at 12:00:53PM -0500, Mimi Zohar wrote:
> Support for fs-verity file digests in IMA was discussed from the beginning,
> prior to fs-verity being upstreamed[1,2]. This patch set adds signature
> verification support based on the fs-verity file digest. Both the
> file digest and the signature must be included in the IMA measurement list
> in order to disambiguate the type of file digest.
>
> [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
> [2] Documentation/filesystems/fsverity.rst
>
> Mimi Zohar (4):
> fs-verity: define a function to return the integrity protected file
> digest
> ima: define a new signature type named IMA_VERITY_DIGSIG
> ima: limit including fs-verity's file digest in measurement list
> ima: support fs-verity file digest based signatures
>
> fs/verity/fsverity_private.h | 6 ---
> fs/verity/measure.c | 49 +++++++++++++++++++++++
> include/linux/fsverity.h | 17 ++++++++
> security/integrity/ima/ima.h | 3 +-
> security/integrity/ima/ima_api.c | 23 ++++++++++-
> security/integrity/ima/ima_appraise.c | 9 ++++-
> security/integrity/ima/ima_main.c | 7 +++-
> security/integrity/ima/ima_template_lib.c | 3 +-
> security/integrity/integrity.h | 1 +
> 9 files changed, 107 insertions(+), 11 deletions(-)
I left some comments, but this generally looks like the right approach.
However, I'm not an expert in IMA, so it's hard for me to review the IMA parts.
Can you add documentation for this feature?
- Eric
Hi Mimi,
On 11/29/2021 6:19 PM, Eric Biggers wrote:
> Generally looks fine. A few nits below:
>
> On Mon, Nov 29, 2021 at 12:00:54PM -0500, Mimi Zohar wrote:
>> Define a function named fsverity_measure() to return the verity file digest
>> and the associated hash algorithm (enum hash_algo).
>>
>> Signed-off-by: Mimi Zohar <[email protected]>
>> ---
>> fs/verity/fsverity_private.h | 6 -----
>> fs/verity/measure.c | 49 ++++++++++++++++++++++++++++++++++++
>> include/linux/fsverity.h | 17 +++++++++++++
>> 3 files changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
>> index a7920434bae5..54c5f0993541 100644
>> --- a/fs/verity/fsverity_private.h
>> +++ b/fs/verity/fsverity_private.h
>> @@ -26,12 +26,6 @@ struct ahash_request;
>> */
>> #define FS_VERITY_MAX_LEVELS 8
>>
>> -/*
>> - * Largest digest size among all hash algorithms supported by fs-verity.
>> - * Currently assumed to be <= size of fsverity_descriptor::root_hash.
>> - */
>> -#define FS_VERITY_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
>
> The include of sha2.h should be removed from this file.
>
>> +/**
>> + * fsverity_measure() - get a verity file's digest
nit: The function name seems to suggest it is measuring the fs-verity
file's digest. Since it is reading the file's digest:
fsverity_read_digest() or fsverity_read_measure()?
-lakshmi
>> + * @inode: inode to get digest of
>> + * @digest: pointer to the digest
>> + * @alg: pointer to the hash algorithm enumeration
>
> It should be made clear that @digest and @alg are output, for example:
>
> * @digest: (out) pointer to the digest
> * @alg: (out) pointer to the hash algorithm enumeration
>
>> + * Return the file hash algorithm, digest size, and digest of an fsverity
>> + * protected file.
>
> The digest size is implied, not returned.
>
>> +
>> + if (!strcmp(hash_alg->name, hash_algo_name[i])) {
>
> As the kernel test robot pointed out, this creates a dependency on
> CRYPTO_HASH_INFO. So FS_VERITY will need to select CRYPTO_HASH_INFO.
>
> - Eric
>
Hi Mimi,
Just one nit comment below in the patch description.
On 11/29/2021 9:00 AM, Mimi Zohar wrote:
> Without the file signature included the IMA measurement list, the type
Without the file signature included in the IMA measurement list, the type...
-lakshmi
> of file digest is unclear. Limit including fs-verity's file digest in
> the IMA measurement list based on whether the template name is ima-sig.
> In the future, this could be relaxed to include any template format that
> includes the file signature.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> security/integrity/ima/ima.h | 3 ++-
> security/integrity/ima/ima_api.c | 3 ++-
> security/integrity/ima/ima_appraise.c | 3 ++-
> security/integrity/ima/ima_main.c | 7 ++++++-
> security/integrity/ima/ima_template_lib.c | 3 ++-
> 5 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index be965a8715e4..ab257e404f8e 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -262,7 +262,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file, void *buf, loff_t size,
> - enum hash_algo algo, struct modsig *modsig);
> + enum hash_algo algo, struct modsig *modsig,
> + bool veritysig);
> void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> const unsigned char *filename,
> struct evm_ima_xattr_data *xattr_value,
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 42c6ff7056e6..179c7f0364c2 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,7 +217,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> */
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file, void *buf, loff_t size,
> - enum hash_algo algo, struct modsig *modsig)
> + enum hash_algo algo, struct modsig *modsig,
> + bool veritysig)
> {
> const char *audit_cause = "failed";
> struct inode *inode = file_inode(file);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index d43a27a9a9b6..b31be383e668 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -510,7 +510,8 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
> !(iint->flags & IMA_HASH))
> return;
>
> - rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo, NULL);
> + rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo,
> + NULL, FALSE);
> if (rc < 0)
> return;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 465865412100..a73e1e845ea8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -216,6 +216,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
> bool violation_check;
> enum hash_algo hash_algo;
> unsigned int allowed_algos = 0;
> + int veritysig = FALSE;
>
> if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> return 0;
> @@ -333,8 +334,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
> }
>
> hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> + if (xattr_value && xattr_value->type == IMA_VERITY_DIGSIG &&
> + strcmp(template_desc->name, "ima-sig") == 0)
> + veritysig = TRUE;
>
> - rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
> + rc = ima_collect_measurement(iint, file, buf, size, hash_algo,
> + modsig, veritysig);
> if (rc != 0 && rc != -EBADF && rc != -EINVAL)
> goto out_locked;
>
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index ca017cae73eb..5bad251f3b07 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -478,7 +478,8 @@ int ima_eventsig_init(struct ima_event_data *event_data,
> {
> struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
>
> - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> + if ((!xattr_value) || !(xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
> + xattr_value->type == IMA_VERITY_DIGSIG))
> return ima_eventevmsig_init(event_data, field_data);
>
> return ima_write_template_field_data(xattr_value, event_data->xattr_len,
>
Hi Mimi,
On 11/29/2021 9:00 AM, Mimi Zohar wrote:
> Instead of calculating a file hash and verifying the signature stored
> in the security.ima xattr against the calculated file hash, verify the
> signature of the fs-verity's file digest. The fs-verity file digest is
> a hash that includes the Merkle tree root hash.
This patch is reading the fs-verity signature for the given file using
the new function fsverity_measure() that was defined in [Patch 1/4]. Is
it also verifying the fs-verity signature here?
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> security/integrity/ima/ima_api.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 179c7f0364c2..ee1701f8c0f3 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -16,6 +16,7 @@
> #include <linux/xattr.h>
> #include <linux/evm.h>
> #include <linux/iversion.h>
> +#include <linux/fsverity.h>
>
> #include "ima.h"
>
> @@ -205,6 +206,23 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> allowed_algos);
> }
>
> +static int ima_collect_verity_measurement(struct integrity_iint_cache *iint,
> + struct ima_digest_data *hash)
> +{
> + u8 verity_digest[FS_VERITY_MAX_DIGEST_SIZE];
> + enum hash_algo verity_alg;
> + int rc;
> +
> + rc = fsverity_measure(iint->inode, verity_digest, &verity_alg);
nit: fsverity_collect_measurement() may be more appropriate for this
function (defined in [PATCH 1/4]).
thanks,
-lakshmi
> + if (rc)
> + return -EINVAL;
> + if (hash->algo != verity_alg)
> + return -EINVAL;
> + hash->length = hash_digest_size[verity_alg];
> + memcpy(hash->digest, verity_digest, hash->length);
> + return 0;
> +}
> +
> /*
> * ima_collect_measurement - collect file measurement
> *
> @@ -256,6 +274,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>
> if (buf)
> result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> + else if (veritysig)
> + result = ima_collect_verity_measurement(iint, &hash.hdr);
> else
> result = ima_calc_file_hash(file, &hash.hdr);
>
>
On Mon, Nov 29, 2021 at 09:33:29PM -0800, Lakshmi Ramasubramanian wrote:
> > > +/**
> > > + * fsverity_measure() - get a verity file's digest
> nit: The function name seems to suggest it is measuring the fs-verity file's
> digest. Since it is reading the file's digest: fsverity_read_digest() or
> fsverity_read_measure()?
I suggest fsverity_get_digest(). "read" is misleading because it's not being
read from disk.
- Eric
On Mon, 2021-11-29 at 18:36 -0800, Eric Biggers wrote:
> On Mon, Nov 29, 2021 at 12:00:53PM -0500, Mimi Zohar wrote:
> > Support for fs-verity file digests in IMA was discussed from the beginning,
> > prior to fs-verity being upstreamed[1,2]. This patch set adds signature
> > verification support based on the fs-verity file digest. Both the
> > file digest and the signature must be included in the IMA measurement list
> > in order to disambiguate the type of file digest.
> >
> > [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
> > [2] Documentation/filesystems/fsverity.rst
> >
> > Mimi Zohar (4):
> > fs-verity: define a function to return the integrity protected file
> > digest
> > ima: define a new signature type named IMA_VERITY_DIGSIG
> > ima: limit including fs-verity's file digest in measurement list
> > ima: support fs-verity file digest based signatures
> >
> > fs/verity/fsverity_private.h | 6 ---
> > fs/verity/measure.c | 49 +++++++++++++++++++++++
> > include/linux/fsverity.h | 17 ++++++++
> > security/integrity/ima/ima.h | 3 +-
> > security/integrity/ima/ima_api.c | 23 ++++++++++-
> > security/integrity/ima/ima_appraise.c | 9 ++++-
> > security/integrity/ima/ima_main.c | 7 +++-
> > security/integrity/ima/ima_template_lib.c | 3 +-
> > security/integrity/integrity.h | 1 +
> > 9 files changed, 107 insertions(+), 11 deletions(-)
>
> I left some comments, but this generally looks like the right approach.
> However, I'm not an expert in IMA, so it's hard for me to review the IMA parts.
Thank you for the quick review!
>
> Can you add documentation for this feature?
Yes, of course. Originally I assumed the fs-verity support would be a
lot more complicated, but to my pleasant surprise by limiting the IMA
fsverity support to just signatures and requiring the file signature be
included in the IMA measurement list, it's a lot simpler than expected.
As there aren't any IMA policy changes, I'm just thinking about where
to document it.
thanks,
Mimi
Hi Eric,
On Mon, 2021-11-29 at 18:35 -0800, Eric Biggers wrote:
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index 42c6ff7056e6..179c7f0364c2 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -217,7 +217,8 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
> > */
> > int ima_collect_measurement(struct integrity_iint_cache *iint,
> > struct file *file, void *buf, loff_t size,
> > - enum hash_algo algo, struct modsig *modsig)
> > + enum hash_algo algo, struct modsig *modsig,
> > + bool veritysig)
>
> 'veritysig' is being added here but it doesn't actually do anything. It seems
> this patchset is not split up correctly.
True, this patch just adds the plumbing. Reversing 3 & 4 could result
in including the fs-verity digest, without the signature in the
measurement list. The alternative is to squash patches 3 & 4.
thanks,
Mimi
Hi Lakshmi, Eric,
On Mon, 2021-11-29 at 21:56 -0800, Lakshmi Ramasubramanian wrote:
> Hi Mimi,
>
> On 11/29/2021 9:00 AM, Mimi Zohar wrote:
> > Instead of calculating a file hash and verifying the signature stored
> > in the security.ima xattr against the calculated file hash, verify the
> > signature of the fs-verity's file digest. The fs-verity file digest is
> > a hash that includes the Merkle tree root hash.
> This patch is reading the fs-verity signature for the given file using
> the new function fsverity_measure() that was defined in [Patch 1/4]. Is
> it also verifying the fs-verity signature here?
Yes, the signature stored in the security.ima xattr may be a file hash,
a regular file signature, or a signature of the fs-verity file digest.
The signature is verified like any other signature stored as an xattr.
>
> > +static int ima_collect_verity_measurement(struct integrity_iint_cache *iint,
> > + struct ima_digest_data *hash)
> > +{
> > + u8 verity_digest[FS_VERITY_MAX_DIGEST_SIZE];
> > + enum hash_algo verity_alg;
> > + int rc;
> > +
> > + rc = fsverity_measure(iint->inode, verity_digest, &verity_alg);
> nit: fsverity_collect_measurement() may be more appropriate for this
> function (defined in [PATCH 1/4]).
From an IMA perspective it certainly would be a better function name,
but this function may be used by other kernel subsystems. Eric
suggested renaming the function as fsverity_get_digest(), as opposed to
fsverity_read_digest(). get/put are normally used to bump a reference
count or to get/release a lock. Perhaps a combination like
fsverity_collect_digest() would be acceptable.
thanks,
Mimi
On Mon, 2021-11-29 at 18:33 -0800, Eric Biggers wrote:
> On Mon, Nov 29, 2021 at 12:00:55PM -0500, Mimi Zohar wrote:
> > To differentiate between a regular file hash and an fs-verity file digest
> > based signature stored as security.ima xattr, define a new signature type
> > named IMA_VERITY_DIGSIG.
> >
> > Signed-off-by: Mimi Zohar <[email protected]>
>
> For this new signature type, what bytes are actually signed? It looks like it's
> just the raw digest, which isn't sufficient since it is ambiguous. It needs to
> include information that makes it clear what the signer is actually signing,
> such as "this is an fs-verity SHA-256 file digest". See
> 'struct fsverity_formatted_digest' for an example of this (but it isn't
> necessary to use that exact structure).
>
> I think the existing IMA signatures have the same problem (but it is hard for me
> to understand the code). However, a new signature type doesn't have
> backwards-compatibility concerns, so it could be done right.
As this change should probably be applicable to all signature types,
the signature version in the signature_v2_hdr should be bumped. The
existing signature version could co-exist with the new signature
version.
thanks,
Mimi
On Tue, 2021-11-30 at 07:56 -0500, Mimi Zohar wrote:
> On Mon, 2021-11-29 at 18:36 -0800, Eric Biggers wrote:
> > On Mon, Nov 29, 2021 at 12:00:53PM -0500, Mimi Zohar wrote:
> > > Support for fs-verity file digests in IMA was discussed from the beginning,
> > > prior to fs-verity being upstreamed[1,2]. This patch set adds signature
> > > verification support based on the fs-verity file digest. Both the
> > > file digest and the signature must be included in the IMA measurement list
> > > in order to disambiguate the type of file digest.
> > >
> > > [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
> > > [2] Documentation/filesystems/fsverity.rst
> > >
> > > Mimi Zohar (4):
> > > fs-verity: define a function to return the integrity protected file
> > > digest
> > > ima: define a new signature type named IMA_VERITY_DIGSIG
> > > ima: limit including fs-verity's file digest in measurement list
> > > ima: support fs-verity file digest based signatures
> > >
> > > fs/verity/fsverity_private.h | 6 ---
> > > fs/verity/measure.c | 49 +++++++++++++++++++++++
> > > include/linux/fsverity.h | 17 ++++++++
> > > security/integrity/ima/ima.h | 3 +-
> > > security/integrity/ima/ima_api.c | 23 ++++++++++-
> > > security/integrity/ima/ima_appraise.c | 9 ++++-
> > > security/integrity/ima/ima_main.c | 7 +++-
> > > security/integrity/ima/ima_template_lib.c | 3 +-
> > > security/integrity/integrity.h | 1 +
> > > 9 files changed, 107 insertions(+), 11 deletions(-)
> >
> > I left some comments, but this generally looks like the right approach.
> > However, I'm not an expert in IMA, so it's hard for me to review the IMA parts.
>
> Thank you for the quick review!
>
> >
> > Can you add documentation for this feature?
>
> Yes, of course. Originally I assumed the fs-verity support would be a
> lot more complicated, but to my pleasant surprise by limiting the IMA
> fsverity support to just signatures and requiring the file signature be
> included in the IMA measurement list, it's a lot simpler than expected.
> As there aren't any IMA policy changes, I'm just thinking about where
> to document it.
I'll update both Documentation/filesystems/fsverity.rst and
Documentation/security/IMA-templates.rst.
thanks,
Mimi
Hi Eric,
On Tue, 2021-11-30 at 13:14 -0500, Mimi Zohar wrote:
> On Mon, 2021-11-29 at 18:33 -0800, Eric Biggers wrote:
> > On Mon, Nov 29, 2021 at 12:00:55PM -0500, Mimi Zohar wrote:
> > > To differentiate between a regular file hash and an fs-verity file digest
> > > based signature stored as security.ima xattr, define a new signature type
> > > named IMA_VERITY_DIGSIG.
> > >
> > > Signed-off-by: Mimi Zohar <[email protected]>
> >
> > For this new signature type, what bytes are actually signed? It looks like it's
> > just the raw digest, which isn't sufficient since it is ambiguous. It needs to
> > include information that makes it clear what the signer is actually signing,
> > such as "this is an fs-verity SHA-256 file digest". See
> > 'struct fsverity_formatted_digest' for an example of this (but it isn't
> > necessary to use that exact structure).
> >
> > I think the existing IMA signatures have the same problem (but it is hard for me
> > to understand the code). However, a new signature type doesn't have
> > backwards-compatibility concerns, so it could be done right.
>
> As this change should probably be applicable to all signature types,
> the signature version in the signature_v2_hdr should be bumped. The
> existing signature version could co-exist with the new signature
> version.
By signing the file hash, the sig field in the IMA measurement list can
be directly verified against the digest field. For appended
signatures, we defined a new template named ima-modsig which contains
two file hashes, with and without the appended signature.
Similarly, by signing a digest containing other metadata and fs-
verity's file digest, the measurement list should include both digests.
Otherwise the consumer of the measurement list would first need to
calculate the signed digest before verifying the signature.
Options:
- Include just fs-verity's file digest and the signature in the
measurement list. Leave it to the consumer of the measurement list to
deal with.
- Define a new template format to include both digests, add a new field
in the iint for the signed digest. (Much more work.)
- As originally posted, directly sign fs-verity's file digest.
thanks,
Mimi
On Thu, Dec 02, 2021 at 11:25:05AM -0500, Mimi Zohar wrote:
> Hi Eric,
>
> On Tue, 2021-11-30 at 13:14 -0500, Mimi Zohar wrote:
> > On Mon, 2021-11-29 at 18:33 -0800, Eric Biggers wrote:
> > > On Mon, Nov 29, 2021 at 12:00:55PM -0500, Mimi Zohar wrote:
> > > > To differentiate between a regular file hash and an fs-verity file digest
> > > > based signature stored as security.ima xattr, define a new signature type
> > > > named IMA_VERITY_DIGSIG.
> > > >
> > > > Signed-off-by: Mimi Zohar <[email protected]>
> > >
> > > For this new signature type, what bytes are actually signed? It looks like it's
> > > just the raw digest, which isn't sufficient since it is ambiguous. It needs to
> > > include information that makes it clear what the signer is actually signing,
> > > such as "this is an fs-verity SHA-256 file digest". See
> > > 'struct fsverity_formatted_digest' for an example of this (but it isn't
> > > necessary to use that exact structure).
> > >
> > > I think the existing IMA signatures have the same problem (but it is hard for me
> > > to understand the code). However, a new signature type doesn't have
> > > backwards-compatibility concerns, so it could be done right.
> >
> > As this change should probably be applicable to all signature types,
> > the signature version in the signature_v2_hdr should be bumped. The
> > existing signature version could co-exist with the new signature
> > version.
>
> By signing the file hash, the sig field in the IMA measurement list can
> be directly verified against the digest field. For appended
> signatures, we defined a new template named ima-modsig which contains
> two file hashes, with and without the appended signature.
>
> Similarly, by signing a digest containing other metadata and fs-
> verity's file digest, the measurement list should include both digests.
> Otherwise the consumer of the measurement list would first need to
> calculate the signed digest before verifying the signature.
>
> Options:
> - Include just fs-verity's file digest and the signature in the
> measurement list. Leave it to the consumer of the measurement list to
> deal with.
> - Define a new template format to include both digests, add a new field
> in the iint for the signed digest. (Much more work.)
> - As originally posted, directly sign fs-verity's file digest.
I don't really have enough knowledge about IMA and how it is used to decide on
one approach or the other. Note that earlier I mentioned that it would be
possible to have an fs-verity setting that makes a full file digest be included
in the fsverity_descriptor, so it gets covered by the fs-verity file digest and
is also retrievable in constant time like the fs-verity file digest is.
If you'd like to solve this problem at the IMA layer instead, by storing the
full file digest in an xattr and signing both the full file digest and fs-verity
file digest together, that would achieve the same goal of making the full file
digest available, and wouldn't require any changes to fs-verity. This would
assume that the file would be signed, though. What about audit-only mode
without signatures; is that something you care about?
Alternatively, maybe this problem doesn't need to be solved at all and IMA would
be fine with the fs-verity file digest only, and not need the full file hash
too. I don't know the answer to that; I think it's up to you to decide.
- Eric
On Thu, 2021-12-02 at 13:17 -0800, Eric Biggers wrote:
> On Thu, Dec 02, 2021 at 11:25:05AM -0500, Mimi Zohar wrote:
> > Hi Eric,
> >
> > On Tue, 2021-11-30 at 13:14 -0500, Mimi Zohar wrote:
> > > On Mon, 2021-11-29 at 18:33 -0800, Eric Biggers wrote:
> > > > On Mon, Nov 29, 2021 at 12:00:55PM -0500, Mimi Zohar wrote:
> > > > > To differentiate between a regular file hash and an fs-verity file digest
> > > > > based signature stored as security.ima xattr, define a new signature type
> > > > > named IMA_VERITY_DIGSIG.
> > > > >
> > > > > Signed-off-by: Mimi Zohar <[email protected]>
> > > >
> > > > For this new signature type, what bytes are actually signed? It looks like it's
> > > > just the raw digest, which isn't sufficient since it is ambiguous. It needs to
> > > > include information that makes it clear what the signer is actually signing,
> > > > such as "this is an fs-verity SHA-256 file digest". See
> > > > 'struct fsverity_formatted_digest' for an example of this (but it isn't
> > > > necessary to use that exact structure).
> > > >
> > > > I think the existing IMA signatures have the same problem (but it is hard for me
> > > > to understand the code). However, a new signature type doesn't have
> > > > backwards-compatibility concerns, so it could be done right.
> > >
> > > As this change should probably be applicable to all signature types,
> > > the signature version in the signature_v2_hdr should be bumped. The
> > > existing signature version could co-exist with the new signature
> > > version.
> >
> > By signing the file hash, the sig field in the IMA measurement list can
> > be directly verified against the digest field. For appended
> > signatures, we defined a new template named ima-modsig which contains
> > two file hashes, with and without the appended signature.
> >
> > Similarly, by signing a digest containing other metadata and fs-
> > verity's file digest, the measurement list should include both digests.
> > Otherwise the consumer of the measurement list would first need to
> > calculate the signed digest before verifying the signature.
> >
> > Options:
> > - Include just fs-verity's file digest and the signature in the
> > measurement list. Leave it to the consumer of the measurement list to
> > deal with.
> > - Define a new template format to include both digests, add a new field
> > in the iint for the signed digest. (Much more work.)
> > - As originally posted, directly sign fs-verity's file digest.
>
> I don't really have enough knowledge about IMA and how it is used to decide on
> one approach or the other. Note that earlier I mentioned that it would be
> possible to have an fs-verity setting that makes a full file digest be included
> in the fsverity_descriptor, so it gets covered by the fs-verity file digest and
> is also retrievable in constant time like the fs-verity file digest is.
>
> If you'd like to solve this problem at the IMA layer instead, by storing the
> full file digest in an xattr and signing both the full file digest and fs-verity
> file digest together, that would achieve the same goal of making the full file
> digest available, and wouldn't require any changes to fs-verity. This would
> assume that the file would be signed, though. What about audit-only mode
> without signatures; is that something you care about?
>
> Alternatively, maybe this problem doesn't need to be solved at all and IMA would
> be fine with the fs-verity file digest only, and not need the full file hash
> too. I don't know the answer to that; I think it's up to you to decide.
I just posted v1 which implements option 1, including the fsverity file
digest in
the measurement list. Both option 2 or including the actual file hash,
will require a new template format with two digests.
Mimi