| Message ID | 1570497267-13672-8-git-send-email-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: Enabling IMA arch specific secure boot policies | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch next (6edfc6487b474fe01857dc3f1a9cd701bb9b21c8) |
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch merge (b05e997bf5d33f38e3fc6a66d52303eb109598ec) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 8 checks, 132 lines checked |
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote: > Asymmetric private keys are used to sign multiple files. The kernel > currently support checking against the blacklisted keys. However, if the > public key is blacklisted, any file signed by the blacklisted key will > automatically fail signature verification. We might not want to blacklist > all the files signed by a particular key, but just a single file. > Blacklisting the public key is not fine enough granularity. > > This patch adds support for blacklisting binaries with appended signatures, > based on the IMA policy. Defined is a new policy option > "appraise_flag=check_blacklist". The blacklisted hash is not the same as the file hash, but is the file hash without the appended signature. Are there tools for calculating the blacklisted hash? Can you provide an example? > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > --- > Documentation/ABI/testing/ima_policy | 1 + > security/integrity/ima/ima.h | 9 +++++++ > security/integrity/ima/ima_appraise.c | 39 +++++++++++++++++++++++++++ > security/integrity/ima/ima_main.c | 12 ++++++--- > security/integrity/ima/ima_policy.c | 10 +++++-- > security/integrity/integrity.h | 1 + > 6 files changed, 66 insertions(+), 6 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index 29ebe9afdac4..4c97afcc0f3c 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -25,6 +25,7 @@ Description: > lsm: [[subj_user=] [subj_role=] [subj_type=] > [obj_user=] [obj_role=] [obj_type=]] > option: [[appraise_type=]] [template=] [permit_directio] > + [appraise_flag=[check_blacklist]] > base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] > [FIRMWARE_CHECK] > [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index ed86c1f70d7f..63e20ccc91ce 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v); > #define IMA_APPRAISE_KEXEC 0x40 > > #ifdef CONFIG_IMA_APPRAISE > +int ima_check_blacklist(struct integrity_iint_cache *iint, > + const struct modsig *modsig, int action, int pcr); > int ima_appraise_measurement(enum ima_hooks func, > struct integrity_iint_cache *iint, > struct file *file, const unsigned char *filename, > @@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry, > struct evm_ima_xattr_data **xattr_value); > > #else > +static inline int ima_check_blacklist(struct integrity_iint_cache *iint, > + const struct modsig *modsig, int action, > + int pcr) > +{ > + return 0; > +} > + > static inline int ima_appraise_measurement(enum ima_hooks func, > struct integrity_iint_cache *iint, > struct file *file, > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 136ae4e0ee92..fe34d64a684c 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -12,6 +12,7 @@ > #include <linux/magic.h> > #include <linux/ima.h> > #include <linux/evm.h> > +#include <keys/system_keyring.h> > > #include "ima.h" > > @@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig, > return rc; > } > > +/* > + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If > + * yes, then adds the hash of the blacklisted binary to the measurement list. > + * > + * Returns -EPERM if the hash is blacklisted. > + */ > +int ima_check_blacklist(struct integrity_iint_cache *iint, > + const struct modsig *modsig, int action, int pcr) > +{ > + enum hash_algo hash_algo; > + const u8 *digest = NULL; > + u32 digestsize = 0; > + u32 secid; > + int rc = 0; > + struct ima_template_desc *template_desc; > + > + template_desc = lookup_template_desc("ima-buf"); > + template_desc_init_fields(template_desc->fmt, &(template_desc->fields), > + &(template_desc->num_fields)); Before using template_desc, make sure that template_desc isn't NULL. For completeness, check the return code of template_desc_init_fields() > + > + if (!(iint->flags & IMA_CHECK_BLACKLIST)) > + return 0; Move this check before getting the template_desc and make sure that modsig isn't NULL. > + > + if (iint->flags & IMA_MODSIG_ALLOWED) { > + security_task_getsecid(current, &secid); secid isn't being used. > + ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize); > + rc = is_binary_blacklisted(digest, digestsize); > + > + /* Returns -EPERM on blacklisted hash found */ Now that it is returning a sane errno, the comment isn't needed. Mimi > + if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) > + process_buffer_measurement(digest, digestsize, > + "blacklisted-hash", pcr, > + template_desc); > + } > + > + return rc; > +} > + > /* > * ima_appraise_measurement - appraise file measurement > * > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 77115e884496..40d30ab17cbe 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -335,10 +335,14 @@ static int process_measurement(struct file *file, const struct cred *cred, > xattr_value, xattr_len, modsig, pcr, > template_desc); > if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) { > - inode_lock(inode); > - rc = ima_appraise_measurement(func, iint, file, pathname, > - xattr_value, xattr_len, modsig); > - inode_unlock(inode); > + rc = ima_check_blacklist(iint, modsig, action, pcr); > + if (rc != -EPERM) { > + inode_lock(inode); > + rc = ima_appraise_measurement(func, iint, file, > + pathname, xattr_value, > + xattr_len, modsig); > + inode_unlock(inode); > + } > if (!rc) > rc = mmap_violation_check(func, file, &pathbuf, > &pathname, filename); > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 5380aca2b351..bfaae7a8443a 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -765,8 +765,8 @@ enum { > Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq, > Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, > Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, > - Opt_appraise_type, Opt_permit_directio, > - Opt_pcr, Opt_template, Opt_err > + Opt_appraise_type, Opt_appraise_flag, > + Opt_permit_directio, Opt_pcr, Opt_template, Opt_err > }; > > static const match_table_t policy_tokens = { > @@ -798,6 +798,7 @@ static const match_table_t policy_tokens = { > {Opt_euid_lt, "euid<%s"}, > {Opt_fowner_lt, "fowner<%s"}, > {Opt_appraise_type, "appraise_type=%s"}, > + {Opt_appraise_flag, "appraise_flag=%s"}, > {Opt_permit_directio, "permit_directio"}, > {Opt_pcr, "pcr=%s"}, > {Opt_template, "template=%s"}, > @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > else > result = -EINVAL; > break; > + case Opt_appraise_flag: > + ima_log_string(ab, "appraise_flag", args[0].from); > + if (strstr(args[0].from, "blacklist")) > + entry->flags |= IMA_CHECK_BLACKLIST; > + break; > case Opt_permit_directio: > entry->flags |= IMA_PERMIT_DIRECTIO; > break; > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index d9323d31a3a8..73fc286834d7 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -32,6 +32,7 @@ > #define EVM_IMMUTABLE_DIGSIG 0x08000000 > #define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000 > #define IMA_MODSIG_ALLOWED 0x20000000 > +#define IMA_CHECK_BLACKLIST 0x40000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ > IMA_HASH | IMA_APPRAISE_SUBMASK)
Hi Mimi, On 10/11/2019 09:19 AM, Mimi Zohar wrote: > On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote: >> Asymmetric private keys are used to sign multiple files. The kernel >> currently support checking against the blacklisted keys. However, if the >> public key is blacklisted, any file signed by the blacklisted key will >> automatically fail signature verification. We might not want to blacklist >> all the files signed by a particular key, but just a single file. >> Blacklisting the public key is not fine enough granularity. >> >> This patch adds support for blacklisting binaries with appended signatures, >> based on the IMA policy. Defined is a new policy option >> "appraise_flag=check_blacklist". > The blacklisted hash is not the same as the file hash, but is the file > hash without the appended signature. Are there tools for calculating > the blacklisted hash? Can you provide an example? I have updated the patch description to specify that the blacklisted hash is the file hash without the appended signature. I hope that makes it clear now. Thanks & Regards, - Nayna
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 29ebe9afdac4..4c97afcc0f3c 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -25,6 +25,7 @@ Description: lsm: [[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] option: [[appraise_type=]] [template=] [permit_directio] + [appraise_flag=[check_blacklist]] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index ed86c1f70d7f..63e20ccc91ce 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v); #define IMA_APPRAISE_KEXEC 0x40 #ifdef CONFIG_IMA_APPRAISE +int ima_check_blacklist(struct integrity_iint_cache *iint, + const struct modsig *modsig, int action, int pcr); int ima_appraise_measurement(enum ima_hooks func, struct integrity_iint_cache *iint, struct file *file, const unsigned char *filename, @@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry, struct evm_ima_xattr_data **xattr_value); #else +static inline int ima_check_blacklist(struct integrity_iint_cache *iint, + const struct modsig *modsig, int action, + int pcr) +{ + return 0; +} + static inline int ima_appraise_measurement(enum ima_hooks func, struct integrity_iint_cache *iint, struct file *file, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 136ae4e0ee92..fe34d64a684c 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -12,6 +12,7 @@ #include <linux/magic.h> #include <linux/ima.h> #include <linux/evm.h> +#include <keys/system_keyring.h> #include "ima.h" @@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig, return rc; } +/* + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If + * yes, then adds the hash of the blacklisted binary to the measurement list. + * + * Returns -EPERM if the hash is blacklisted. + */ +int ima_check_blacklist(struct integrity_iint_cache *iint, + const struct modsig *modsig, int action, int pcr) +{ + enum hash_algo hash_algo; + const u8 *digest = NULL; + u32 digestsize = 0; + u32 secid; + int rc = 0; + struct ima_template_desc *template_desc; + + template_desc = lookup_template_desc("ima-buf"); + template_desc_init_fields(template_desc->fmt, &(template_desc->fields), + &(template_desc->num_fields)); + + if (!(iint->flags & IMA_CHECK_BLACKLIST)) + return 0; + + if (iint->flags & IMA_MODSIG_ALLOWED) { + security_task_getsecid(current, &secid); + ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize); + rc = is_binary_blacklisted(digest, digestsize); + + /* Returns -EPERM on blacklisted hash found */ + if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) + process_buffer_measurement(digest, digestsize, + "blacklisted-hash", pcr, + template_desc); + } + + return rc; +} + /* * ima_appraise_measurement - appraise file measurement * diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 77115e884496..40d30ab17cbe 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -335,10 +335,14 @@ static int process_measurement(struct file *file, const struct cred *cred, xattr_value, xattr_len, modsig, pcr, template_desc); if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) { - inode_lock(inode); - rc = ima_appraise_measurement(func, iint, file, pathname, - xattr_value, xattr_len, modsig); - inode_unlock(inode); + rc = ima_check_blacklist(iint, modsig, action, pcr); + if (rc != -EPERM) { + inode_lock(inode); + rc = ima_appraise_measurement(func, iint, file, + pathname, xattr_value, + xattr_len, modsig); + inode_unlock(inode); + } if (!rc) rc = mmap_violation_check(func, file, &pathbuf, &pathname, filename); diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 5380aca2b351..bfaae7a8443a 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -765,8 +765,8 @@ enum { Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq, Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, - Opt_appraise_type, Opt_permit_directio, - Opt_pcr, Opt_template, Opt_err + Opt_appraise_type, Opt_appraise_flag, + Opt_permit_directio, Opt_pcr, Opt_template, Opt_err }; static const match_table_t policy_tokens = { @@ -798,6 +798,7 @@ static const match_table_t policy_tokens = { {Opt_euid_lt, "euid<%s"}, {Opt_fowner_lt, "fowner<%s"}, {Opt_appraise_type, "appraise_type=%s"}, + {Opt_appraise_flag, "appraise_flag=%s"}, {Opt_permit_directio, "permit_directio"}, {Opt_pcr, "pcr=%s"}, {Opt_template, "template=%s"}, @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else result = -EINVAL; break; + case Opt_appraise_flag: + ima_log_string(ab, "appraise_flag", args[0].from); + if (strstr(args[0].from, "blacklist")) + entry->flags |= IMA_CHECK_BLACKLIST; + break; case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; break; diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index d9323d31a3a8..73fc286834d7 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -32,6 +32,7 @@ #define EVM_IMMUTABLE_DIGSIG 0x08000000 #define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000 #define IMA_MODSIG_ALLOWED 0x20000000 +#define IMA_CHECK_BLACKLIST 0x40000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_HASH | IMA_APPRAISE_SUBMASK)
Asymmetric private keys are used to sign multiple files. The kernel currently support checking against the blacklisted keys. However, if the public key is blacklisted, any file signed by the blacklisted key will automatically fail signature verification. We might not want to blacklist all the files signed by a particular key, but just a single file. Blacklisting the public key is not fine enough granularity. This patch adds support for blacklisting binaries with appended signatures, based on the IMA policy. Defined is a new policy option "appraise_flag=check_blacklist". Signed-off-by: Nayna Jain <nayna@linux.ibm.com> --- Documentation/ABI/testing/ima_policy | 1 + security/integrity/ima/ima.h | 9 +++++++ security/integrity/ima/ima_appraise.c | 39 +++++++++++++++++++++++++++ security/integrity/ima/ima_main.c | 12 ++++++--- security/integrity/ima/ima_policy.c | 10 +++++-- security/integrity/integrity.h | 1 + 6 files changed, 66 insertions(+), 6 deletions(-)