| Message ID | 20241127210234.121546-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: instantiate the bprm_creds_for_exec() hook | expand |
Hi Mimi, kernel test robot noticed the following build errors: [auto build test ERROR on zohar-integrity/next-integrity] [also build test ERROR on linus/master v6.12 next-20241128] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mimi-Zohar/ima-instantiate-the-bprm_creds_for_exec-hook/20241128-120656 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity patch link: https://lore.kernel.org/r/20241127210234.121546-1-zohar%40linux.ibm.com patch subject: [PATCH] ima: instantiate the bprm_creds_for_exec() hook config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20241128/202411282320.KuWvLpDS-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241128/202411282320.KuWvLpDS-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411282320.KuWvLpDS-lkp@intel.com/ All errors (new ones prefixed by >>): security/integrity/ima/ima_main.c: In function 'ima_bprm_creds_for_exec': >> security/integrity/ima/ima_main.c:572:18: error: 'struct linux_binprm' has no member named 'is_check' 572 | if (!bprm->is_check) | ^~ -- security/integrity/ima/ima_appraise.c: In function 'is_bprm_creds_for_exec': >> security/integrity/ima/ima_appraise.c:492:25: error: 'struct linux_binprm' has no member named 'is_check' 492 | if (bprm->is_check) | ^~ vim +572 security/integrity/ima/ima_main.c 556 557 /** 558 * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. 559 * @bprm: contains the linux_binprm structure 560 * 561 * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and 562 * appraise the integrity of a file to be executed by script interpreters. 563 * Unlike any of the other LSM hooks where the kernel enforces file integrity, 564 * enforcing file integrity is left up to the discretion of the script 565 * interpreter (userspace). 566 * 567 * On success return 0. On integrity appraisal error, assuming the file 568 * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. 569 */ 570 static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) 571 { > 572 if (!bprm->is_check) 573 return 0; 574 575 return ima_bprm_check(bprm); 576 } 577
Hi Mimi,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on linus/master v6.12 next-20241128]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mimi-Zohar/ima-instantiate-the-bprm_creds_for_exec-hook/20241128-120656
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20241127210234.121546-1-zohar%40linux.ibm.com
patch subject: [PATCH] ima: instantiate the bprm_creds_for_exec() hook
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20241129/202411290413.VUC6seTw-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411290413.VUC6seTw-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411290413.VUC6seTw-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from security/integrity/ima/ima_main.c:23:
In file included from include/linux/mman.h:5:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from security/integrity/ima/ima_main.c:26:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from security/integrity/ima/ima_main.c:26:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from security/integrity/ima/ima_main.c:26:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> security/integrity/ima/ima_main.c:572:13: error: no member named 'is_check' in 'struct linux_binprm'
572 | if (!bprm->is_check)
| ~~~~ ^
7 warnings and 1 error generated.
--
In file included from security/integrity/ima/ima_appraise.c:13:
In file included from include/linux/xattr.h:18:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from security/integrity/ima/ima_appraise.c:15:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from security/integrity/ima/ima_appraise.c:15:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from security/integrity/ima/ima_appraise.c:15:
In file included from include/linux/ima.h:12:
In file included from include/linux/security.h:35:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> security/integrity/ima/ima_appraise.c:492:13: error: no member named 'is_check' in 'struct linux_binprm'
492 | if (bprm->is_check)
| ~~~~ ^
7 warnings and 1 error generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +572 security/integrity/ima/ima_main.c
556
557 /**
558 * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement.
559 * @bprm: contains the linux_binprm structure
560 *
561 * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and
562 * appraise the integrity of a file to be executed by script interpreters.
563 * Unlike any of the other LSM hooks where the kernel enforces file integrity,
564 * enforcing file integrity is left up to the discretion of the script
565 * interpreter (userspace).
566 *
567 * On success return 0. On integrity appraisal error, assuming the file
568 * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
569 */
570 static int ima_bprm_creds_for_exec(struct linux_binprm *bprm)
571 {
> 572 if (!bprm->is_check)
573 return 0;
574
575 return ima_bprm_check(bprm);
576 }
577
For reference, here is the base patch series: https://lore.kernel.org/all/20241112191858.162021-1-mic@digikod.net/ CCing audit@ On Wed, Nov 27, 2024 at 04:02:34PM -0500, Mimi Zohar wrote: > Like direct file execution (e.g. ./script.sh), indirect file execution > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > the new security_bprm_creds_for_exec() hook to measure and verify the > indirect file's integrity. Unlike direct file execution, indirect file > execution integrity is optionally enforced by the interpreter. > > Update the audit messages to differentiate between kernel and userspace > enforced integrity. I'm not sure to see the full picture. What is the difference between execveat() calls and execveat() + AT_EXECVE_CHECK calls? Both are from user space, the only difference is that the first can lead to a full execution, but the intent is the same. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/ima/ima_appraise.c | 84 ++++++++++++++++++++------- > security/integrity/ima/ima_main.c | 22 +++++++ > 2 files changed, 86 insertions(+), 20 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 656c709b974f..b5f8e49cde9d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -8,6 +8,7 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/file.h> > +#include <linux/binfmts.h> > #include <linux/fs.h> > #include <linux/xattr.h> > #include <linux/magic.h> > @@ -16,6 +17,7 @@ > #include <linux/fsverity.h> > #include <keys/system_keyring.h> > #include <uapi/linux/fsverity.h> > +#include <linux/securebits.h> > > #include "ima.h" > > @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, > */ > static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > - enum integrity_status *status, const char **cause) > + enum integrity_status *status, const char **cause, > + bool is_check) > { > struct ima_max_digest_data hash; > struct signature_v2_hdr *sig; > @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > if (*status != INTEGRITY_PASS_IMMUTABLE) { > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (iint->flags & IMA_VERITY_REQUIRED) > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; This looks simpler (same for all following checks): is_check ? "verity-signature-required(userspace)" : "verity-signature-required"; > else > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > else > rc = -EINVAL; > if (rc) { > - *cause = "invalid-hash"; > + *cause = !is_check ? "invalid-hash" : > + "invalid-hash(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; > if ((iint->flags & mask) == mask) { > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > > sig = (typeof(sig))xattr_value; > if (sig->version >= 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > iint->ima_hash->digest, > iint->ima_hash->length); > if (rc) { > - *cause = "invalid-signature"; > + *cause = !is_check ? "invalid-signature" : > + "invalid-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (!(iint->flags & IMA_VERITY_REQUIRED)) { > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > sig = (typeof(sig))xattr_value; > if (sig->version != 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > container_of(&hash.hdr, > struct ima_digest_data, hdr)); > if (rc) { > - *cause = "sigv3-hashing-error"; > + *cause = !is_check ? "sigv3-hashing-error" : > + "sigv3-hashing-error(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > xattr_len, hash.digest, > hash.hdr.length); > if (rc) { > - *cause = "invalid-verity-signature"; > + *cause = !is_check ? "invalid-verity-signature" : > + "invalid-verify-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > break; > default: > *status = INTEGRITY_UNKNOWN; > - *cause = "unknown-ima-data"; > + *cause = !is_check ? "unknown-ima-data" : > + "unknown-ima-data(userspace)"; > break; > } > > @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, > return rc; > } > > +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) > +{ > + struct linux_binprm *bprm = NULL; > + > + if (func == BPRM_CHECK) { > + bprm = container_of(&file, struct linux_binprm, file); > + if (bprm->is_check) > + return 1; > + } > + return 0; > +} > + > /* > * ima_appraise_measurement - appraise file measurement > * > @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > enum integrity_status status = INTEGRITY_UNKNOWN; > int rc = xattr_len; > bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; > + bool is_check = false; > > /* If not appraising a modsig, we need an xattr. */ > if (!(inode->i_opflags & IOP_XATTR) && !try_modsig) > return INTEGRITY_UNKNOWN; > > + /* > + * Unlike any of the other LSM hooks where the kernel enforces file > + * integrity, enforcing file integrity for the bprm_creds_for_exec() > + * LSM hook is left up to the discretion of the script interpreter > + * (userspace). > + * > + * Since the SECBIT_EXEC_RESTRICT_FILE flag is just a hint as to > + * userspace intentions, simply annotate the audit messages indicating > + * a userspace based query. > + */ > + is_check = is_bprm_creds_for_exec(func, file); > + > /* If reading the xattr failed and there's no modsig, error out. */ > if (rc <= 0 && !try_modsig) { > if (rc && rc != -ENODATA) > @@ -501,11 +540,14 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (iint->flags & IMA_VERITY_REQUIRED) > - cause = "verity-signature-required"; > + cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > else > - cause = "IMA-signature-required"; > + cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > } else { > - cause = "missing-hash"; > + cause = !is_check ? "missing-hash" : > + "missing-hash(userspace)"; > } > > status = INTEGRITY_NOLABEL; > @@ -531,14 +573,15 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > break; > fallthrough; > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > - cause = "missing-HMAC"; > + cause = !is_check ? "missing-HMAC" : "missing-HMAC(userspace)"; > goto out; > case INTEGRITY_FAIL_IMMUTABLE: > set_bit(IMA_DIGSIG, &iint->atomic_flags); > - cause = "invalid-fail-immutable"; > + cause = !is_check ? "invalid-fail-immutable" : > + "invalid-fail-immutable(userspace)"; > goto out; > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > - cause = "invalid-HMAC"; > + cause = !is_check ? "invalid-HMAC" : "invalid-HMAC(userspace)"; > goto out; > default: > WARN_ONCE(true, "Unexpected integrity status %d\n", status); > @@ -546,7 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > if (xattr_value) > rc = xattr_verify(func, iint, xattr_value, xattr_len, &status, > - &cause); > + &cause, is_check); > > /* > * If we have a modsig and either no imasig or the imasig's key isn't > @@ -568,7 +611,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) || > (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) { > status = INTEGRITY_FAIL; > - cause = "unverifiable-signature"; > + cause = !is_check ? "unverifiable-signature" : > + "unverifiable-signature(userspace)"; > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, > op, cause, rc, 0); Instead of adding new causes, another option would be to add a new audit record type (e.g. AUDIT_INTEGRITY_DATA_CHECK). This would help filter these new kind of messages and I guess scale better. Another alternative would be to extend the audit message with a new field (e.g. "check=1"), but that would not help for efficient filtering. > } else if (status != INTEGRITY_PASS) { > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 06132cf47016..2b5d6bae77a4 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm) > MAY_EXEC, CREDS_CHECK); > } > > +/** > + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. > + * @bprm: contains the linux_binprm structure > + * > + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and > + * appraise the integrity of a file to be executed by script interpreters. > + * Unlike any of the other LSM hooks where the kernel enforces file integrity, > + * enforcing file integrity is left up to the discretion of the script > + * interpreter (userspace). > + * > + * On success return 0. On integrity appraisal error, assuming the file > + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. > + */ > +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) > +{ > + if (!bprm->is_check) > + return 0; > + > + return ima_bprm_check(bprm); > +} > + > /** > * ima_file_check - based on policy, collect/store measurement. > * @file: pointer to the file to be measured > @@ -1177,6 +1198,7 @@ static int __init init_ima(void) > > static struct security_hook_list ima_hooks[] __ro_after_init = { > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > + LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec), Why not replace bprm_check_security with bprm_creds_for_exec implementation altogether? > LSM_HOOK_INIT(file_post_open, ima_file_check), > LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile), > LSM_HOOK_INIT(file_release, ima_file_free), > -- > 2.47.0 > >
On Fri, 2024-11-29 at 12:06 +0100, Mickaël Salaün wrote: > For reference, here is the base patch series: > https://lore.kernel.org/all/20241112191858.162021-1-mic@digikod.net/ > > CCing audit@ > > On Wed, Nov 27, 2024 at 04:02:34PM -0500, Mimi Zohar wrote: > > Like direct file execution (e.g. ./script.sh), indirect file execution > > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > > the new security_bprm_creds_for_exec() hook to measure and verify the > > indirect file's integrity. Unlike direct file execution, indirect file > > execution integrity is optionally enforced by the interpreter. > > > > Update the audit messages to differentiate between kernel and userspace > > enforced integrity. > > I'm not sure to see the full picture. What is the difference between > execveat() calls and execveat() + AT_EXECVE_CHECK calls? Both are from > user space, the only difference is that the first can lead to a full > execution, but the intent is the same. We do want the full execution in order to measure/appraise/audit both the direct file execution (e.g. ./script.sh) and the interpreter (e.g. #!/usr/bin/bash) specified. > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > security/integrity/ima/ima_appraise.c | 84 ++++++++++++++++++++------- > > security/integrity/ima/ima_main.c | 22 +++++++ > > 2 files changed, 86 insertions(+), 20 deletions(-) > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > index 656c709b974f..b5f8e49cde9d 100644 > > --- a/security/integrity/ima/ima_appraise.c > > +++ b/security/integrity/ima/ima_appraise.c > > @@ -8,6 +8,7 @@ > > #include <linux/module.h> > > #include <linux/init.h> > > #include <linux/file.h> > > +#include <linux/binfmts.h> > > #include <linux/fs.h> > > #include <linux/xattr.h> > > #include <linux/magic.h> > > @@ -16,6 +17,7 @@ > > #include <linux/fsverity.h> > > #include <keys/system_keyring.h> > > #include <uapi/linux/fsverity.h> > > +#include <linux/securebits.h> > > > > #include "ima.h" > > > > @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, > > */ > > static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > - enum integrity_status *status, const char **cause) > > + enum integrity_status *status, const char **cause, > > + bool is_check) > > { > > struct ima_max_digest_data hash; > > struct signature_v2_hdr *sig; > > @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > if (*status != INTEGRITY_PASS_IMMUTABLE) { > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > if (iint->flags & IMA_VERITY_REQUIRED) > > - *cause = "verity-signature-required"; > > + *cause = !is_check ? "verity-signature-required" : > > + "verity-signature-required(userspace)"; > > This looks simpler (same for all following checks): > is_check ? "verity-signature-required(userspace)" : "verity-signature-required"; > > > else > > - *cause = "IMA-signature-required"; > > + *cause = !is_check ? "IMA-signature-required" : > > + "IMA-signature-required(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > else > > rc = -EINVAL; > > if (rc) { > > - *cause = "invalid-hash"; > > + *cause = !is_check ? "invalid-hash" : > > + "invalid-hash(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; > > if ((iint->flags & mask) == mask) { > > - *cause = "verity-signature-required"; > > + *cause = !is_check ? "verity-signature-required" : > > + "verity-signature-required(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > > > sig = (typeof(sig))xattr_value; > > if (sig->version >= 3) { > > - *cause = "invalid-signature-version"; > > + *cause = !is_check ? "invalid-signature-version" : > > + "invalid-signature-version(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > iint->ima_hash->digest, > > iint->ima_hash->length); > > if (rc) { > > - *cause = "invalid-signature"; > > + *cause = !is_check ? "invalid-signature" : > > + "invalid-signature(userspace)"; > > *status = INTEGRITY_FAIL; > > } else { > > *status = INTEGRITY_PASS; > > @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > if (!(iint->flags & IMA_VERITY_REQUIRED)) { > > - *cause = "IMA-signature-required"; > > + *cause = !is_check ? "IMA-signature-required" : > > + "IMA-signature-required(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > sig = (typeof(sig))xattr_value; > > if (sig->version != 3) { > > - *cause = "invalid-signature-version"; > > + *cause = !is_check ? "invalid-signature-version" : > > + "invalid-signature-version(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > container_of(&hash.hdr, > > struct ima_digest_data, hdr)); > > if (rc) { > > - *cause = "sigv3-hashing-error"; > > + *cause = !is_check ? "sigv3-hashing-error" : > > + "sigv3-hashing-error(userspace)"; > > *status = INTEGRITY_FAIL; > > break; > > } > > @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > xattr_len, hash.digest, > > hash.hdr.length); > > if (rc) { > > - *cause = "invalid-verity-signature"; > > + *cause = !is_check ? "invalid-verity-signature" : > > + "invalid-verify-signature(userspace)"; > > *status = INTEGRITY_FAIL; > > } else { > > *status = INTEGRITY_PASS; > > @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > break; > > default: > > *status = INTEGRITY_UNKNOWN; > > - *cause = "unknown-ima-data"; > > + *cause = !is_check ? "unknown-ima-data" : > > + "unknown-ima-data(userspace)"; > > break; > > } > > > > @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, > > return rc; > > } > > > > +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) > > +{ > > + struct linux_binprm *bprm = NULL; > > + > > + if (func == BPRM_CHECK) { > > + bprm = container_of(&file, struct linux_binprm, file); > > + if (bprm->is_check) > > + return 1; > > + } > > + return 0; > > +} > > + > > /* > > * ima_appraise_measurement - appraise file measurement > > * > > @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > enum integrity_status status = INTEGRITY_UNKNOWN; > > int rc = xattr_len; > > bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; > > + bool is_check = false; > > > > /* If not appraising a modsig, we need an xattr. */ > > if (!(inode->i_opflags & IOP_XATTR) && !try_modsig) > > return INTEGRITY_UNKNOWN; > > > > + /* > > + * Unlike any of the other LSM hooks where the kernel enforces file > > + * integrity, enforcing file integrity for the bprm_creds_for_exec() > > + * LSM hook is left up to the discretion of the script interpreter > > + * (userspace). > > + * > > + * Since the SECBIT_EXEC_RESTRICT_FILE flag is just a hint as to > > + * userspace intentions, simply annotate the audit messages indicating > > + * a userspace based query. > > + */ > > + is_check = is_bprm_creds_for_exec(func, file); > > + > > /* If reading the xattr failed and there's no modsig, error out. */ > > if (rc <= 0 && !try_modsig) { > > if (rc && rc != -ENODATA) > > @@ -501,11 +540,14 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > if (iint->flags & IMA_VERITY_REQUIRED) > > - cause = "verity-signature-required"; > > + cause = !is_check ? "verity-signature-required" : > > + "verity-signature-required(userspace)"; > > else > > - cause = "IMA-signature-required"; > > + cause = !is_check ? "IMA-signature-required" : > > + "IMA-signature-required(userspace)"; > > } else { > > - cause = "missing-hash"; > > + cause = !is_check ? "missing-hash" : > > + "missing-hash(userspace)"; > > } > > > > status = INTEGRITY_NOLABEL; > > @@ -531,14 +573,15 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > break; > > fallthrough; > > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > > - cause = "missing-HMAC"; > > + cause = !is_check ? "missing-HMAC" : "missing-HMAC(userspace)"; > > goto out; > > case INTEGRITY_FAIL_IMMUTABLE: > > set_bit(IMA_DIGSIG, &iint->atomic_flags); > > - cause = "invalid-fail-immutable"; > > + cause = !is_check ? "invalid-fail-immutable" : > > + "invalid-fail-immutable(userspace)"; > > goto out; > > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > > - cause = "invalid-HMAC"; > > + cause = !is_check ? "invalid-HMAC" : "invalid-HMAC(userspace)"; > > goto out; > > default: > > WARN_ONCE(true, "Unexpected integrity status %d\n", status); > > @@ -546,7 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > > if (xattr_value) > > rc = xattr_verify(func, iint, xattr_value, xattr_len, &status, > > - &cause); > > + &cause, is_check); > > > > /* > > * If we have a modsig and either no imasig or the imasig's key isn't > > @@ -568,7 +611,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) || > > (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) { > > status = INTEGRITY_FAIL; > > - cause = "unverifiable-signature"; > > + cause = !is_check ? "unverifiable-signature" : > > + "unverifiable-signature(userspace)"; > > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, > > op, cause, rc, 0); > > Instead of adding new causes, another option would be to add a new audit > record type (e.g. AUDIT_INTEGRITY_DATA_CHECK). This would help filter > these new kind of messages and I guess scale better. Thanks. This sounds like a better alternative. > > Another alternative would be to extend the audit message with a new > field (e.g. "check=1"), but that would not help for efficient filtering. > > > } else if (status != INTEGRITY_PASS) { > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > index 06132cf47016..2b5d6bae77a4 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm) > > MAY_EXEC, CREDS_CHECK); > > } > > > > +/** > > + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. > > + * @bprm: contains the linux_binprm structure > > + * > > + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and > > + * appraise the integrity of a file to be executed by script interpreters. > > + * Unlike any of the other LSM hooks where the kernel enforces file integrity, > > + * enforcing file integrity is left up to the discretion of the script > > + * interpreter (userspace). > > + * > > + * On success return 0. On integrity appraisal error, assuming the file > > + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. > > + */ > > +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) > > +{ > > + if (!bprm->is_check) > > + return 0; > > + > > + return ima_bprm_check(bprm); > > +} > > + > > /** > > * ima_file_check - based on policy, collect/store measurement. > > * @file: pointer to the file to be measured > > @@ -1177,6 +1198,7 @@ static int __init init_ima(void) > > > > static struct security_hook_list ima_hooks[] __ro_after_init = { > > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > > + LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec), > > Why not replace bprm_check_security with bprm_creds_for_exec > implementation altogether? To measure/appraise/audit the interpreter specified in the direct file (e.g. ./script.sh). > > > LSM_HOOK_INIT(file_post_open, ima_file_check), > > LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile), > > LSM_HOOK_INIT(file_release, ima_file_free), > > -- > > 2.47.0 > > > > >
On 11/27/24 4:02 PM, Mimi Zohar wrote: > Like direct file execution (e.g. ./script.sh), indirect file execution > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > the new security_bprm_creds_for_exec() hook to measure and verify the > indirect file's integrity. Unlike direct file execution, indirect file > execution integrity is optionally enforced by the interpreter. > > Update the audit messages to differentiate between kernel and userspace > enforced integrity. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/ima/ima_appraise.c | 84 ++++++++++++++++++++------- > security/integrity/ima/ima_main.c | 22 +++++++ > 2 files changed, 86 insertions(+), 20 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 656c709b974f..b5f8e49cde9d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -8,6 +8,7 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/file.h> > +#include <linux/binfmts.h> > #include <linux/fs.h> > #include <linux/xattr.h> > #include <linux/magic.h> > @@ -16,6 +17,7 @@ > #include <linux/fsverity.h> > #include <keys/system_keyring.h> > #include <uapi/linux/fsverity.h> > +#include <linux/securebits.h> > > #include "ima.h" > > @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, > */ > static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > - enum integrity_status *status, const char **cause) > + enum integrity_status *status, const char **cause, > + bool is_check) > { > struct ima_max_digest_data hash; > struct signature_v2_hdr *sig; > @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > if (*status != INTEGRITY_PASS_IMMUTABLE) { > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (iint->flags & IMA_VERITY_REQUIRED) > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > else > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > else > rc = -EINVAL; > if (rc) { > - *cause = "invalid-hash"; > + *cause = !is_check ? "invalid-hash" : > + "invalid-hash(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; > if ((iint->flags & mask) == mask) { > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > > sig = (typeof(sig))xattr_value; > if (sig->version >= 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > iint->ima_hash->digest, > iint->ima_hash->length); > if (rc) { > - *cause = "invalid-signature"; > + *cause = !is_check ? "invalid-signature" : > + "invalid-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (!(iint->flags & IMA_VERITY_REQUIRED)) { > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > sig = (typeof(sig))xattr_value; > if (sig->version != 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > container_of(&hash.hdr, > struct ima_digest_data, hdr)); > if (rc) { > - *cause = "sigv3-hashing-error"; > + *cause = !is_check ? "sigv3-hashing-error" : > + "sigv3-hashing-error(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > xattr_len, hash.digest, > hash.hdr.length); > if (rc) { > - *cause = "invalid-verity-signature"; > + *cause = !is_check ? "invalid-verity-signature" : > + "invalid-verify-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > break; > default: > *status = INTEGRITY_UNKNOWN; > - *cause = "unknown-ima-data"; > + *cause = !is_check ? "unknown-ima-data" : > + "unknown-ima-data(userspace)"; > break; > } > > @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, > return rc; > } > > +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) is_check is bool, so this should probably also return bool > +{ > + struct linux_binprm *bprm = NULL; > + > + if (func == BPRM_CHECK) { > + bprm = container_of(&file, struct linux_binprm, file); > + if (bprm->is_check) > + return 1; > + } > + return 0; > +} > + > /* > * ima_appraise_measurement - appraise file measurement > * > @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > enum integrity_status status = INTEGRITY_UNKNOWN; > int rc = xattr_len; > bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; > + bool is_check = false; no need to initialize it
On Mon, Dec 02, 2024 at 02:40:35PM -0500, Mimi Zohar wrote: > On Fri, 2024-11-29 at 12:06 +0100, Mickaël Salaün wrote: > > For reference, here is the base patch series: > > https://lore.kernel.org/all/20241112191858.162021-1-mic@digikod.net/ > > > > CCing audit@ > > > > On Wed, Nov 27, 2024 at 04:02:34PM -0500, Mimi Zohar wrote: > > > Like direct file execution (e.g. ./script.sh), indirect file execution > > > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > > > the new security_bprm_creds_for_exec() hook to measure and verify the > > > indirect file's integrity. Unlike direct file execution, indirect file > > > execution integrity is optionally enforced by the interpreter. > > > > > > Update the audit messages to differentiate between kernel and userspace > > > enforced integrity. > > > > I'm not sure to see the full picture. What is the difference between > > execveat() calls and execveat() + AT_EXECVE_CHECK calls? Both are from > > user space, the only difference is that the first can lead to a full > > execution, but the intent is the same. > > We do want the full execution in order to measure/appraise/audit both the direct > file execution (e.g. ./script.sh) and the interpreter (e.g. #!/usr/bin/bash) > specified. Yes, but I was wondering about the difference in the log messages. In both cases the script is checked, but only without AT_EXECVE_CHECK its "dependencies" (e.g. script interpreter) are checked. I guess it could be useful to differenciate those but I wanted to make sure we were on the same page. > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > --- > > > security/integrity/ima/ima_appraise.c | 84 ++++++++++++++++++++------- > > > security/integrity/ima/ima_main.c | 22 +++++++ > > > 2 files changed, 86 insertions(+), 20 deletions(-) > > > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > > index 656c709b974f..b5f8e49cde9d 100644 > > > --- a/security/integrity/ima/ima_appraise.c > > > +++ b/security/integrity/ima/ima_appraise.c > > > @@ -8,6 +8,7 @@ > > > #include <linux/module.h> > > > #include <linux/init.h> > > > #include <linux/file.h> > > > +#include <linux/binfmts.h> > > > #include <linux/fs.h> > > > #include <linux/xattr.h> > > > #include <linux/magic.h> > > > @@ -16,6 +17,7 @@ > > > #include <linux/fsverity.h> > > > #include <keys/system_keyring.h> > > > #include <uapi/linux/fsverity.h> > > > +#include <linux/securebits.h> > > > > > > #include "ima.h" > > > > > > @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, > > > */ > > > static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > - enum integrity_status *status, const char **cause) > > > + enum integrity_status *status, const char **cause, > > > + bool is_check) > > > { > > > struct ima_max_digest_data hash; > > > struct signature_v2_hdr *sig; > > > @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > if (*status != INTEGRITY_PASS_IMMUTABLE) { > > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > > if (iint->flags & IMA_VERITY_REQUIRED) > > > - *cause = "verity-signature-required"; > > > + *cause = !is_check ? "verity-signature-required" : > > > + "verity-signature-required(userspace)"; > > > > This looks simpler (same for all following checks): > > is_check ? "verity-signature-required(userspace)" : "verity-signature-required"; > > > > > else > > > - *cause = "IMA-signature-required"; > > > + *cause = !is_check ? "IMA-signature-required" : > > > + "IMA-signature-required(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > else > > > rc = -EINVAL; > > > if (rc) { > > > - *cause = "invalid-hash"; > > > + *cause = !is_check ? "invalid-hash" : > > > + "invalid-hash(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > > > mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; > > > if ((iint->flags & mask) == mask) { > > > - *cause = "verity-signature-required"; > > > + *cause = !is_check ? "verity-signature-required" : > > > + "verity-signature-required(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > > > > sig = (typeof(sig))xattr_value; > > > if (sig->version >= 3) { > > > - *cause = "invalid-signature-version"; > > > + *cause = !is_check ? "invalid-signature-version" : > > > + "invalid-signature-version(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > iint->ima_hash->digest, > > > iint->ima_hash->length); > > > if (rc) { > > > - *cause = "invalid-signature"; > > > + *cause = !is_check ? "invalid-signature" : > > > + "invalid-signature(userspace)"; > > > *status = INTEGRITY_FAIL; > > > } else { > > > *status = INTEGRITY_PASS; > > > @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > > if (!(iint->flags & IMA_VERITY_REQUIRED)) { > > > - *cause = "IMA-signature-required"; > > > + *cause = !is_check ? "IMA-signature-required" : > > > + "IMA-signature-required(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > > > > sig = (typeof(sig))xattr_value; > > > if (sig->version != 3) { > > > - *cause = "invalid-signature-version"; > > > + *cause = !is_check ? "invalid-signature-version" : > > > + "invalid-signature-version(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > container_of(&hash.hdr, > > > struct ima_digest_data, hdr)); > > > if (rc) { > > > - *cause = "sigv3-hashing-error"; > > > + *cause = !is_check ? "sigv3-hashing-error" : > > > + "sigv3-hashing-error(userspace)"; > > > *status = INTEGRITY_FAIL; > > > break; > > > } > > > @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > xattr_len, hash.digest, > > > hash.hdr.length); > > > if (rc) { > > > - *cause = "invalid-verity-signature"; > > > + *cause = !is_check ? "invalid-verity-signature" : > > > + "invalid-verify-signature(userspace)"; > > > *status = INTEGRITY_FAIL; > > > } else { > > > *status = INTEGRITY_PASS; > > > @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > > break; > > > default: > > > *status = INTEGRITY_UNKNOWN; > > > - *cause = "unknown-ima-data"; > > > + *cause = !is_check ? "unknown-ima-data" : > > > + "unknown-ima-data(userspace)"; > > > break; > > > } > > > > > > @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, > > > return rc; > > > } > > > > > > +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) > > > +{ > > > + struct linux_binprm *bprm = NULL; > > > + > > > + if (func == BPRM_CHECK) { > > > + bprm = container_of(&file, struct linux_binprm, file); > > > + if (bprm->is_check) > > > + return 1; > > > + } > > > + return 0; > > > +} > > > + > > > /* > > > * ima_appraise_measurement - appraise file measurement > > > * > > > @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > enum integrity_status status = INTEGRITY_UNKNOWN; > > > int rc = xattr_len; > > > bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; > > > + bool is_check = false; > > > > > > /* If not appraising a modsig, we need an xattr. */ > > > if (!(inode->i_opflags & IOP_XATTR) && !try_modsig) > > > return INTEGRITY_UNKNOWN; > > > > > > + /* > > > + * Unlike any of the other LSM hooks where the kernel enforces file > > > + * integrity, enforcing file integrity for the bprm_creds_for_exec() > > > + * LSM hook is left up to the discretion of the script interpreter > > > + * (userspace). > > > + * > > > + * Since the SECBIT_EXEC_RESTRICT_FILE flag is just a hint as to > > > + * userspace intentions, simply annotate the audit messages indicating > > > + * a userspace based query. > > > + */ > > > + is_check = is_bprm_creds_for_exec(func, file); > > > + > > > /* If reading the xattr failed and there's no modsig, error out. */ > > > if (rc <= 0 && !try_modsig) { > > > if (rc && rc != -ENODATA) > > > @@ -501,11 +540,14 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > > > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > > > if (iint->flags & IMA_VERITY_REQUIRED) > > > - cause = "verity-signature-required"; > > > + cause = !is_check ? "verity-signature-required" : > > > + "verity-signature-required(userspace)"; > > > else > > > - cause = "IMA-signature-required"; > > > + cause = !is_check ? "IMA-signature-required" : > > > + "IMA-signature-required(userspace)"; > > > } else { > > > - cause = "missing-hash"; > > > + cause = !is_check ? "missing-hash" : > > > + "missing-hash(userspace)"; > > > } > > > > > > status = INTEGRITY_NOLABEL; > > > @@ -531,14 +573,15 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > break; > > > fallthrough; > > > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > > > - cause = "missing-HMAC"; > > > + cause = !is_check ? "missing-HMAC" : "missing-HMAC(userspace)"; > > > goto out; > > > case INTEGRITY_FAIL_IMMUTABLE: > > > set_bit(IMA_DIGSIG, &iint->atomic_flags); > > > - cause = "invalid-fail-immutable"; > > > + cause = !is_check ? "invalid-fail-immutable" : > > > + "invalid-fail-immutable(userspace)"; > > > goto out; > > > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > > > - cause = "invalid-HMAC"; > > > + cause = !is_check ? "invalid-HMAC" : "invalid-HMAC(userspace)"; > > > goto out; > > > default: > > > WARN_ONCE(true, "Unexpected integrity status %d\n", status); > > > @@ -546,7 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > > > > if (xattr_value) > > > rc = xattr_verify(func, iint, xattr_value, xattr_len, &status, > > > - &cause); > > > + &cause, is_check); > > > > > > /* > > > * If we have a modsig and either no imasig or the imasig's key isn't > > > @@ -568,7 +611,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > > ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) || > > > (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) { > > > status = INTEGRITY_FAIL; > > > - cause = "unverifiable-signature"; > > > + cause = !is_check ? "unverifiable-signature" : > > > + "unverifiable-signature(userspace)"; > > > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, > > > op, cause, rc, 0); > > > > Instead of adding new causes, another option would be to add a new audit > > record type (e.g. AUDIT_INTEGRITY_DATA_CHECK). This would help filter > > these new kind of messages and I guess scale better. > > Thanks. This sounds like a better alternative. > > > > > Another alternative would be to extend the audit message with a new > > field (e.g. "check=1"), but that would not help for efficient filtering. > > > > > } else if (status != INTEGRITY_PASS) { > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > > index 06132cf47016..2b5d6bae77a4 100644 > > > --- a/security/integrity/ima/ima_main.c > > > +++ b/security/integrity/ima/ima_main.c > > > @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm) > > > MAY_EXEC, CREDS_CHECK); > > > } > > > > > > +/** > > > + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. > > > + * @bprm: contains the linux_binprm structure > > > + * > > > + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and > > > + * appraise the integrity of a file to be executed by script interpreters. > > > + * Unlike any of the other LSM hooks where the kernel enforces file integrity, > > > + * enforcing file integrity is left up to the discretion of the script > > > + * interpreter (userspace). > > > + * > > > + * On success return 0. On integrity appraisal error, assuming the file > > > + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. > > > + */ > > > +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) > > > +{ > > > + if (!bprm->is_check) > > > + return 0; > > > + > > > + return ima_bprm_check(bprm); > > > +} > > > + > > > /** > > > * ima_file_check - based on policy, collect/store measurement. > > > * @file: pointer to the file to be measured > > > @@ -1177,6 +1198,7 @@ static int __init init_ima(void) > > > > > > static struct security_hook_list ima_hooks[] __ro_after_init = { > > > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > > > + LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec), > > > > Why not replace bprm_check_security with bprm_creds_for_exec > > implementation altogether? > > To measure/appraise/audit the interpreter specified in the direct file (e.g. > ./script.sh). It makes sense. :) And there is no need to add another is_check check in the bprm_check_security implementation because it will not be called when is_check is set. > > > > > > LSM_HOOK_INIT(file_post_open, ima_file_check), > > > LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile), > > > LSM_HOOK_INIT(file_release, ima_file_free), > > > -- > > > 2.47.0 > > > > > > > > > >
Fan, would IPE need a similar update? See https://lore.kernel.org/r/20241127210234.121546-1-zohar@linux.ibm.com On Wed, Nov 27, 2024 at 04:02:34PM -0500, Mimi Zohar wrote: > Like direct file execution (e.g. ./script.sh), indirect file execution > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > the new security_bprm_creds_for_exec() hook to measure and verify the > indirect file's integrity. Unlike direct file execution, indirect file > execution integrity is optionally enforced by the interpreter. > > Update the audit messages to differentiate between kernel and userspace > enforced integrity. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > security/integrity/ima/ima_appraise.c | 84 ++++++++++++++++++++------- > security/integrity/ima/ima_main.c | 22 +++++++ > 2 files changed, 86 insertions(+), 20 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 656c709b974f..b5f8e49cde9d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -8,6 +8,7 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/file.h> > +#include <linux/binfmts.h> > #include <linux/fs.h> > #include <linux/xattr.h> > #include <linux/magic.h> > @@ -16,6 +17,7 @@ > #include <linux/fsverity.h> > #include <keys/system_keyring.h> > #include <uapi/linux/fsverity.h> > +#include <linux/securebits.h> > > #include "ima.h" > > @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, > */ > static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > - enum integrity_status *status, const char **cause) > + enum integrity_status *status, const char **cause, > + bool is_check) > { > struct ima_max_digest_data hash; > struct signature_v2_hdr *sig; > @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > if (*status != INTEGRITY_PASS_IMMUTABLE) { > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (iint->flags & IMA_VERITY_REQUIRED) > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > else > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > else > rc = -EINVAL; > if (rc) { > - *cause = "invalid-hash"; > + *cause = !is_check ? "invalid-hash" : > + "invalid-hash(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; > if ((iint->flags & mask) == mask) { > - *cause = "verity-signature-required"; > + *cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > > sig = (typeof(sig))xattr_value; > if (sig->version >= 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > iint->ima_hash->digest, > iint->ima_hash->length); > if (rc) { > - *cause = "invalid-signature"; > + *cause = !is_check ? "invalid-signature" : > + "invalid-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (!(iint->flags & IMA_VERITY_REQUIRED)) { > - *cause = "IMA-signature-required"; > + *cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > > sig = (typeof(sig))xattr_value; > if (sig->version != 3) { > - *cause = "invalid-signature-version"; > + *cause = !is_check ? "invalid-signature-version" : > + "invalid-signature-version(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > container_of(&hash.hdr, > struct ima_digest_data, hdr)); > if (rc) { > - *cause = "sigv3-hashing-error"; > + *cause = !is_check ? "sigv3-hashing-error" : > + "sigv3-hashing-error(userspace)"; > *status = INTEGRITY_FAIL; > break; > } > @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > xattr_len, hash.digest, > hash.hdr.length); > if (rc) { > - *cause = "invalid-verity-signature"; > + *cause = !is_check ? "invalid-verity-signature" : > + "invalid-verify-signature(userspace)"; > *status = INTEGRITY_FAIL; > } else { > *status = INTEGRITY_PASS; > @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, > break; > default: > *status = INTEGRITY_UNKNOWN; > - *cause = "unknown-ima-data"; > + *cause = !is_check ? "unknown-ima-data" : > + "unknown-ima-data(userspace)"; > break; > } > > @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, > return rc; > } > > +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) > +{ > + struct linux_binprm *bprm = NULL; > + > + if (func == BPRM_CHECK) { > + bprm = container_of(&file, struct linux_binprm, file); > + if (bprm->is_check) > + return 1; > + } > + return 0; > +} > + > /* > * ima_appraise_measurement - appraise file measurement > * > @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > enum integrity_status status = INTEGRITY_UNKNOWN; > int rc = xattr_len; > bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; > + bool is_check = false; > > /* If not appraising a modsig, we need an xattr. */ > if (!(inode->i_opflags & IOP_XATTR) && !try_modsig) > return INTEGRITY_UNKNOWN; > > + /* > + * Unlike any of the other LSM hooks where the kernel enforces file > + * integrity, enforcing file integrity for the bprm_creds_for_exec() > + * LSM hook is left up to the discretion of the script interpreter > + * (userspace). > + * > + * Since the SECBIT_EXEC_RESTRICT_FILE flag is just a hint as to > + * userspace intentions, simply annotate the audit messages indicating > + * a userspace based query. > + */ > + is_check = is_bprm_creds_for_exec(func, file); > + > /* If reading the xattr failed and there's no modsig, error out. */ > if (rc <= 0 && !try_modsig) { > if (rc && rc != -ENODATA) > @@ -501,11 +540,14 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > if (iint->flags & IMA_DIGSIG_REQUIRED) { > if (iint->flags & IMA_VERITY_REQUIRED) > - cause = "verity-signature-required"; > + cause = !is_check ? "verity-signature-required" : > + "verity-signature-required(userspace)"; > else > - cause = "IMA-signature-required"; > + cause = !is_check ? "IMA-signature-required" : > + "IMA-signature-required(userspace)"; > } else { > - cause = "missing-hash"; > + cause = !is_check ? "missing-hash" : > + "missing-hash(userspace)"; > } > > status = INTEGRITY_NOLABEL; > @@ -531,14 +573,15 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > break; > fallthrough; > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > - cause = "missing-HMAC"; > + cause = !is_check ? "missing-HMAC" : "missing-HMAC(userspace)"; > goto out; > case INTEGRITY_FAIL_IMMUTABLE: > set_bit(IMA_DIGSIG, &iint->atomic_flags); > - cause = "invalid-fail-immutable"; > + cause = !is_check ? "invalid-fail-immutable" : > + "invalid-fail-immutable(userspace)"; > goto out; > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > - cause = "invalid-HMAC"; > + cause = !is_check ? "invalid-HMAC" : "invalid-HMAC(userspace)"; > goto out; > default: > WARN_ONCE(true, "Unexpected integrity status %d\n", status); > @@ -546,7 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > > if (xattr_value) > rc = xattr_verify(func, iint, xattr_value, xattr_len, &status, > - &cause); > + &cause, is_check); > > /* > * If we have a modsig and either no imasig or the imasig's key isn't > @@ -568,7 +611,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, > ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) || > (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) { > status = INTEGRITY_FAIL; > - cause = "unverifiable-signature"; > + cause = !is_check ? "unverifiable-signature" : > + "unverifiable-signature(userspace)"; > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, > op, cause, rc, 0); > } else if (status != INTEGRITY_PASS) { > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 06132cf47016..2b5d6bae77a4 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm) > MAY_EXEC, CREDS_CHECK); > } > > +/** > + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. > + * @bprm: contains the linux_binprm structure > + * > + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and > + * appraise the integrity of a file to be executed by script interpreters. > + * Unlike any of the other LSM hooks where the kernel enforces file integrity, > + * enforcing file integrity is left up to the discretion of the script > + * interpreter (userspace). > + * > + * On success return 0. On integrity appraisal error, assuming the file > + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. > + */ > +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) > +{ > + if (!bprm->is_check) > + return 0; > + > + return ima_bprm_check(bprm); > +} > + > /** > * ima_file_check - based on policy, collect/store measurement. > * @file: pointer to the file to be measured > @@ -1177,6 +1198,7 @@ static int __init init_ima(void) > > static struct security_hook_list ima_hooks[] __ro_after_init = { > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > + LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec), > LSM_HOOK_INIT(file_post_open, ima_file_check), > LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile), > LSM_HOOK_INIT(file_release, ima_file_free), > -- > 2.47.0 > >
On Tue, 2024-12-03 at 12:53 +0100, Mickaël Salaün wrote: > On Mon, Dec 02, 2024 at 02:40:35PM -0500, Mimi Zohar wrote: > > On Fri, 2024-11-29 at 12:06 +0100, Mickaël Salaün wrote: > > > For reference, here is the base patch series: > > > https://lore.kernel.org/all/20241112191858.162021-1-mic@digikod.net/ > > > > > > CCing audit@ > > > > > > On Wed, Nov 27, 2024 at 04:02:34PM -0500, Mimi Zohar wrote: > > > > Like direct file execution (e.g. ./script.sh), indirect file execution > > > > (e.g. sh script.sh) needs to be measured and appraised. Instantiate > > > > the new security_bprm_creds_for_exec() hook to measure and verify the > > > > indirect file's integrity. Unlike direct file execution, indirect file > > > > execution integrity is optionally enforced by the interpreter. > > > > > > > > Update the audit messages to differentiate between kernel and userspace > > > > enforced integrity. > > > > > > I'm not sure to see the full picture. What is the difference between > > > execveat() calls and execveat() + AT_EXECVE_CHECK calls? Both are from > > > user space, the only difference is that the first can lead to a full > > > execution, but the intent is the same. > > > > We do want the full execution in order to measure/appraise/audit both the direct > > file execution (e.g. ./script.sh) and the interpreter (e.g. #!/usr/bin/bash) > > specified. > > Yes, but I was wondering about the difference in the log messages. In > both cases the script is checked, but only without AT_EXECVE_CHECK its > "dependencies" (e.g. script interpreter) are checked. I guess it could > be useful to differenciate those but I wanted to make sure we were on > the same page. By "those" I assume you're referring to with/without AT_EXECVE_CHECK and not the missing "dependencies". In both cases the integrity of the script is being checked, but in one case the integrity is being enforced by the kernel, while in the other case userspace may enforce integrity. The audit message should different between these two cases. Mimi
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 656c709b974f..b5f8e49cde9d 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -8,6 +8,7 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/file.h> +#include <linux/binfmts.h> #include <linux/fs.h> #include <linux/xattr.h> #include <linux/magic.h> @@ -16,6 +17,7 @@ #include <linux/fsverity.h> #include <keys/system_keyring.h> #include <uapi/linux/fsverity.h> +#include <linux/securebits.h> #include "ima.h" @@ -276,7 +278,8 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type, */ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, struct evm_ima_xattr_data *xattr_value, int xattr_len, - enum integrity_status *status, const char **cause) + enum integrity_status *status, const char **cause, + bool is_check) { struct ima_max_digest_data hash; struct signature_v2_hdr *sig; @@ -292,9 +295,11 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, if (*status != INTEGRITY_PASS_IMMUTABLE) { if (iint->flags & IMA_DIGSIG_REQUIRED) { if (iint->flags & IMA_VERITY_REQUIRED) - *cause = "verity-signature-required"; + *cause = !is_check ? "verity-signature-required" : + "verity-signature-required(userspace)"; else - *cause = "IMA-signature-required"; + *cause = !is_check ? "IMA-signature-required" : + "IMA-signature-required(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -314,7 +319,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, else rc = -EINVAL; if (rc) { - *cause = "invalid-hash"; + *cause = !is_check ? "invalid-hash" : + "invalid-hash(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -325,14 +331,16 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED; if ((iint->flags & mask) == mask) { - *cause = "verity-signature-required"; + *cause = !is_check ? "verity-signature-required" : + "verity-signature-required(userspace)"; *status = INTEGRITY_FAIL; break; } sig = (typeof(sig))xattr_value; if (sig->version >= 3) { - *cause = "invalid-signature-version"; + *cause = !is_check ? "invalid-signature-version" : + "invalid-signature-version(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -353,7 +361,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, iint->ima_hash->digest, iint->ima_hash->length); if (rc) { - *cause = "invalid-signature"; + *cause = !is_check ? "invalid-signature" : + "invalid-signature(userspace)"; *status = INTEGRITY_FAIL; } else { *status = INTEGRITY_PASS; @@ -364,7 +373,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, if (iint->flags & IMA_DIGSIG_REQUIRED) { if (!(iint->flags & IMA_VERITY_REQUIRED)) { - *cause = "IMA-signature-required"; + *cause = !is_check ? "IMA-signature-required" : + "IMA-signature-required(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -372,7 +382,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, sig = (typeof(sig))xattr_value; if (sig->version != 3) { - *cause = "invalid-signature-version"; + *cause = !is_check ? "invalid-signature-version" : + "invalid-signature-version(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -382,7 +393,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, container_of(&hash.hdr, struct ima_digest_data, hdr)); if (rc) { - *cause = "sigv3-hashing-error"; + *cause = !is_check ? "sigv3-hashing-error" : + "sigv3-hashing-error(userspace)"; *status = INTEGRITY_FAIL; break; } @@ -392,7 +404,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, xattr_len, hash.digest, hash.hdr.length); if (rc) { - *cause = "invalid-verity-signature"; + *cause = !is_check ? "invalid-verity-signature" : + "invalid-verify-signature(userspace)"; *status = INTEGRITY_FAIL; } else { *status = INTEGRITY_PASS; @@ -401,7 +414,8 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint, break; default: *status = INTEGRITY_UNKNOWN; - *cause = "unknown-ima-data"; + *cause = !is_check ? "unknown-ima-data" : + "unknown-ima-data(userspace)"; break; } @@ -469,6 +483,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint, return rc; } +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file) +{ + struct linux_binprm *bprm = NULL; + + if (func == BPRM_CHECK) { + bprm = container_of(&file, struct linux_binprm, file); + if (bprm->is_check) + return 1; + } + return 0; +} + /* * ima_appraise_measurement - appraise file measurement * @@ -489,11 +515,24 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, enum integrity_status status = INTEGRITY_UNKNOWN; int rc = xattr_len; bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig; + bool is_check = false; /* If not appraising a modsig, we need an xattr. */ if (!(inode->i_opflags & IOP_XATTR) && !try_modsig) return INTEGRITY_UNKNOWN; + /* + * Unlike any of the other LSM hooks where the kernel enforces file + * integrity, enforcing file integrity for the bprm_creds_for_exec() + * LSM hook is left up to the discretion of the script interpreter + * (userspace). + * + * Since the SECBIT_EXEC_RESTRICT_FILE flag is just a hint as to + * userspace intentions, simply annotate the audit messages indicating + * a userspace based query. + */ + is_check = is_bprm_creds_for_exec(func, file); + /* If reading the xattr failed and there's no modsig, error out. */ if (rc <= 0 && !try_modsig) { if (rc && rc != -ENODATA) @@ -501,11 +540,14 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, if (iint->flags & IMA_DIGSIG_REQUIRED) { if (iint->flags & IMA_VERITY_REQUIRED) - cause = "verity-signature-required"; + cause = !is_check ? "verity-signature-required" : + "verity-signature-required(userspace)"; else - cause = "IMA-signature-required"; + cause = !is_check ? "IMA-signature-required" : + "IMA-signature-required(userspace)"; } else { - cause = "missing-hash"; + cause = !is_check ? "missing-hash" : + "missing-hash(userspace)"; } status = INTEGRITY_NOLABEL; @@ -531,14 +573,15 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, break; fallthrough; case INTEGRITY_NOLABEL: /* No security.evm xattr. */ - cause = "missing-HMAC"; + cause = !is_check ? "missing-HMAC" : "missing-HMAC(userspace)"; goto out; case INTEGRITY_FAIL_IMMUTABLE: set_bit(IMA_DIGSIG, &iint->atomic_flags); - cause = "invalid-fail-immutable"; + cause = !is_check ? "invalid-fail-immutable" : + "invalid-fail-immutable(userspace)"; goto out; case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ - cause = "invalid-HMAC"; + cause = !is_check ? "invalid-HMAC" : "invalid-HMAC(userspace)"; goto out; default: WARN_ONCE(true, "Unexpected integrity status %d\n", status); @@ -546,7 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, if (xattr_value) rc = xattr_verify(func, iint, xattr_value, xattr_len, &status, - &cause); + &cause, is_check); /* * If we have a modsig and either no imasig or the imasig's key isn't @@ -568,7 +611,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint, ((inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) || (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) { status = INTEGRITY_FAIL; - cause = "unverifiable-signature"; + cause = !is_check ? "unverifiable-signature" : + "unverifiable-signature(userspace)"; integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, op, cause, rc, 0); } else if (status != INTEGRITY_PASS) { diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 06132cf47016..2b5d6bae77a4 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm) MAY_EXEC, CREDS_CHECK); } +/** + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement. + * @bprm: contains the linux_binprm structure + * + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and + * appraise the integrity of a file to be executed by script interpreters. + * Unlike any of the other LSM hooks where the kernel enforces file integrity, + * enforcing file integrity is left up to the discretion of the script + * interpreter (userspace). + * + * On success return 0. On integrity appraisal error, assuming the file + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. + */ +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm) +{ + if (!bprm->is_check) + return 0; + + return ima_bprm_check(bprm); +} + /** * ima_file_check - based on policy, collect/store measurement. * @file: pointer to the file to be measured @@ -1177,6 +1198,7 @@ static int __init init_ima(void) static struct security_hook_list ima_hooks[] __ro_after_init = { LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), + LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec), LSM_HOOK_INIT(file_post_open, ima_file_check), LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile), LSM_HOOK_INIT(file_release, ima_file_free),