Support for including fs-verity file digests and signatures in the IMA
measurement list as well as verifying the fs-verity file digest based
signatures, both based on IMA policy rules, was discussed prior to
fs-verity being upstreamed[1,2].
Support for including fs-verity file digests in the 'd-ng' template
field is based on a new policy rule option named 'digest_type=verity'.
A new template field named 'd-type' as well as a new template named
'ima-ngv2' are defined to differentiate between the regular IMA file
hashes from the fs-verity file digests (tree-hash based file hashes)
stored in the 'd-ng' template field of the measurement list.
A new signature version (v3) is defined as a hash of the 'ima_file_id'
struct, to disambiguate the signatures stored as 'security.ima' xattr.
The policy rule 'appraise_type=' option is extended to support 'sigv3',
which is initially limited to fs-verity.
The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
option and require the 'appraise_type=sigv3' option.
Lastly, for IMA to differentiate between the original IMA signature
from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
is defined.
[1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
[2] Documentation/filesystems/fsverity.rst
Changelog v5:
- Define ima_max_digest_size struct, removing the locally defined versions.
- Don't overload the 'digest_type=verity' to imply a verity signature,
but extend the 'appraise_type' policy rule option to define 'sigv3'.
Changelog v4:
- Based on Eric Bigger's signature verification concerns of replacing the
contents of a file with the ima_file_id struct hash, require per policy
rule signature versions.
- Addressed Eric Bigger's other comments.
- Added new audit messages "causes".
- Updated patch descriptions.
Changelog v3:
- Addressed Eric Bigger's comments: included Ack, incremented the
signature format version, the crypto issues are generic and will be
addressed by him separately.
- Addressed Vitaly Chikunov's comments: hard coded maximum digest size
rather than using a flexible array, removed unnecessary assignment, and
fixed comment to match variable name.
- Defined new "ima_max_digest_size" struct to avoid wrapping the
"ima_digest_data" struct inside a function local structure or
having to dynamically allocate it with enough memory for the specific
hash algo size.
Changelog v2:
- Addressed Eric Bigger's comments: sign the hash of fsverity's digest
and the digest's metadata, use match_string, use preferred function
name fsverity_get_digest(), support including unsigned fs-verity's
digests in the IMA measurement list.
- Remove signatures requirement for including fs-verity's file digests in
the 'd-ng' field of the measurement list.
Changelog v1:
- Updated both fsverity and IMA documentation.
- Addressed both Eric Bigger's and Lakshmi's comments.
Mimi Zohar (8):
ima: rename IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS
ima: define ima_max_digest_data struct without a flexible array
variable
fs-verity: define a function to return the integrity protected file
digest
ima: define a new template field 'd-type' and a new template
'ima-ngv2'
ima: permit fsverity's file digests in the IMA measurement list
ima: define signature version 3
ima: support fs-verity file digest based version 3 signatures
fsverity: update the documentation
Documentation/ABI/testing/ima_policy | 30 +++++-
Documentation/filesystems/fsverity.rst | 22 +++--
Documentation/security/IMA-templates.rst | 11 ++-
fs/verity/Kconfig | 1 +
fs/verity/fsverity_private.h | 7 --
fs/verity/measure.c | 41 ++++++++
include/linux/fsverity.h | 18 ++++
security/integrity/digsig.c | 3 +-
security/integrity/ima/ima_api.c | 49 ++++++++--
security/integrity/ima/ima_appraise.c | 112 +++++++++++++++++++++-
security/integrity/ima/ima_init.c | 5 +-
security/integrity/ima/ima_main.c | 7 +-
security/integrity/ima/ima_policy.c | 66 +++++++++++--
security/integrity/ima/ima_template.c | 3 +
security/integrity/ima/ima_template_lib.c | 28 +++++-
security/integrity/ima/ima_template_lib.h | 2 +
security/integrity/integrity.h | 39 +++++++-
17 files changed, 388 insertions(+), 56 deletions(-)
--
2.27.0
To support larger hash digests in the 'iint' cache, instead of defining
the 'digest' field as the maximum digest size, the 'digest' field was
defined as a flexible array variable. The "ima_digest_data" struct was
wrapped inside a local structure with the maximum digest size. But
before adding the record to the iint cache, memory for the exact digest
size was dynamically allocated.
The original reason for defining the 'digest' field as a flexible array
variable is still valid for the 'iint' cache use case. Instead of
wrapping the 'ima_digest_data' struct in a local structure define
'ima_max_digest_data' struct.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/ima_api.c | 10 ++++------
security/integrity/ima/ima_init.c | 5 +----
security/integrity/ima/ima_main.c | 5 +----
security/integrity/ima/ima_template_lib.c | 5 +----
security/integrity/integrity.h | 10 ++++++++++
5 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 5b220a2fe573..c6805af46211 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
const char *filename = file->f_path.dentry->d_name.name;
+ struct ima_max_digest_data hash;
int result = 0;
int length;
void *tmpbuf;
u64 i_version;
- struct {
- struct ima_digest_data hdr;
- char digest[IMA_MAX_DIGEST_SIZE];
- } hash;
/*
* Always collect the modsig, because IMA might have already collected
@@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
/*
* Detecting file change is based on i_version. On filesystems
- * which do not support i_version, support is limited to an initial
- * measurement/appraisal/audit.
+ * which do not support i_version, support was originally limited
+ * to an initial measurement/appraisal/audit, but was modified to
+ * assume the file changed.
*/
i_version = inode_query_iversion(inode);
hash.hdr.algo = algo;
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index b26fa67476b4..63979aefc95f 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
struct ima_event_data event_data = { .iint = iint,
.filename = boot_aggregate_name };
+ struct ima_max_digest_data hash;
int result = -ENOMEM;
int violation = 0;
- struct {
- struct ima_digest_data hdr;
- char digest[TPM_MAX_DIGEST_SIZE];
- } hash;
memset(iint, 0, sizeof(*iint));
memset(&hash, 0, sizeof(hash));
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7c80dfe2c7a5..c6412dec3810 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
.buf = buf,
.buf_len = size};
struct ima_template_desc *template;
- struct {
- struct ima_digest_data hdr;
- char digest[IMA_MAX_DIGEST_SIZE];
- } hash = {};
+ struct ima_max_digest_data hash;
char digest_hash[IMA_MAX_DIGEST_SIZE];
int digest_hash_len = hash_digest_size[ima_hash_algo];
int violation = 0;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 5a5d462ab36d..7155d17a3b75 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
int ima_eventdigest_init(struct ima_event_data *event_data,
struct ima_field_data *field_data)
{
- struct {
- struct ima_digest_data hdr;
- char digest[IMA_MAX_DIGEST_SIZE];
- } hash;
+ struct ima_max_digest_data hash;
u8 *cur_digest = NULL;
u32 cur_digestsize = 0;
struct inode *inode;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d045dccd415a..daf49894fd7d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
#include <linux/types.h>
#include <linux/integrity.h>
#include <crypto/sha1.h>
+#include <crypto/hash.h>
#include <linux/key.h>
#include <linux/audit.h>
@@ -110,6 +111,15 @@ struct ima_digest_data {
u8 digest[];
} __packed;
+/*
+ * Instead of wrapping the ima_digest_data struct inside a local structure
+ * with the maximum hash size, define ima_max_digest_data struct.
+ */
+struct ima_max_digest_data {
+ struct ima_digest_data hdr;
+ u8 digest[HASH_MAX_DIGESTSIZE];
+} __packed;
+
/*
* signature format v2 - for using with asymmetric keys
*/
--
2.27.0
On 2/11/22 16:43, Mimi Zohar wrote:
> To support larger hash digests in the 'iint' cache, instead of defining
> the 'digest' field as the maximum digest size, the 'digest' field was
> defined as a flexible array variable. The "ima_digest_data" struct was
> wrapped inside a local structure with the maximum digest size. But
> before adding the record to the iint cache, memory for the exact digest
> size was dynamically allocated.
>
> The original reason for defining the 'digest' field as a flexible array
> variable is still valid for the 'iint' cache use case. Instead of
> wrapping the 'ima_digest_data' struct in a local structure define
> 'ima_max_digest_data' struct.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> security/integrity/ima/ima_api.c | 10 ++++------
> security/integrity/ima/ima_init.c | 5 +----
> security/integrity/ima/ima_main.c | 5 +----
> security/integrity/ima/ima_template_lib.c | 5 +----
> security/integrity/integrity.h | 10 ++++++++++
> 5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 5b220a2fe573..c6805af46211 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> const char *audit_cause = "failed";
> struct inode *inode = file_inode(file);
> const char *filename = file->f_path.dentry->d_name.name;
> + struct ima_max_digest_data hash;
> int result = 0;
> int length;
> void *tmpbuf;
> u64 i_version;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash;
>
> /*
> * Always collect the modsig, because IMA might have already collected
> @@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>
> /*
> * Detecting file change is based on i_version. On filesystems
> - * which do not support i_version, support is limited to an initial
> - * measurement/appraisal/audit.
> + * which do not support i_version, support was originally limited
> + * to an initial measurement/appraisal/audit, but was modified to
> + * assume the file changed.
> */
> i_version = inode_query_iversion(inode);
> hash.hdr.algo = algo;
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index b26fa67476b4..63979aefc95f 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
> struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> struct ima_event_data event_data = { .iint = iint,
> .filename = boot_aggregate_name };
> + struct ima_max_digest_data hash;
> int result = -ENOMEM;
> int violation = 0;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[TPM_MAX_DIGEST_SIZE];
> - } hash;
>
> memset(iint, 0, sizeof(*iint));
> memset(&hash, 0, sizeof(hash));
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 7c80dfe2c7a5..c6412dec3810 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
> .buf = buf,
> .buf_len = size};
> struct ima_template_desc *template;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash = {};
> + struct ima_max_digest_data hash;
It looks like the initialization wasn't necessary.
Reviewed-by: Stefan Berger <[email protected]>
> char digest_hash[IMA_MAX_DIGEST_SIZE];
> int digest_hash_len = hash_digest_size[ima_hash_algo];
> int violation = 0;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 5a5d462ab36d..7155d17a3b75 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
> int ima_eventdigest_init(struct ima_event_data *event_data,
> struct ima_field_data *field_data)
> {
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash;
> + struct ima_max_digest_data hash;
> u8 *cur_digest = NULL;
> u32 cur_digestsize = 0;
> struct inode *inode;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d045dccd415a..daf49894fd7d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
> #include <linux/types.h>
> #include <linux/integrity.h>
> #include <crypto/sha1.h>
> +#include <crypto/hash.h>
> #include <linux/key.h>
> #include <linux/audit.h>
>
> @@ -110,6 +111,15 @@ struct ima_digest_data {
> u8 digest[];
> } __packed;
>
> +/*
> + * Instead of wrapping the ima_digest_data struct inside a local structure
> + * with the maximum hash size, define ima_max_digest_data struct.
> + */
> +struct ima_max_digest_data {
> + struct ima_digest_data hdr;
> + u8 digest[HASH_MAX_DIGESTSIZE];
> +} __packed;
> +
> /*
> * signature format v2 - for using with asymmetric keys
> */