Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, 2013-02-18 at 13:21 -0500, Vivek Goyal wrote: > On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote: > > On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: > > > On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: > > > > > > [..] > > > > > Yep, I got that. Default policy gets overruled when a new policy is > > > > > loaded. > > > > > > > > > > In secureboot mode, somehow above rule needs to take effect by > > > > > default. > > > > > One option would be that kernel can enforce above rule. > > > > > (I guess by adding it to both default_list as well as policy list). > > > > > > > > The default policy is empty, but can be replaced with boot command line > > > > options. The existing options are ima_tcb and/ ima_appraise_tcb. > > > > Please feel free to define an additional policy. > > > > > > I think just defining a new command line option is not sufficient > > > for secureboot use case. > > > > > > - One can easily remove kernel command line option without breaking > > > booting and easily bypass secureboot restrictions. > > > > > - I guess this is one mandated rule by secureboot. There might still > > > be a user policy which can co-exist with this rule. > > > > > > So to me this is not a new policy. It is just one mandatory rule which > > > gets appended to any policy in secureboot mode. Think of it as mandatory > > > rule imposed by kernel for any policy user can define. And in secureboot > > > mode a user can not get rid of this rule. (Otherwise it breaks user > > > space signing and one can bypass secureboot and boot into unsigned > > > kernel). > > > > Your rule allows both signed and unsigned files to be executed. Signed > > files will just have more capabilities. The ima_appraise_tcb option > > requires all files owned by root to be signed, otherwise access is > > denied. The two policies simply can not co-exist. > > Thinking loud. I guess we might have to extend ima policy/rules to allow > multiple appraise rules to co-exist. And access permission will finally > depend on if all the rules in same category return success. ima_appraise_tcb rules: dont_appraise fsmagic=PROC_SUPER_MAGIC . . . dont_appraise fsmagic=SELINUX_MAGIC appraise fowner=0 ima_secureboot: appraise fowner=0 func=bprm appraise_type=optional The ima_appraise_tcb appraise rule includes everything that would match the ima_secureboot rule. It isn't possible to combine the two policies. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, 2013-02-18 at 13:21 -0500, Vivek Goyal wrote: On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote: On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: [..] Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. I think just defining a new command line option is not sufficient for secureboot use case. - One can easily remove kernel command line option without breaking booting and easily bypass secureboot restrictions. - I guess this is one mandated rule by secureboot. There might still be a user policy which can co-exist with this rule. So to me this is not a new policy. It is just one mandatory rule which gets appended to any policy in secureboot mode. Think of it as mandatory rule imposed by kernel for any policy user can define. And in secureboot mode a user can not get rid of this rule. (Otherwise it breaks user space signing and one can bypass secureboot and boot into unsigned kernel). Your rule allows both signed and unsigned files to be executed. Signed files will just have more capabilities. The ima_appraise_tcb option requires all files owned by root to be signed, otherwise access is denied. The two policies simply can not co-exist. Thinking loud. I guess we might have to extend ima policy/rules to allow multiple appraise rules to co-exist. And access permission will finally depend on if all the rules in same category return success. ima_appraise_tcb rules: dont_appraise fsmagic=PROC_SUPER_MAGIC . . . dont_appraise fsmagic=SELINUX_MAGIC appraise fowner=0 ima_secureboot: appraise fowner=0 func=bprm appraise_type=optional The ima_appraise_tcb appraise rule includes everything that would match the ima_secureboot rule. It isn't possible to combine the two policies. Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote: > On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: > > On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: > > > > [..] > > > > Yep, I got that. Default policy gets overruled when a new policy is > > > > loaded. > > > > > > > > In secureboot mode, somehow above rule needs to take effect by default. > > > > One option would be that kernel can enforce above rule. > > > > (I guess by adding it to both default_list as well as policy list). > > > > > > The default policy is empty, but can be replaced with boot command line > > > options. The existing options are ima_tcb and/ ima_appraise_tcb. > > > Please feel free to define an additional policy. > > > > I think just defining a new command line option is not sufficient > > for secureboot use case. > > > > - One can easily remove kernel command line option without breaking > > booting and easily bypass secureboot restrictions. > > > - I guess this is one mandated rule by secureboot. There might still > > be a user policy which can co-exist with this rule. > > > > So to me this is not a new policy. It is just one mandatory rule which > > gets appended to any policy in secureboot mode. Think of it as mandatory > > rule imposed by kernel for any policy user can define. And in secureboot > > mode a user can not get rid of this rule. (Otherwise it breaks user > > space signing and one can bypass secureboot and boot into unsigned > > kernel). > > Your rule allows both signed and unsigned files to be executed. Signed > files will just have more capabilities. The ima_appraise_tcb option > requires all files owned by root to be signed, otherwise access is > denied. The two policies simply can not co-exist. Thinking loud. I guess we might have to extend ima policy/rules to allow multiple appraise rules to co-exist. And access permission will finally depend on if all the rules in same category return success. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 10:30:15AM -0500, Mimi Zohar wrote: On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: [..] Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. I think just defining a new command line option is not sufficient for secureboot use case. - One can easily remove kernel command line option without breaking booting and easily bypass secureboot restrictions. - I guess this is one mandated rule by secureboot. There might still be a user policy which can co-exist with this rule. So to me this is not a new policy. It is just one mandatory rule which gets appended to any policy in secureboot mode. Think of it as mandatory rule imposed by kernel for any policy user can define. And in secureboot mode a user can not get rid of this rule. (Otherwise it breaks user space signing and one can bypass secureboot and boot into unsigned kernel). Your rule allows both signed and unsigned files to be executed. Signed files will just have more capabilities. The ima_appraise_tcb option requires all files owned by root to be signed, otherwise access is denied. The two policies simply can not co-exist. Thinking loud. I guess we might have to extend ima policy/rules to allow multiple appraise rules to co-exist. And access permission will finally depend on if all the rules in same category return success. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 15:57 -0500, Vivek Goyal wrote: > On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote: > > On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote: > > > > [..] > > > > > I think you're making this more complicated than it needs to be. > > > > > Allow > > > > > the execution unless the file failed signature verification. The > > > > > additional capability is given only if the signature verification > > > > > succeeds. > > > > > > > > I am just trying to bring it inline with module signature verification. > > > > There also module loading fails if signatures are present but kernel > > > > can't verify it. > > > > > > A specific hook is defined for kernel module signature verification, > > > which is enabled/disabled in Kconfig. When enabled, only signed modules > > > are loaded. The kernel module hook does not verify the integrity of the > > > userspace application (eg. insmod, modprobe), but of the kernel module > > > being loaded. > > > > > > Your original patches verified the integrity of the userspace > > > application kexec, not the image being loaded. ima_bprm_check() > > > verifies the integrity of executables. To permit both signed and > > > unsigned files to execute, we defined the 'optional' IMA policy flag, > > > with the intention of giving more capability to signed executables. > > > > > > Unless we define a kexec specific hook for verifying kernel images, it's > > > not the same. > > > > I think we are talking of two different things here. > > > > I am referring to kernel module signing where signatures are appended > > to module (not IMA hook). > > > > Also I am just referring to behavior about what happens if some error > > happens while signature verification. > > > > - If signature verification fails, it is clear what to do. > > - If signature verification passes, it is clear what to do. > > - Grey area is, what happens if some error is encountered during signature > > verification. Should the module loading be allowed/disallowed. Looking > > at the module loading code, once it is determined that module has > > signature appended to it, module loading fails if some error occurs > > during signature verification. There better not be any gray area, if CONFIG_MODULE_SIG_FORCE is enabled, only validly signed modules should be loaded. > > So I am just referring to that fact and trying to draw parallels between > > error handling during module signature verification and error handling > > when file appraisal happens in IMA. > > There can be two options. > > > > - Disallow execution only if signature verification fails. If some error > > happens during verification, ignore it, let the executable continue. > > Just that it does not get extra capability. > > > > - Disallow execution only if executable is not signed or it has valid > > signature. If executable is signed and some error happens during the > > process of verifying signature, execution is denied. > > > > Little typo in second option. I meant "Allow execution only if executable > is not signed or it has valid signatures". Executables will be run with or without a valid signature. The only fair comparison would be between loading the kernel module and setting a capability. Both are only done based on a valid signature. Of the two options, I would choose the second. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote: > On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote: > > [..] > > > > I think you're making this more complicated than it needs to be. Allow > > > > the execution unless the file failed signature verification. The > > > > additional capability is given only if the signature verification > > > > succeeds. > > > > > > I am just trying to bring it inline with module signature verification. > > > There also module loading fails if signatures are present but kernel > > > can't verify it. > > > > A specific hook is defined for kernel module signature verification, > > which is enabled/disabled in Kconfig. When enabled, only signed modules > > are loaded. The kernel module hook does not verify the integrity of the > > userspace application (eg. insmod, modprobe), but of the kernel module > > being loaded. > > > > Your original patches verified the integrity of the userspace > > application kexec, not the image being loaded. ima_bprm_check() > > verifies the integrity of executables. To permit both signed and > > unsigned files to execute, we defined the 'optional' IMA policy flag, > > with the intention of giving more capability to signed executables. > > > > Unless we define a kexec specific hook for verifying kernel images, it's > > not the same. > > I think we are talking of two different things here. > > I am referring to kernel module signing where signatures are appended > to module (not IMA hook). > > Also I am just referring to behavior about what happens if some error > happens while signature verification. > > - If signature verification fails, it is clear what to do. > - If signature verification passes, it is clear what to do. > - Grey area is, what happens if some error is encountered during signature > verification. Should the module loading be allowed/disallowed. Looking > at the module loading code, once it is determined that module has > signature appended to it, module loading fails if some error occurs > during signature verification. > > So I am just referring to that fact and trying to draw parallels between > error handling during module signature verification and error handling > when file appraisal happens in IMA. > > There can be two options. > > - Disallow execution only if signature verification fails. If some error > happens during verification, ignore it, let the executable continue. > Just that it does not get extra capability. > > - Disallow execution only if executable is not signed or it has valid > signature. If executable is signed and some error happens during the > process of verifying signature, execution is denied. > Little typo in second option. I meant "Allow execution only if executable is not signed or it has valid signatures". Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote:
[..]
> > > I think you're making this more complicated than it needs to be. Allow
> > > the execution unless the file failed signature verification. The
> > > additional capability is given only if the signature verification
> > > succeeds.
> >
> > I am just trying to bring it inline with module signature verification.
> > There also module loading fails if signatures are present but kernel
> > can't verify it.
>
> A specific hook is defined for kernel module signature verification,
> which is enabled/disabled in Kconfig. When enabled, only signed modules
> are loaded. The kernel module hook does not verify the integrity of the
> userspace application (eg. insmod, modprobe), but of the kernel module
> being loaded.
>
> Your original patches verified the integrity of the userspace
> application kexec, not the image being loaded. ima_bprm_check()
> verifies the integrity of executables. To permit both signed and
> unsigned files to execute, we defined the 'optional' IMA policy flag,
> with the intention of giving more capability to signed executables.
>
> Unless we define a kexec specific hook for verifying kernel images, it's
> not the same.
I think we are talking of two different things here.
I am referring to kernel module signing where signatures are appended
to module (not IMA hook).
Also I am just referring to behavior about what happens if some error
happens while signature verification.
- If signature verification fails, it is clear what to do.
- If signature verification passes, it is clear what to do.
- Grey area is, what happens if some error is encountered during signature
verification. Should the module loading be allowed/disallowed. Looking
at the module loading code, once it is determined that module has
signature appended to it, module loading fails if some error occurs
during signature verification.
So I am just referring to that fact and trying to draw parallels between
error handling during module signature verification and error handling
when file appraisal happens in IMA.
There can be two options.
- Disallow execution only if signature verification fails. If some error
happens during verification, ignore it, let the executable continue.
Just that it does not get extra capability.
- Disallow execution only if executable is not signed or it has valid
signature. If executable is signed and some error happens during the
process of verifying signature, execution is denied.
In V3 posting of my patches I have taken second appraoch right now. Though
it is up for detbate.
>
> > Following behavior is strange.
> >
> > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> > xattr_value->digest, rc - 1,
> > iint->ima_xattr.digest,
> > IMA_DIGEST_SIZE);
> > if (rc == -EOPNOTSUPP) {
> > status = INTEGRITY_UNKNOWN;
> > } else if (rc) {
> > cause = "invalid-signature";
> > status = INTEGRITY_FAIL;
> > } else {
> > status = INTEGRITY_PASS;
> > }
> >
> > signature verification can fail for so many reasons.
> >
> > - EINVAL
> > - keyring is not present
> > - key is not present -ENOKEY
> > - ENOTSUPP
> > - ENOMEM
> > ..
> > ..
> >
> > And in all these cases we return INTEGRITY_FAIL. But only in case of
> > -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
>
> This exception is for filesystems that don't support extended
> attributes, such as CPIO. We assumed, correctly/incorrectly, that the
> initramfs would already be measured and appraised.
Who makes use of it. ima_appraise_measurement() will return
INTEGRITY_UNKNOWN if filesystem does not support xattr. And
process_measurement() will deny access for INTEGRITY_UNKNOWN.
if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
return -EACCES;
So IIUC, if file system does not support xattr, appraisal will anyway
fail. So why to special case -EOPNOTSUPP with integrity_digsig_verify().
>
> > So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
>
> Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
> to boot.
Am I reading the code wrong? Given above code snippet, if rc is non
zero, -EACCSS will be returned. So if initramfs does not support
xattr, appraisal will fail.
I suspect following default rule saves us there as it will avoid
appraisal on tmpfs.
action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 11:17 -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > >
> > > [..]
> > > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > > point.
> > > > >
> > > > > Above option will not have any effect on evm behavior? This only
> > > > > impacts
> > > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > > is fine and file access is allowed. But if EVM is enabled and
> > > > > initialized
> > > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or
> > > > > returns
> > > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > >
> > > > Can't happen. evm_verifyxattr() is called from
> > > > ima_appraise_measurement(), only if 'security.ima' exists.
> > >
> > > Actually what I meant is following.
> > >
> > > Currently in process_measurement(), I will allow access if
> > > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> >
> > I think you're making this more complicated than it needs to be. Allow
> > the execution unless the file failed signature verification. The
> > additional capability is given only if the signature verification
> > succeeds.
>
> I am just trying to bring it inline with module signature verification.
> There also module loading fails if signatures are present but kernel
> can't verify it.
A specific hook is defined for kernel module signature verification,
which is enabled/disabled in Kconfig. When enabled, only signed modules
are loaded. The kernel module hook does not verify the integrity of the
userspace application (eg. insmod, modprobe), but of the kernel module
being loaded.
Your original patches verified the integrity of the userspace
application kexec, not the image being loaded. ima_bprm_check()
verifies the integrity of executables. To permit both signed and
unsigned files to execute, we defined the 'optional' IMA policy flag,
with the intention of giving more capability to signed executables.
Unless we define a kexec specific hook for verifying kernel images, it's
not the same.
> Following behavior is strange.
>
> rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> xattr_value->digest, rc - 1,
> iint->ima_xattr.digest,
> IMA_DIGEST_SIZE);
> if (rc == -EOPNOTSUPP) {
> status = INTEGRITY_UNKNOWN;
> } else if (rc) {
> cause = "invalid-signature";
> status = INTEGRITY_FAIL;
> } else {
> status = INTEGRITY_PASS;
> }
>
> signature verification can fail for so many reasons.
>
> - EINVAL
> - keyring is not present
> - key is not present -ENOKEY
> - ENOTSUPP
> - ENOMEM
> ..
> ..
>
> And in all these cases we return INTEGRITY_FAIL. But only in case of
> -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
This exception is for filesystems that don't support extended
attributes, such as CPIO. We assumed, correctly/incorrectly, that the
initramfs would already be measured and appraised.
> So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
to boot.
> This will bring it inline with other error codes. And then in
> process_measurement() I can allow access in every case except
> INTEGRITY_FAIL.
thanks,
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 09:40 -0500, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote: > [..] > > > If it would happen that it contains signature, then IMA_DIGSIG flag > > > would be set, > > > and process could get needed capability as Vivek wants. > > > > With the 'optional' condition, both unsigned and validly signed files > > will succeed. One way of making this information accessible to an LSM, > > would be to define a new integrity capability and set it here. The new > > integrity capability would indicate the file was validly signed. > > Thinking loud. > > The problem with integrity capability is that it goes only so far. If > we provide capability in exec() path, then that capability means much > more in the sense, we know file is locked to run from memory. An integrity > capability just means file is validly signed. > So exec() code might have to do another capability on top which will > also ensure that file is executable is locked in memory and signature > verification is done after loading in memory so that it is not open > to writing to disk block attacks. > > And based on this capability we probably need to deny write access to file > till file is open for exec() (I noticed that after load, we seem to be > allowing access to write access). I think we're back to my original comment that the bprm_check hook might need to be moved or an additional hook added, as the existing bprm_check hook is located before the file is locked from modification. :) thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: > > [..] > > > Yep, I got that. Default policy gets overruled when a new policy is > > > loaded. > > > > > > In secureboot mode, somehow above rule needs to take effect by default. > > > One option would be that kernel can enforce above rule. > > > (I guess by adding it to both default_list as well as policy list). > > > > The default policy is empty, but can be replaced with boot command line > > options. The existing options are ima_tcb and/ ima_appraise_tcb. > > Please feel free to define an additional policy. > > I think just defining a new command line option is not sufficient > for secureboot use case. > > - One can easily remove kernel command line option without breaking > booting and easily bypass secureboot restrictions. > - I guess this is one mandated rule by secureboot. There might still > be a user policy which can co-exist with this rule. > > So to me this is not a new policy. It is just one mandatory rule which > gets appended to any policy in secureboot mode. Think of it as mandatory > rule imposed by kernel for any policy user can define. And in secureboot > mode a user can not get rid of this rule. (Otherwise it breaks user > space signing and one can bypass secureboot and boot into unsigned > kernel). Your rule allows both signed and unsigned files to be executed. Signed files will just have more capabilities. The ima_appraise_tcb option requires all files owned by root to be signed, otherwise access is denied. The two policies simply can not co-exist. How about defining your single rule as ima_secureboot and making it the default policy. Only if ima_appraise_tcb is specified on the kernel command line, will the default policy be replaced. This type of change, going from a null policy to an ima_secureboot policy, would require community approval. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 11:17:19AM -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > >
> > > [..]
> > > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > > point.
> > > > >
> > > > > Above option will not have any effect on evm behavior? This only
> > > > > impacts
> > > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > > is fine and file access is allowed. But if EVM is enabled and
> > > > > initialized
> > > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or
> > > > > returns
> > > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > >
> > > > Can't happen. evm_verifyxattr() is called from
> > > > ima_appraise_measurement(), only if 'security.ima' exists.
> > >
> > > Actually what I meant is following.
> > >
> > > Currently in process_measurement(), I will allow access if
> > > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> >
> > I think you're making this more complicated than it needs to be. Allow
> > the execution unless the file failed signature verification. The
> > additional capability is given only if the signature verification
> > succeeds.
>
> I am just trying to bring it inline with module signature verification.
> There also module loading fails if signatures are present but kernel
> can't verify it.
>
> Following behavior is strange.
>
> rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> xattr_value->digest, rc - 1,
> iint->ima_xattr.digest,
> IMA_DIGEST_SIZE);
> if (rc == -EOPNOTSUPP) {
> status = INTEGRITY_UNKNOWN;
> } else if (rc) {
> cause = "invalid-signature";
> status = INTEGRITY_FAIL;
> } else {
> status = INTEGRITY_PASS;
> }
>
> signature verification can fail for so many reasons.
>
> - EINVAL
> - keyring is not present
> - key is not present -ENOKEY
> - ENOTSUPP
> - ENOMEM
> ..
> ..
>
> And in all these cases we return INTEGRITY_FAIL. But only in case of
> -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
>
> So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
> This will bring it inline with other error codes. And then in
> process_measurement() I can allow access in every case except
> INTEGRITY_FAIL.
Actually even this is not sufficient. Because if security.ima is present
and it contains digital signature but security.evm is not present (assume
EVM is enabled), then appraise_measurement() will return NOLABEL or
NOXATTRS and access to file will be allowed.
But what I am trying to implement is that if digital signatures are
present, then they have to be valid. Any failure to assess the validity
of digital signature should result in failure of exec().
So to me it is important to come to a common understanding that
appraise_type=optional affects only IMA behavior and not EVM behavior. And
then there is a need to separate out IMA and EVM return codes so that
one can enforce appraise_type=optional properly. Otherwise we are leaving
lots of grey areas behind.
Thanks
Vivek
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > point.
> > > >
> > > > Above option will not have any effect on evm behavior? This only impacts
> > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > is fine and file access is allowed. But if EVM is enabled and
> > > > initialized
> > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > >
> > > Can't happen. evm_verifyxattr() is called from
> > > ima_appraise_measurement(), only if 'security.ima' exists.
> >
> > Actually what I meant is following.
> >
> > Currently in process_measurement(), I will allow access if
> > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
>
> I think you're making this more complicated than it needs to be. Allow
> the execution unless the file failed signature verification. The
> additional capability is given only if the signature verification
> succeeds.
I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.
Following behavior is strange.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value->digest, rc - 1,
iint->ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
cause = "invalid-signature";
status = INTEGRITY_FAIL;
} else {
status = INTEGRITY_PASS;
}
signature verification can fail for so many reasons.
- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..
And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
This will bring it inline with other error codes. And then in
process_measurement() I can allow access in every case except
INTEGRITY_FAIL.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote: > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote: > > [..] > > > Ok, I will cleanup the code to do above. Just wanted to clear up one > > > point. > > > > > > Above option will not have any effect on evm behavior? This only impacts > > > IMA appraisal behavior. For example, if security.ima is not present it > > > is fine and file access is allowed. But if EVM is enabled and initialized > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns > > > INTEGRITY_NOXATTRS, file access should still be denied? > > > > Can't happen. evm_verifyxattr() is called from > > ima_appraise_measurement(), only if 'security.ima' exists. > > Actually what I meant is following. > > Currently in process_measurement(), I will allow access if > ima_appraise_measurement() returns INTEGRITY_NOLABEL. I think you're making this more complicated than it needs to be. Allow the execution unless the file failed signature verification. The additional capability is given only if the signature verification succeeds. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote: [..] > > Ok, I will cleanup the code to do above. Just wanted to clear up one > > point. > > > > Above option will not have any effect on evm behavior? This only impacts > > IMA appraisal behavior. For example, if security.ima is not present it > > is fine and file access is allowed. But if EVM is enabled and initialized > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns > > INTEGRITY_NOXATTRS, file access should still be denied? > > Can't happen. evm_verifyxattr() is called from > ima_appraise_measurement(), only if 'security.ima' exists. Actually what I meant is following. Currently in process_measurement(), I will allow access if ima_appraise_measurement() returns INTEGRITY_NOLABEL. Now this could mean 2 things. - security.ima was not present. - security.ima was there but security.evm was not present. With appraise_type=optional, I think we would want to allow access in first case but not the second one. IOW, appraise_type= affects behavior of IMA and not EVM. That means we need to introduce new codes. INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL to differentiate between above two cases? Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: [..] > > Yep, I got that. Default policy gets overruled when a new policy is > > loaded. > > > > In secureboot mode, somehow above rule needs to take effect by default. > > One option would be that kernel can enforce above rule. > > (I guess by adding it to both default_list as well as policy list). > > The default policy is empty, but can be replaced with boot command line > options. The existing options are ima_tcb and/ ima_appraise_tcb. > Please feel free to define an additional policy. I think just defining a new command line option is not sufficient for secureboot use case. - One can easily remove kernel command line option without breaking booting and easily bypass secureboot restrictions. - I guess this is one mandated rule by secureboot. There might still be a user policy which can co-exist with this rule. So to me this is not a new policy. It is just one mandatory rule which gets appended to any policy in secureboot mode. Think of it as mandatory rule imposed by kernel for any policy user can define. And in secureboot mode a user can not get rid of this rule. (Otherwise it breaks user space signing and one can bypass secureboot and boot into unsigned kernel). Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote: [..] > Option 3: appraise_type:= [imasig] | [imahash] | [optional] > > Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the > iint flags. I like option 3. If there is a use case down the line where definition of optional needs to be refined in such a way that we want to force signature or hash then we can extend appraise_type to also include following. Option 3: appraise_type:= [imasig] | [imahash] | [optional] | [imasig_optional] | [imahash_optional] > > > Any of these options should work. > > > If it would happen that it contains signature, then IMA_DIGSIG flag > > would be set, > > and process could get needed capability as Vivek wants. > > With the 'optional' condition, both unsigned and validly signed files > will succeed. One way of making this information accessible to an LSM, > would be to define a new integrity capability and set it here. The new > integrity capability would indicate the file was validly signed. Thinking loud. The problem with integrity capability is that it goes only so far. If we provide capability in exec() path, then that capability means much more in the sense, we know file is locked to run from memory. An integrity capability just means file is validly signed. So exec() code might have to do another capability on top which will also ensure that file is executable is locked in memory and signature verification is done after loading in memory so that it is not open to writing to disk block attacks. And based on this capability we probably need to deny write access to file till file is open for exec() (I noticed that after load, we seem to be allowing access to write access). Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 11:59 -0500, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote: > > [..] > > > I see it is more logical if it is "appraise_type=optional", > > > which means that we might have no xattr value, hash or signature. > > > It if happens to be a signature, then IMA_DIGSIG flag will be set. > > > > Right, 'appraise_type=' could have been defined either as a comma > > separated list of options (eg. appraise_type=imassig,optional) or, as > > Vivek implemented, a new option. Eventually, we will need to extend > > 'appraise_type=' (eg. required algorithm), but for now I don't have a > > problem with the new option. > > Ok, I will cleanup the code to do above. Just wanted to clear up one > point. > > Above option will not have any effect on evm behavior? This only impacts > IMA appraisal behavior. For example, if security.ima is not present it > is fine and file access is allowed. But if EVM is enabled and initialized > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns > INTEGRITY_NOXATTRS, file access should still be denied? Can't happen. evm_verifyxattr() is called from ima_appraise_measurement(), only if 'security.ima' exists. > BTW, what's the difference between INTEGRITY_NOLABEL and > INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()). INTEGRITY_NOLABEL indicates the requested xattr doesn't exist, while INTEGRITY_NOXATTRS implies no EVM protected xattrs exist. The latter normally occurs when a file is first created. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 11:59 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote: [..] I see it is more logical if it is appraise_type=optional, which means that we might have no xattr value, hash or signature. It if happens to be a signature, then IMA_DIGSIG flag will be set. Right, 'appraise_type=' could have been defined either as a comma separated list of options (eg. appraise_type=imassig,optional) or, as Vivek implemented, a new option. Eventually, we will need to extend 'appraise_type=' (eg. required algorithm), but for now I don't have a problem with the new option. Ok, I will cleanup the code to do above. Just wanted to clear up one point. Above option will not have any effect on evm behavior? This only impacts IMA appraisal behavior. For example, if security.ima is not present it is fine and file access is allowed. But if EVM is enabled and initialized and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns INTEGRITY_NOXATTRS, file access should still be denied? Can't happen. evm_verifyxattr() is called from ima_appraise_measurement(), only if 'security.ima' exists. BTW, what's the difference between INTEGRITY_NOLABEL and INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()). INTEGRITY_NOLABEL indicates the requested xattr doesn't exist, while INTEGRITY_NOXATTRS implies no EVM protected xattrs exist. The latter normally occurs when a file is first created. Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote: [..] Option 3: appraise_type:= [imasig] | [imahash] | [optional] Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the iint flags. I like option 3. If there is a use case down the line where definition of optional needs to be refined in such a way that we want to force signature or hash then we can extend appraise_type to also include following. Option 3: appraise_type:= [imasig] | [imahash] | [optional] | [imasig_optional] | [imahash_optional] Any of these options should work. If it would happen that it contains signature, then IMA_DIGSIG flag would be set, and process could get needed capability as Vivek wants. With the 'optional' condition, both unsigned and validly signed files will succeed. One way of making this information accessible to an LSM, would be to define a new integrity capability and set it here. The new integrity capability would indicate the file was validly signed. Thinking loud. The problem with integrity capability is that it goes only so far. If we provide capability in exec() path, then that capability means much more in the sense, we know file is locked to run from memory. An integrity capability just means file is validly signed. So exec() code might have to do another capability on top which will also ensure that file is executable is locked in memory and signature verification is done after loading in memory so that it is not open to writing to disk block attacks. And based on this capability we probably need to deny write access to file till file is open for exec() (I noticed that after load, we seem to be allowing access to write access). Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: [..] Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. I think just defining a new command line option is not sufficient for secureboot use case. - One can easily remove kernel command line option without breaking booting and easily bypass secureboot restrictions. - I guess this is one mandated rule by secureboot. There might still be a user policy which can co-exist with this rule. So to me this is not a new policy. It is just one mandatory rule which gets appended to any policy in secureboot mode. Think of it as mandatory rule imposed by kernel for any policy user can define. And in secureboot mode a user can not get rid of this rule. (Otherwise it breaks user space signing and one can bypass secureboot and boot into unsigned kernel). Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote: [..] Ok, I will cleanup the code to do above. Just wanted to clear up one point. Above option will not have any effect on evm behavior? This only impacts IMA appraisal behavior. For example, if security.ima is not present it is fine and file access is allowed. But if EVM is enabled and initialized and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns INTEGRITY_NOXATTRS, file access should still be denied? Can't happen. evm_verifyxattr() is called from ima_appraise_measurement(), only if 'security.ima' exists. Actually what I meant is following. Currently in process_measurement(), I will allow access if ima_appraise_measurement() returns INTEGRITY_NOLABEL. Now this could mean 2 things. - security.ima was not present. - security.ima was there but security.evm was not present. With appraise_type=optional, I think we would want to allow access in first case but not the second one. IOW, appraise_type= affects behavior of IMA and not EVM. That means we need to introduce new codes. INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL to differentiate between above two cases? Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote: On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote: [..] Ok, I will cleanup the code to do above. Just wanted to clear up one point. Above option will not have any effect on evm behavior? This only impacts IMA appraisal behavior. For example, if security.ima is not present it is fine and file access is allowed. But if EVM is enabled and initialized and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns INTEGRITY_NOXATTRS, file access should still be denied? Can't happen. evm_verifyxattr() is called from ima_appraise_measurement(), only if 'security.ima' exists. Actually what I meant is following. Currently in process_measurement(), I will allow access if ima_appraise_measurement() returns INTEGRITY_NOLABEL. I think you're making this more complicated than it needs to be. Allow the execution unless the file failed signature verification. The additional capability is given only if the signature verification succeeds. thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
[..]
Ok, I will cleanup the code to do above. Just wanted to clear up one
point.
Above option will not have any effect on evm behavior? This only impacts
IMA appraisal behavior. For example, if security.ima is not present it
is fine and file access is allowed. But if EVM is enabled and
initialized
and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
INTEGRITY_NOXATTRS, file access should still be denied?
Can't happen. evm_verifyxattr() is called from
ima_appraise_measurement(), only if 'security.ima' exists.
Actually what I meant is following.
Currently in process_measurement(), I will allow access if
ima_appraise_measurement() returns INTEGRITY_NOLABEL.
I think you're making this more complicated than it needs to be. Allow
the execution unless the file failed signature verification. The
additional capability is given only if the signature verification
succeeds.
I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.
Following behavior is strange.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
cause = invalid-signature;
status = INTEGRITY_FAIL;
} else {
status = INTEGRITY_PASS;
}
signature verification can fail for so many reasons.
- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..
And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
This will bring it inline with other error codes. And then in
process_measurement() I can allow access in every case except
INTEGRITY_FAIL.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 11:17:19AM -0500, Vivek Goyal wrote:
On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
[..]
Ok, I will cleanup the code to do above. Just wanted to clear up one
point.
Above option will not have any effect on evm behavior? This only
impacts
IMA appraisal behavior. For example, if security.ima is not present it
is fine and file access is allowed. But if EVM is enabled and
initialized
and EVM does not find security.evm label (INTEGRITY_NOLABEL) or
returns
INTEGRITY_NOXATTRS, file access should still be denied?
Can't happen. evm_verifyxattr() is called from
ima_appraise_measurement(), only if 'security.ima' exists.
Actually what I meant is following.
Currently in process_measurement(), I will allow access if
ima_appraise_measurement() returns INTEGRITY_NOLABEL.
I think you're making this more complicated than it needs to be. Allow
the execution unless the file failed signature verification. The
additional capability is given only if the signature verification
succeeds.
I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.
Following behavior is strange.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
cause = invalid-signature;
status = INTEGRITY_FAIL;
} else {
status = INTEGRITY_PASS;
}
signature verification can fail for so many reasons.
- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..
And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
This will bring it inline with other error codes. And then in
process_measurement() I can allow access in every case except
INTEGRITY_FAIL.
Actually even this is not sufficient. Because if security.ima is present
and it contains digital signature but security.evm is not present (assume
EVM is enabled), then appraise_measurement() will return NOLABEL or
NOXATTRS and access to file will be allowed.
But what I am trying to implement is that if digital signatures are
present, then they have to be valid. Any failure to assess the validity
of digital signature should result in failure of exec().
So to me it is important to come to a common understanding that
appraise_type=optional affects only IMA behavior and not EVM behavior. And
then there is a need to separate out IMA and EVM return codes so that
one can enforce appraise_type=optional properly. Otherwise we are leaving
lots of grey areas behind.
Thanks
Vivek
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe
linux-security-module in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 10:03 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 05:27:01PM -0500, Mimi Zohar wrote: [..] Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. I think just defining a new command line option is not sufficient for secureboot use case. - One can easily remove kernel command line option without breaking booting and easily bypass secureboot restrictions. - I guess this is one mandated rule by secureboot. There might still be a user policy which can co-exist with this rule. So to me this is not a new policy. It is just one mandatory rule which gets appended to any policy in secureboot mode. Think of it as mandatory rule imposed by kernel for any policy user can define. And in secureboot mode a user can not get rid of this rule. (Otherwise it breaks user space signing and one can bypass secureboot and boot into unsigned kernel). Your rule allows both signed and unsigned files to be executed. Signed files will just have more capabilities. The ima_appraise_tcb option requires all files owned by root to be signed, otherwise access is denied. The two policies simply can not co-exist. How about defining your single rule as ima_secureboot and making it the default policy. Only if ima_appraise_tcb is specified on the kernel command line, will the default policy be replaced. This type of change, going from a null policy to an ima_secureboot policy, would require community approval. thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 09:40 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 04:45:23PM -0500, Mimi Zohar wrote: [..] If it would happen that it contains signature, then IMA_DIGSIG flag would be set, and process could get needed capability as Vivek wants. With the 'optional' condition, both unsigned and validly signed files will succeed. One way of making this information accessible to an LSM, would be to define a new integrity capability and set it here. The new integrity capability would indicate the file was validly signed. Thinking loud. The problem with integrity capability is that it goes only so far. If we provide capability in exec() path, then that capability means much more in the sense, we know file is locked to run from memory. An integrity capability just means file is validly signed. So exec() code might have to do another capability on top which will also ensure that file is executable is locked in memory and signature verification is done after loading in memory so that it is not open to writing to disk block attacks. And based on this capability we probably need to deny write access to file till file is open for exec() (I noticed that after load, we seem to be allowing access to write access). I think we're back to my original comment that the bprm_check hook might need to be moved or an additional hook added, as the existing bprm_check hook is located before the file is locked from modification. :) thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 11:17 -0500, Vivek Goyal wrote:
On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
[..]
Ok, I will cleanup the code to do above. Just wanted to clear up one
point.
Above option will not have any effect on evm behavior? This only
impacts
IMA appraisal behavior. For example, if security.ima is not present it
is fine and file access is allowed. But if EVM is enabled and
initialized
and EVM does not find security.evm label (INTEGRITY_NOLABEL) or
returns
INTEGRITY_NOXATTRS, file access should still be denied?
Can't happen. evm_verifyxattr() is called from
ima_appraise_measurement(), only if 'security.ima' exists.
Actually what I meant is following.
Currently in process_measurement(), I will allow access if
ima_appraise_measurement() returns INTEGRITY_NOLABEL.
I think you're making this more complicated than it needs to be. Allow
the execution unless the file failed signature verification. The
additional capability is given only if the signature verification
succeeds.
I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.
A specific hook is defined for kernel module signature verification,
which is enabled/disabled in Kconfig. When enabled, only signed modules
are loaded. The kernel module hook does not verify the integrity of the
userspace application (eg. insmod, modprobe), but of the kernel module
being loaded.
Your original patches verified the integrity of the userspace
application kexec, not the image being loaded. ima_bprm_check()
verifies the integrity of executables. To permit both signed and
unsigned files to execute, we defined the 'optional' IMA policy flag,
with the intention of giving more capability to signed executables.
Unless we define a kexec specific hook for verifying kernel images, it's
not the same.
Following behavior is strange.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
cause = invalid-signature;
status = INTEGRITY_FAIL;
} else {
status = INTEGRITY_PASS;
}
signature verification can fail for so many reasons.
- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..
And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
This exception is for filesystems that don't support extended
attributes, such as CPIO. We assumed, correctly/incorrectly, that the
initramfs would already be measured and appraised.
So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
to boot.
This will bring it inline with other error codes. And then in
process_measurement() I can allow access in every case except
INTEGRITY_FAIL.
thanks,
Mimi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote:
[..]
I think you're making this more complicated than it needs to be. Allow
the execution unless the file failed signature verification. The
additional capability is given only if the signature verification
succeeds.
I am just trying to bring it inline with module signature verification.
There also module loading fails if signatures are present but kernel
can't verify it.
A specific hook is defined for kernel module signature verification,
which is enabled/disabled in Kconfig. When enabled, only signed modules
are loaded. The kernel module hook does not verify the integrity of the
userspace application (eg. insmod, modprobe), but of the kernel module
being loaded.
Your original patches verified the integrity of the userspace
application kexec, not the image being loaded. ima_bprm_check()
verifies the integrity of executables. To permit both signed and
unsigned files to execute, we defined the 'optional' IMA policy flag,
with the intention of giving more capability to signed executables.
Unless we define a kexec specific hook for verifying kernel images, it's
not the same.
I think we are talking of two different things here.
I am referring to kernel module signing where signatures are appended
to module (not IMA hook).
Also I am just referring to behavior about what happens if some error
happens while signature verification.
- If signature verification fails, it is clear what to do.
- If signature verification passes, it is clear what to do.
- Grey area is, what happens if some error is encountered during signature
verification. Should the module loading be allowed/disallowed. Looking
at the module loading code, once it is determined that module has
signature appended to it, module loading fails if some error occurs
during signature verification.
So I am just referring to that fact and trying to draw parallels between
error handling during module signature verification and error handling
when file appraisal happens in IMA.
There can be two options.
- Disallow execution only if signature verification fails. If some error
happens during verification, ignore it, let the executable continue.
Just that it does not get extra capability.
- Disallow execution only if executable is not signed or it has valid
signature. If executable is signed and some error happens during the
process of verifying signature, execution is denied.
In V3 posting of my patches I have taken second appraoch right now. Though
it is up for detbate.
Following behavior is strange.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
cause = invalid-signature;
status = INTEGRITY_FAIL;
} else {
status = INTEGRITY_PASS;
}
signature verification can fail for so many reasons.
- EINVAL
- keyring is not present
- key is not present -ENOKEY
- ENOTSUPP
- ENOMEM
..
..
And in all these cases we return INTEGRITY_FAIL. But only in case of
-EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
This exception is for filesystems that don't support extended
attributes, such as CPIO. We assumed, correctly/incorrectly, that the
initramfs would already be measured and appraised.
Who makes use of it. ima_appraise_measurement() will return
INTEGRITY_UNKNOWN if filesystem does not support xattr. And
process_measurement() will deny access for INTEGRITY_UNKNOWN.
if ((rc must_appraise) (ima_appraise IMA_APPRAISE_ENFORCE))
return -EACCES;
So IIUC, if file system does not support xattr, appraisal will anyway
fail. So why to special case -EOPNOTSUPP with integrity_digsig_verify().
So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
Perhaps, but unless the initramfs supports xattrs, we wouldn't be able
to boot.
Am I reading the code wrong? Given above code snippet, if rc is non
zero, -EACCSS will be returned. So if initramfs does not support
xattr, appraisal will fail.
I suspect following default rule saves us there as it will avoid
appraisal on tmpfs.
action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote: On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote: [..] I think you're making this more complicated than it needs to be. Allow the execution unless the file failed signature verification. The additional capability is given only if the signature verification succeeds. I am just trying to bring it inline with module signature verification. There also module loading fails if signatures are present but kernel can't verify it. A specific hook is defined for kernel module signature verification, which is enabled/disabled in Kconfig. When enabled, only signed modules are loaded. The kernel module hook does not verify the integrity of the userspace application (eg. insmod, modprobe), but of the kernel module being loaded. Your original patches verified the integrity of the userspace application kexec, not the image being loaded. ima_bprm_check() verifies the integrity of executables. To permit both signed and unsigned files to execute, we defined the 'optional' IMA policy flag, with the intention of giving more capability to signed executables. Unless we define a kexec specific hook for verifying kernel images, it's not the same. I think we are talking of two different things here. I am referring to kernel module signing where signatures are appended to module (not IMA hook). Also I am just referring to behavior about what happens if some error happens while signature verification. - If signature verification fails, it is clear what to do. - If signature verification passes, it is clear what to do. - Grey area is, what happens if some error is encountered during signature verification. Should the module loading be allowed/disallowed. Looking at the module loading code, once it is determined that module has signature appended to it, module loading fails if some error occurs during signature verification. So I am just referring to that fact and trying to draw parallels between error handling during module signature verification and error handling when file appraisal happens in IMA. There can be two options. - Disallow execution only if signature verification fails. If some error happens during verification, ignore it, let the executable continue. Just that it does not get extra capability. - Disallow execution only if executable is not signed or it has valid signature. If executable is signed and some error happens during the process of verifying signature, execution is denied. Little typo in second option. I meant Allow execution only if executable is not signed or it has valid signatures. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Thu, 2013-02-14 at 15:57 -0500, Vivek Goyal wrote: On Thu, Feb 14, 2013 at 03:54:45PM -0500, Vivek Goyal wrote: On Thu, Feb 14, 2013 at 02:49:16PM -0500, Mimi Zohar wrote: [..] I think you're making this more complicated than it needs to be. Allow the execution unless the file failed signature verification. The additional capability is given only if the signature verification succeeds. I am just trying to bring it inline with module signature verification. There also module loading fails if signatures are present but kernel can't verify it. A specific hook is defined for kernel module signature verification, which is enabled/disabled in Kconfig. When enabled, only signed modules are loaded. The kernel module hook does not verify the integrity of the userspace application (eg. insmod, modprobe), but of the kernel module being loaded. Your original patches verified the integrity of the userspace application kexec, not the image being loaded. ima_bprm_check() verifies the integrity of executables. To permit both signed and unsigned files to execute, we defined the 'optional' IMA policy flag, with the intention of giving more capability to signed executables. Unless we define a kexec specific hook for verifying kernel images, it's not the same. I think we are talking of two different things here. I am referring to kernel module signing where signatures are appended to module (not IMA hook). Also I am just referring to behavior about what happens if some error happens while signature verification. - If signature verification fails, it is clear what to do. - If signature verification passes, it is clear what to do. - Grey area is, what happens if some error is encountered during signature verification. Should the module loading be allowed/disallowed. Looking at the module loading code, once it is determined that module has signature appended to it, module loading fails if some error occurs during signature verification. There better not be any gray area, if CONFIG_MODULE_SIG_FORCE is enabled, only validly signed modules should be loaded. So I am just referring to that fact and trying to draw parallels between error handling during module signature verification and error handling when file appraisal happens in IMA. There can be two options. - Disallow execution only if signature verification fails. If some error happens during verification, ignore it, let the executable continue. Just that it does not get extra capability. - Disallow execution only if executable is not signed or it has valid signature. If executable is signed and some error happens during the process of verifying signature, execution is denied. Little typo in second option. I meant Allow execution only if executable is not signed or it has valid signatures. Executables will be run with or without a valid signature. The only fair comparison would be between loading the kernel module and setting a capability. Both are only done based on a valid signature. Of the two options, I would choose the second. Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 10:30 -0500, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote: > > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal wrote: > > > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > > >> It should not be the only line in the policy. > > >> Can you share full policy? > > > > > > I verified by putting some printk. There is only single rule in > > > ima_policy_rules list after I have updated the rules through "policy" > > > file. > > > > > > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > > > > /sys/kernel/security/policy > > > > There is a default policy which has several rules. > > > > And when you do your "echo" you will replace all rules with that single > > rule. > > But ok, you have one rule only and it is fine to have even a single rule... > > Yep, I got that. Default policy gets overruled when a new policy is > loaded. > > In secureboot mode, somehow above rule needs to take effect by default. > One option would be that kernel can enforce above rule. > (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 19:33 +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar wrote:
> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar
> >> wrote:
> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
> >> >
> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> >> >> > integrity_iint_cache *iint,
> >> >> > }
> >> >> > switch (xattr_value->type) {
> >> >> > case IMA_XATTR_DIGEST:
> >> >> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> >> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> >> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> >> > cause = "IMA signature required";
> >> >> > status = INTEGRITY_FAIL;
> >> >> > break;
> >> >>
> >> >> This looks a bit odd... If "optional" signature is missing - we fail..
> >> >> It is optional... Why we should fail?
> >> >
> >> > 'imasig_optional' does not only mean that the signature is optional, but
> >> > also implies that it has to be a digital signature, not a hash. This
> >> > latter part is what IMA_DIGSIG_REQUIRED means.
> >> >
> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >> >
> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >> >
> >>
> >> It sounds odd that optional signature is actually required.
> >> Optional for me means that it may be there or may be not.
> >> If it is not there, then it may be hash or nothing.
> >>
> >> I see it is more logical if it is "appraise_type=optional",
> >> which means that we might have no xattr value, hash or signature.
> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
> >
> > Right, 'appraise_type=' could have been defined either as a comma
> > separated list of options (eg. appraise_type=imassig,optional) or, as
> > Vivek implemented, a new option. Eventually, we will need to extend
> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
> > problem with the new option.
> >
>
> It is not exactly what I meant. IOW, I do not want
> appraise_type=imasig,optional.
>
> Optional for me is that xattr value is optional. It might be nothing,
> hash or imasig...
To summarize:
Option 1: appraise_type:= [[imasig] | [imahash]] [optional]
sample rules:
# require either hash or signature
func=bprm
# require signature
func=bprm appraise_type=imasig
# permit nothing, hash, or signature
func=bprm appraise_type=optional
# permit nothing or hash
func=bprm appraise_type=imahash, optional
# permit nothing or signature
func=bprm appraise_type=imasig, optional
Option 2: appraise_type:= [imasig] | [imasig_optional]
As I don't see a use case for either just 'optional' or 'imahash,
optional', this leaves 'imasig,optional', which could be expressed as
'imasig_optional'.
Option 3: appraise_type:= [imasig] | [imahash] | [optional]
Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the
iint flags.
Any of these options should work.
> If it would happen that it contains signature, then IMA_DIGSIG flag
> would be set,
> and process could get needed capability as Vivek wants.
With the 'optional' condition, both unsigned and validly signed files
will succeed. One way of making this information accessible to an LSM,
would be to define a new integrity capability and set it here. The new
integrity capability would indicate the file was validly signed.
thanks,
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 7:51 PM, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
>> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar wrote:
>> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
>> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar
>> >> wrote:
>> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal
>> >> >> wrote:
>> >> >
>> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
>> >> >> > integrity_iint_cache *iint,
>> >> >> > }
>> >> >> > switch (xattr_value->type) {
>> >> >> > case IMA_XATTR_DIGEST:
>> >> >> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> >> >> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> >> >> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
>> >> >> > cause = "IMA signature required";
>> >> >> > status = INTEGRITY_FAIL;
>> >> >> > break;
>> >> >>
>> >> >> This looks a bit odd... If "optional" signature is missing - we fail..
>> >> >> It is optional... Why we should fail?
>> >> >
>> >> > 'imasig_optional' does not only mean that the signature is optional, but
>> >> > also implies that it has to be a digital signature, not a hash. This
>> >> > latter part is what IMA_DIGSIG_REQUIRED means.
>> >> >
>> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
>> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>> >> >
>> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
>> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>> >> >
>> >>
>> >> It sounds odd that optional signature is actually required.
>> >> Optional for me means that it may be there or may be not.
>> >> If it is not there, then it may be hash or nothing.
>> >>
>> >> I see it is more logical if it is "appraise_type=optional",
>> >> which means that we might have no xattr value, hash or signature.
>> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
>> >
>> > Right, 'appraise_type=' could have been defined either as a comma
>> > separated list of options (eg. appraise_type=imassig,optional) or, as
>> > Vivek implemented, a new option. Eventually, we will need to extend
>> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
>> > problem with the new option.
>> >
>>
>> It is not exactly what I meant. IOW, I do not want
>> appraise_type=imasig,optional.
>>
>> Optional for me is that xattr value is optional. It might be nothing,
>> hash or imasig...
>>
>> If it would happen that it contains signature, then IMA_DIGSIG flag
>> would be set,
>> and process could get needed capability as Vivek wants.
>
> Hi Dmitry,
>
> While we are at it, I thought I will mention what else is going in my
> mind w.r.t next step.
>
> I looked at your patch. That patch looks at IMG_DIGSIG
> flag and sets bprm->unsafe with appropriate flag. It works well if
> signature verification before loading executable alone is sufficient.
>
> But this leaves a small window open where somebody could write to the
> disk block directly (bypass filesystem) after IMA signature verification.
> Then modified image will be loaded by the binary handler.
>
> To avoid that, I think I will need to do appraisal after loading the file
> (with VM_LOCKED flag).
>
> I guess I will need to introduce another hook say bprm_post_load() to
> verify integrity of file.
>
> So this raises few questions.
>
> - Are two appraisals really necessary. From my use case perspective
> initially I just need to know whether digital signature are present
> or not and appraisal of file is not required. I guess it is one
> optimization area to keep appraisal overhead minimum in exec() path.
>
> - When I go for second appraisal after loading file, current IMA code
> will have success result in iint->flags. I need to figure a way out
> to reset cache result and force reappraisal.
>
> Thanks
> Vivek
Hello,
My patch was just a 5 minute example how to go with that.
Sure, when we deal with MAP_LOCKED, we might do appraisal after
loading the binary.
Yes, it looks like that we really need another hook for that...
But then process_measurement might do things a bit differently.
It might check the policy, allocated iint and return if locking is
needed (or another hook will be called).
Next hook will just collect and appriase...
- Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar wrote:
> > On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> >> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar
> >> wrote:
> >> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
> >> >
> >> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> >> >> > integrity_iint_cache *iint,
> >> >> > }
> >> >> > switch (xattr_value->type) {
> >> >> > case IMA_XATTR_DIGEST:
> >> >> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> >> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> >> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> >> > cause = "IMA signature required";
> >> >> > status = INTEGRITY_FAIL;
> >> >> > break;
> >> >>
> >> >> This looks a bit odd... If "optional" signature is missing - we fail..
> >> >> It is optional... Why we should fail?
> >> >
> >> > 'imasig_optional' does not only mean that the signature is optional, but
> >> > also implies that it has to be a digital signature, not a hash. This
> >> > latter part is what IMA_DIGSIG_REQUIRED means.
> >> >
> >> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> >> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >> >
> >> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> >> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >> >
> >>
> >> It sounds odd that optional signature is actually required.
> >> Optional for me means that it may be there or may be not.
> >> If it is not there, then it may be hash or nothing.
> >>
> >> I see it is more logical if it is "appraise_type=optional",
> >> which means that we might have no xattr value, hash or signature.
> >> It if happens to be a signature, then IMA_DIGSIG flag will be set.
> >
> > Right, 'appraise_type=' could have been defined either as a comma
> > separated list of options (eg. appraise_type=imassig,optional) or, as
> > Vivek implemented, a new option. Eventually, we will need to extend
> > 'appraise_type=' (eg. required algorithm), but for now I don't have a
> > problem with the new option.
> >
>
> It is not exactly what I meant. IOW, I do not want
> appraise_type=imasig,optional.
>
> Optional for me is that xattr value is optional. It might be nothing,
> hash or imasig...
>
> If it would happen that it contains signature, then IMA_DIGSIG flag
> would be set,
> and process could get needed capability as Vivek wants.
Hi Dmitry,
While we are at it, I thought I will mention what else is going in my
mind w.r.t next step.
I looked at your patch. That patch looks at IMG_DIGSIG
flag and sets bprm->unsafe with appropriate flag. It works well if
signature verification before loading executable alone is sufficient.
But this leaves a small window open where somebody could write to the
disk block directly (bypass filesystem) after IMA signature verification.
Then modified image will be loaded by the binary handler.
To avoid that, I think I will need to do appraisal after loading the file
(with VM_LOCKED flag).
I guess I will need to introduce another hook say bprm_post_load() to
verify integrity of file.
So this raises few questions.
- Are two appraisals really necessary. From my use case perspective
initially I just need to know whether digital signature are present
or not and appraisal of file is not required. I guess it is one
optimization area to keep appraisal overhead minimum in exec() path.
- When I go for second appraisal after loading file, current IMA code
will have success result in iint->flags. I need to figure a way out
to reset cache result and force reappraisal.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar wrote:
> On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
>> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar wrote:
>> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
>> >
>> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
>> >> > integrity_iint_cache *iint,
>> >> > }
>> >> > switch (xattr_value->type) {
>> >> > case IMA_XATTR_DIGEST:
>> >> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> >> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> >> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
>> >> > cause = "IMA signature required";
>> >> > status = INTEGRITY_FAIL;
>> >> > break;
>> >>
>> >> This looks a bit odd... If "optional" signature is missing - we fail..
>> >> It is optional... Why we should fail?
>> >
>> > 'imasig_optional' does not only mean that the signature is optional, but
>> > also implies that it has to be a digital signature, not a hash. This
>> > latter part is what IMA_DIGSIG_REQUIRED means.
>> >
>> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
>> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>> >
>> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
>> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>> >
>>
>> It sounds odd that optional signature is actually required.
>> Optional for me means that it may be there or may be not.
>> If it is not there, then it may be hash or nothing.
>>
>> I see it is more logical if it is "appraise_type=optional",
>> which means that we might have no xattr value, hash or signature.
>> It if happens to be a signature, then IMA_DIGSIG flag will be set.
>
> Right, 'appraise_type=' could have been defined either as a comma
> separated list of options (eg. appraise_type=imassig,optional) or, as
> Vivek implemented, a new option. Eventually, we will need to extend
> 'appraise_type=' (eg. required algorithm), but for now I don't have a
> problem with the new option.
>
It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.
Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...
If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.
- Dmitry
> thanks,
>
> Mimi
>
>> I asked Vivek to send a policy file he uses in his system.
>> I would like to see how system configured as a whole...
>>
>> - Dmitry
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote: > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal wrote: > > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > >> It should not be the only line in the policy. > >> Can you share full policy? > > > > I verified by putting some printk. There is only single rule in > > ima_policy_rules list after I have updated the rules through "policy" > > file. > > > > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > > > /sys/kernel/security/policy > > There is a default policy which has several rules. > > And when you do your "echo" you will replace all rules with that single rule. > But ok, you have one rule only and it is fine to have even a single rule... Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote: [..] > > I see it is more logical if it is "appraise_type=optional", > > which means that we might have no xattr value, hash or signature. > > It if happens to be a signature, then IMA_DIGSIG flag will be set. > > Right, 'appraise_type=' could have been defined either as a comma > separated list of options (eg. appraise_type=imassig,optional) or, as > Vivek implemented, a new option. Eventually, we will need to extend > 'appraise_type=' (eg. required algorithm), but for now I don't have a > problem with the new option. Ok, I will cleanup the code to do above. Just wanted to clear up one point. Above option will not have any effect on evm behavior? This only impacts IMA appraisal behavior. For example, if security.ima is not present it is fine and file access is allowed. But if EVM is enabled and initialized and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns INTEGRITY_NOXATTRS, file access should still be denied? BTW, what's the difference between INTEGRITY_NOLABEL and INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()). If yes, we probbaly need to differentiate between IMA/EVM nolabel. Say, INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL. appraise_type=optional should allow file access if rc = INTEGRITY_IMA_NOLABEL but deny it when rc = INTEGRITY_EVM_NOLABEL? Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 09:38 -0500, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > > It should not be the only line in the policy. > > Can you share full policy? > > I verified by putting some printk. If anyone is interested in posting a patch to display the current policy, it shouldn't be too difficult to do. > There is only single rule in > ima_policy_rules list after I have updated the rules through "policy" > file. > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > > /sys/kernel/security/policy Right, this replaces one policy with another one. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:29:43PM +0200, Kasatkin, Dmitry wrote: > On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry > wrote: > > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal wrote: > >> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > >>> It should not be the only line in the policy. > >>> Can you share full policy? > >> > >> I verified by putting some printk. There is only single rule in > >> ima_policy_rules list after I have updated the rules through "policy" > >> file. > >> > >> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > > >> /sys/kernel/security/policy > > > > There is a default policy which has several rules. > > > > And when you do your "echo" you will replace all rules with that single > > rule. > > But ok, you have one rule only and it is fine to have even a single rule... > > > > What I have not said yet it is that in your case, because you use only > BPRM_CHECK hook, > you do not need "dont_appraise" and "dont_measure" rules for pseudo > filesystems. Agreed. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry wrote: > On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal wrote: >> On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: >>> It should not be the only line in the policy. >>> Can you share full policy? >> >> I verified by putting some printk. There is only single rule in >> ima_policy_rules list after I have updated the rules through "policy" >> file. >> >> echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > >> /sys/kernel/security/policy > > There is a default policy which has several rules. > > And when you do your "echo" you will replace all rules with that single rule. > But ok, you have one rule only and it is fine to have even a single rule... > What I have not said yet it is that in your case, because you use only BPRM_CHECK hook, you do not need "dont_appraise" and "dont_measure" rules for pseudo filesystems. >> >> Thanks >> Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal wrote: > On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: >> It should not be the only line in the policy. >> Can you share full policy? > > I verified by putting some printk. There is only single rule in > ima_policy_rules list after I have updated the rules through "policy" > file. > > echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > > /sys/kernel/security/policy There is a default policy which has several rules. And when you do your "echo" you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... > > Thanks > Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > It should not be the only line in the policy. > Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through "policy" file. echo "appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional" > /sys/kernel/security/policy Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 15:36 +0200, Kasatkin, Dmitry wrote: > It should not be the only line in the policy. > Can you share full policy? > On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal wrote: > > > > appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional Different use cases require different policies. Our concern is that appraising file integrity not be added to the kernel in an ad-hoc manner. This rule implements the intent of the original patches Vivek posted. We might personally disagree with the intent, but that is a totally separate discussion - one that should have been raised when he posted the original patches. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: > It should not be the only line in the policy. So a single rule is not allowed or kernel has imposed more rules internally. > Can you share full policy? How do I get to full policy. Is there an interface I can read it from? Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
> On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar wrote:
> > On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> >> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
> >
> >> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> >> > integrity_iint_cache *iint,
> >> > }
> >> > switch (xattr_value->type) {
> >> > case IMA_XATTR_DIGEST:
> >> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> >> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> >> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> >> > cause = "IMA signature required";
> >> > status = INTEGRITY_FAIL;
> >> > break;
> >>
> >> This looks a bit odd... If "optional" signature is missing - we fail..
> >> It is optional... Why we should fail?
> >
> > 'imasig_optional' does not only mean that the signature is optional, but
> > also implies that it has to be a digital signature, not a hash. This
> > latter part is what IMA_DIGSIG_REQUIRED means.
> >
> > If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> > 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
> >
> > IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> > IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
> >
>
> It sounds odd that optional signature is actually required.
> Optional for me means that it may be there or may be not.
> If it is not there, then it may be hash or nothing.
>
> I see it is more logical if it is "appraise_type=optional",
> which means that we might have no xattr value, hash or signature.
> It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
thanks,
Mimi
> I asked Vivek to send a policy file he uses in his system.
> I would like to see how system configured as a whole...
>
> - Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
It should not be the only line in the policy.
Can you share full policy?
Thanks,
Dmitry
On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal wrote:
> On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
>> Hello Vivek,
>>
>> Can you please send to us how your IMA policy looks like.
>
> Hi Dmitry,
>
> For testing purposes, I am using following.
>
> appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional
>
> I set this using /sys/kernel/security/policy interface after boot.
>
> Thanks
> Vivek
>
>>
>> Thanks,
>> Dmitry
>>
>> On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal wrote:
>> > On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
>> >> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>> >>
>> >> [..]
>> >> > > > > --- a/security/integrity/ima/ima_appraise.c
>> >> > > > > +++ b/security/integrity/ima/ima_appraise.c
>> >> > > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func,
>> >> > > > > struct integrity_iint_cache *iint,
>> >> > > > > enum integrity_status status = INTEGRITY_UNKNOWN;
>> >> > > > > const char *op = "appraise_data";
>> >> > > > > char *cause = "unknown";
>> >> > > > > - int rc;
>> >> > > > > + int rc, audit_info = 0;
>> >> > > > >
>> >> > > > > if (!ima_appraise)
>> >> > > > > return 0;
>> >> > > > > - if (!inode->i_op->getxattr)
>> >> > > > > + if (!inode->i_op->getxattr) {
>> >> > > > > + /* getxattr not supported. file couldn't have been
>> >> > > > > signed */
>> >> > > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
>> >> > > > > + return INTEGRITY_PASS;
>> >> > > > > return INTEGRITY_UNKNOWN;
>> >> > > > > + }
>> >> > > > >
>> >> > > >
>> >> > > > Please don't change the result of the appraisal like this. A single
>> >> > > > change can be made towards the bottom of process_measurement().
>> >> > >
>> >> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
>> >> > > I can probably maintain a bool variable, say pass_appraisal, and set
>> >> > > that here and at the end of function, parse that variable and change
>> >> > > the status accordingly.
>> >> >
>> >> > process_measurement() is the only caller of ima_appraise_measurement().
>> >> > Leave the results of ima_appraise_measurement() alone. There's already
>> >> > code at the end of process_measurement() which decides what to return.
>> >> > Just modify it based on the appraisal results.
>> >>
>> >
>> > If we do this, audit logs will be filled with integrity unknown failures.
>> > As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
>> > and an audit message will be logged.
>> >
>> > Thanks
>> > Vivek
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe
>> > linux-security-module" in
>> > the body of a message to majord...@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
> Hello Vivek,
>
> Can you please send to us how your IMA policy looks like.
Hi Dmitry,
For testing purposes, I am using following.
appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional
I set this using /sys/kernel/security/policy interface after boot.
Thanks
Vivek
>
> Thanks,
> Dmitry
>
> On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal wrote:
> > On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
> >> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
> >>
> >> [..]
> >> > > > > --- a/security/integrity/ima/ima_appraise.c
> >> > > > > +++ b/security/integrity/ima/ima_appraise.c
> >> > > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func,
> >> > > > > struct integrity_iint_cache *iint,
> >> > > > > enum integrity_status status = INTEGRITY_UNKNOWN;
> >> > > > > const char *op = "appraise_data";
> >> > > > > char *cause = "unknown";
> >> > > > > - int rc;
> >> > > > > + int rc, audit_info = 0;
> >> > > > >
> >> > > > > if (!ima_appraise)
> >> > > > > return 0;
> >> > > > > - if (!inode->i_op->getxattr)
> >> > > > > + if (!inode->i_op->getxattr) {
> >> > > > > + /* getxattr not supported. file couldn't have been
> >> > > > > signed */
> >> > > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> >> > > > > + return INTEGRITY_PASS;
> >> > > > > return INTEGRITY_UNKNOWN;
> >> > > > > + }
> >> > > > >
> >> > > >
> >> > > > Please don't change the result of the appraisal like this. A single
> >> > > > change can be made towards the bottom of process_measurement().
> >> > >
> >> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> >> > > I can probably maintain a bool variable, say pass_appraisal, and set
> >> > > that here and at the end of function, parse that variable and change
> >> > > the status accordingly.
> >> >
> >> > process_measurement() is the only caller of ima_appraise_measurement().
> >> > Leave the results of ima_appraise_measurement() alone. There's already
> >> > code at the end of process_measurement() which decides what to return.
> >> > Just modify it based on the appraisal results.
> >>
> >
> > If we do this, audit logs will be filled with integrity unknown failures.
> > As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
> > and an audit message will be logged.
> >
> > Thanks
> > Vivek
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-security-module" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar wrote:
> On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
>> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
>
>> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
>> > integrity_iint_cache *iint,
>> > }
>> > switch (xattr_value->type) {
>> > case IMA_XATTR_DIGEST:
>> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
>> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
>> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
>> > cause = "IMA signature required";
>> > status = INTEGRITY_FAIL;
>> > break;
>>
>> This looks a bit odd... If "optional" signature is missing - we fail..
>> It is optional... Why we should fail?
>
> 'imasig_optional' does not only mean that the signature is optional, but
> also implies that it has to be a digital signature, not a hash. This
> latter part is what IMA_DIGSIG_REQUIRED means.
>
> If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
> 'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
>
> IMA_DIGSIG_REQUIRED would enforce that it is a signature.
> IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
>
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is "appraise_type=optional",
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
I asked Vivek to send a policy file he uses in his system.
I would like to see how system configured as a whole...
- Dmitry
> thanks,
>
> Mimi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
> On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> > integrity_iint_cache *iint,
> > }
> > switch (xattr_value->type) {
> > case IMA_XATTR_DIGEST:
> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> > cause = "IMA signature required";
> > status = INTEGRITY_FAIL;
> > break;
>
> This looks a bit odd... If "optional" signature is missing - we fail..
> It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
thanks,
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal wrote:
> appraise_type=imasig_optional will allow appraisal to pass even if no
> signatures are present on the file. If signatures are present, then it
> has to be valid digital signature, otherwise appraisal will fail.
>
> This can allow to selectively sign executables in the system and based
> on appraisal results, signed executables with valid signatures can be
> given extra capability to perform priviliged operations in secureboot
> mode.
>
> Signed-off-by: Vivek Goyal
> ---
> Documentation/ABI/testing/ima_policy |2 +-
> security/integrity/ima/ima_appraise.c | 24 +++-
> security/integrity/ima/ima_policy.c |2 ++
> security/integrity/integrity.h|1 +
> 4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index de16de3..5ca0c23 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -30,7 +30,7 @@ Description:
> uid:= decimal value
> fowner:=decimal value
> lsm:are LSM specific
> - option: appraise_type:= [imasig]
> + option: appraise_type:= [imasig] | [imasig_optional]
>
> default policy:
> # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 3710f44..222ade0 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> integrity_iint_cache *iint,
> enum integrity_status status = INTEGRITY_UNKNOWN;
> const char *op = "appraise_data";
> char *cause = "unknown";
> - int rc;
> + int rc, audit_info = 0;
>
> if (!ima_appraise)
> return 0;
> - if (!inode->i_op->getxattr)
> + if (!inode->i_op->getxattr) {
> + /* getxattr not supported. file couldn't have been signed */
> + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> + return INTEGRITY_PASS;
> return INTEGRITY_UNKNOWN;
> + }
>
> rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)_value,
> 0, GFP_NOFS);
> if (rc <= 0) {
> /* File system does not support security xattr */
> - if (rc == -EOPNOTSUPP)
> + if (rc == -EOPNOTSUPP) {
> + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> + return INTEGRITY_PASS;
> return INTEGRITY_UNKNOWN;
> + }
>
> if (rc && rc != -ENODATA)
> goto out;
> @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> integrity_iint_cache *iint,
> }
> switch (xattr_value->type) {
> case IMA_XATTR_DIGEST:
> - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> + iint->flags & IMA_DIGSIG_OPTIONAL) {
> cause = "IMA signature required";
> status = INTEGRITY_FAIL;
> break;
This looks a bit odd... If "optional" signature is missing - we fail..
It is optional... Why we should fail?
Mimi?
> @@ -201,8 +209,14 @@ out:
> if (!ima_fix_xattr(dentry, iint))
> status = INTEGRITY_PASS;
> }
> + if (status == INTEGRITY_NOLABEL &&
> + iint->flags & IMA_DIGSIG_OPTIONAL) {
> + status = INTEGRITY_PASS;
> + /* Don't flood audit logs with skipped appraise */
> + audit_info = 1;
> + }
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> - op, cause, rc, 0);
> + op, cause, rc, audit_info);
> } else {
> ima_cache_flags(iint, func);
> }
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 4adcd0f..8b8cd5f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> ima_log_string(ab, "appraise_type", args[0].from);
> if ((strcmp(args[0].from, "imasig")) == 0)
> entry->flags |= IMA_DIGSIG_REQUIRED;
> + else if ((strcmp(args[0].from, "imasig_optional")) ==
> 0)
> + entry->flags |= IMA_DIGSIG_OPTIONAL;
> else
> result =
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
Hello Vivek,
Can you please send to us how your IMA policy looks like.
Thanks,
Dmitry
On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
>> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>>
>> [..]
>> > > > > --- a/security/integrity/ima/ima_appraise.c
>> > > > > +++ b/security/integrity/ima/ima_appraise.c
>> > > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
>> > > > > integrity_iint_cache *iint,
>> > > > > enum integrity_status status = INTEGRITY_UNKNOWN;
>> > > > > const char *op = "appraise_data";
>> > > > > char *cause = "unknown";
>> > > > > - int rc;
>> > > > > + int rc, audit_info = 0;
>> > > > >
>> > > > > if (!ima_appraise)
>> > > > > return 0;
>> > > > > - if (!inode->i_op->getxattr)
>> > > > > + if (!inode->i_op->getxattr) {
>> > > > > + /* getxattr not supported. file couldn't have been
>> > > > > signed */
>> > > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
>> > > > > + return INTEGRITY_PASS;
>> > > > > return INTEGRITY_UNKNOWN;
>> > > > > + }
>> > > > >
>> > > >
>> > > > Please don't change the result of the appraisal like this. A single
>> > > > change can be made towards the bottom of process_measurement().
>> > >
>> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
>> > > I can probably maintain a bool variable, say pass_appraisal, and set
>> > > that here and at the end of function, parse that variable and change
>> > > the status accordingly.
>> >
>> > process_measurement() is the only caller of ima_appraise_measurement().
>> > Leave the results of ima_appraise_measurement() alone. There's already
>> > code at the end of process_measurement() which decides what to return.
>> > Just modify it based on the appraisal results.
>>
>
> If we do this, audit logs will be filled with integrity unknown failures.
> As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
> and an audit message will be logged.
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
Hello Vivek,
Can you please send to us how your IMA policy looks like.
Thanks,
Dmitry
On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal vgo...@redhat.com wrote:
On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe
linux-security-module in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal vgo...@redhat.com
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value,
0, GFP_NOFS);
if (rc = 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
if (rc rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
Mimi?
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL
+ iint-flags IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
ima_log_string(ab, appraise_type, args[0].from);
if ((strcmp(args[0].from, imasig)) == 0)
entry-flags |= IMA_DIGSIG_REQUIRED;
+ else if ((strcmp(args[0].from, imasig_optional)) ==
0)
+ entry-flags |= IMA_DIGSIG_OPTIONAL;
else
result = -EINVAL;
break;
diff --git a/security/integrity/integrity.h
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
thanks,
Mimi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
I asked Vivek to send a policy file he uses in his system.
I would like to see how system configured as a whole...
- Dmitry
thanks,
Mimi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
Hello Vivek,
Can you please send to us how your IMA policy looks like.
Hi Dmitry,
For testing purposes, I am using following.
appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional
I set this using /sys/kernel/security/policy interface after boot.
Thanks
Vivek
Thanks,
Dmitry
On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal vgo...@redhat.com wrote:
On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func,
struct integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe
linux-security-module in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
It should not be the only line in the policy.
Can you share full policy?
Thanks,
Dmitry
On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal vgo...@redhat.com wrote:
On Wed, Feb 13, 2013 at 02:14:55PM +0200, Kasatkin, Dmitry wrote:
Hello Vivek,
Can you please send to us how your IMA policy looks like.
Hi Dmitry,
For testing purposes, I am using following.
appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional
I set this using /sys/kernel/security/policy interface after boot.
Thanks
Vivek
Thanks,
Dmitry
On Tue, Feb 12, 2013 at 8:57 PM, Vivek Goyal vgo...@redhat.com wrote:
On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func,
struct integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe
linux-security-module in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
thanks,
Mimi
I asked Vivek to send a policy file he uses in his system.
I would like to see how system configured as a whole...
- Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. So a single rule is not allowed or kernel has imposed more rules internally. Can you share full policy? How do I get to full policy. Is there an interface I can read it from? Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 15:36 +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? On Wed, Feb 13, 2013 at 3:29 PM, Vivek Goyal vgo...@redhat.com wrote: appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional Different use cases require different policies. Our concern is that appraising file integrity not be added to the kernel in an ad-hoc manner. This rule implements the intent of the original patches Vivek posted. We might personally disagree with the intent, but that is a totally separate discussion - one that should have been raised when he posted the original patches. thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal vgo...@redhat.com wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy There is a default policy which has several rules. And when you do your echo you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry dmitry.kasat...@intel.com wrote: On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal vgo...@redhat.com wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy There is a default policy which has several rules. And when you do your echo you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... What I have not said yet it is that in your case, because you use only BPRM_CHECK hook, you do not need dont_appraise and dont_measure rules for pseudo filesystems. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:29:43PM +0200, Kasatkin, Dmitry wrote: On Wed, Feb 13, 2013 at 5:26 PM, Kasatkin, Dmitry dmitry.kasat...@intel.com wrote: On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal vgo...@redhat.com wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy There is a default policy which has several rules. And when you do your echo you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... What I have not said yet it is that in your case, because you use only BPRM_CHECK hook, you do not need dont_appraise and dont_measure rules for pseudo filesystems. Agreed. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 09:38 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. If anyone is interested in posting a patch to display the current policy, it shouldn't be too difficult to do. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy Right, this replaces one policy with another one. thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 08:44:04AM -0500, Mimi Zohar wrote: [..] I see it is more logical if it is appraise_type=optional, which means that we might have no xattr value, hash or signature. It if happens to be a signature, then IMA_DIGSIG flag will be set. Right, 'appraise_type=' could have been defined either as a comma separated list of options (eg. appraise_type=imassig,optional) or, as Vivek implemented, a new option. Eventually, we will need to extend 'appraise_type=' (eg. required algorithm), but for now I don't have a problem with the new option. Ok, I will cleanup the code to do above. Just wanted to clear up one point. Above option will not have any effect on evm behavior? This only impacts IMA appraisal behavior. For example, if security.ima is not present it is fine and file access is allowed. But if EVM is enabled and initialized and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns INTEGRITY_NOXATTRS, file access should still be denied? BTW, what's the difference between INTEGRITY_NOLABEL and INTEGRITY_NOXATTRS (as returned by evm_verifyxattr()). If yes, we probbaly need to differentiate between IMA/EVM nolabel. Say, INTEGRITY_IMA_NOLABEL and INTEGRITY_EVM_NOLABEL. appraise_type=optional should allow file access if rc = INTEGRITY_IMA_NOLABEL but deny it when rc = INTEGRITY_EVM_NOLABEL? Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote: On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal vgo...@redhat.com wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy There is a default policy which has several rules. And when you do your echo you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.
Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...
If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.
- Dmitry
thanks,
Mimi
I asked Vivek to send a policy file he uses in his system.
I would like to see how system configured as a whole...
- Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com
wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.
Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...
If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.
Hi Dmitry,
While we are at it, I thought I will mention what else is going in my
mind w.r.t next step.
I looked at your patch. That patch looks at IMG_DIGSIG
flag and sets bprm-unsafe with appropriate flag. It works well if
signature verification before loading executable alone is sufficient.
But this leaves a small window open where somebody could write to the
disk block directly (bypass filesystem) after IMA signature verification.
Then modified image will be loaded by the binary handler.
To avoid that, I think I will need to do appraisal after loading the file
(with VM_LOCKED flag).
I guess I will need to introduce another hook say bprm_post_load() to
verify integrity of file.
So this raises few questions.
- Are two appraisals really necessary. From my use case perspective
initially I just need to know whether digital signature are present
or not and appraisal of file is not required. I guess it is one
optimization area to keep appraisal overhead minimum in exec() path.
- When I go for second appraisal after loading file, current IMA code
will have success result in iint-flags. I need to figure a way out
to reset cache result and force reappraisal.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, Feb 13, 2013 at 7:51 PM, Vivek Goyal vgo...@redhat.com wrote:
On Wed, Feb 13, 2013 at 07:33:13PM +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com
wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com
wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.
Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...
If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.
Hi Dmitry,
While we are at it, I thought I will mention what else is going in my
mind w.r.t next step.
I looked at your patch. That patch looks at IMG_DIGSIG
flag and sets bprm-unsafe with appropriate flag. It works well if
signature verification before loading executable alone is sufficient.
But this leaves a small window open where somebody could write to the
disk block directly (bypass filesystem) after IMA signature verification.
Then modified image will be loaded by the binary handler.
To avoid that, I think I will need to do appraisal after loading the file
(with VM_LOCKED flag).
I guess I will need to introduce another hook say bprm_post_load() to
verify integrity of file.
So this raises few questions.
- Are two appraisals really necessary. From my use case perspective
initially I just need to know whether digital signature are present
or not and appraisal of file is not required. I guess it is one
optimization area to keep appraisal overhead minimum in exec() path.
- When I go for second appraisal after loading file, current IMA code
will have success result in iint-flags. I need to figure a way out
to reset cache result and force reappraisal.
Thanks
Vivek
Hello,
My patch was just a 5 minute example how to go with that.
Sure, when we deal with MAP_LOCKED, we might do appraisal after
loading the binary.
Yes, it looks like that we really need another hook for that...
But then process_measurement might do things a bit differently.
It might check the policy, allocated iint and return if locking is
needed (or another hook will be called).
Next hook will just collect and appriase...
- Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 19:33 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 3:44 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
On Wed, 2013-02-13 at 15:13 +0200, Kasatkin, Dmitry wrote:
On Wed, Feb 13, 2013 at 2:56 PM, Mimi Zohar zo...@linux.vnet.ibm.com
wrote:
On Wed, 2013-02-13 at 14:31 +0200, Kasatkin, Dmitry wrote:
On Mon, Feb 11, 2013 at 10:11 PM, Vivek Goyal vgo...@redhat.com wrote:
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
This looks a bit odd... If optional signature is missing - we fail..
It is optional... Why we should fail?
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
If 'imasig_optional' set both 'IMA_DIGSIG_OPTIONAL' and
'IMA_DIGSIG_REQUIRED', then this change wouldn't be necessary.
IMA_DIGSIG_REQUIRED would enforce that it is a signature.
IMA_DIGSIG_OPTIONAL would fail only for files with invalid signatures.
It sounds odd that optional signature is actually required.
Optional for me means that it may be there or may be not.
If it is not there, then it may be hash or nothing.
I see it is more logical if it is appraise_type=optional,
which means that we might have no xattr value, hash or signature.
It if happens to be a signature, then IMA_DIGSIG flag will be set.
Right, 'appraise_type=' could have been defined either as a comma
separated list of options (eg. appraise_type=imassig,optional) or, as
Vivek implemented, a new option. Eventually, we will need to extend
'appraise_type=' (eg. required algorithm), but for now I don't have a
problem with the new option.
It is not exactly what I meant. IOW, I do not want
appraise_type=imasig,optional.
Optional for me is that xattr value is optional. It might be nothing,
hash or imasig...
To summarize:
Option 1: appraise_type:= [[imasig] | [imahash]] [optional]
sample rules:
# require either hash or signature
func=bprm
# require signature
func=bprm appraise_type=imasig
# permit nothing, hash, or signature
func=bprm appraise_type=optional
# permit nothing or hash
func=bprm appraise_type=imahash, optional
# permit nothing or signature
func=bprm appraise_type=imasig, optional
Option 2: appraise_type:= [imasig] | [imasig_optional]
As I don't see a use case for either just 'optional' or 'imahash,
optional', this leaves 'imasig,optional', which could be expressed as
'imasig_optional'.
Option 3: appraise_type:= [imasig] | [imahash] | [optional]
Dmitry is recommending this syntax, as IMA_DIGSIG will be set in the
iint flags.
Any of these options should work.
If it would happen that it contains signature, then IMA_DIGSIG flag
would be set,
and process could get needed capability as Vivek wants.
With the 'optional' condition, both unsigned and validly signed files
will succeed. One way of making this information accessible to an LSM,
would be to define a new integrity capability and set it here. The new
integrity capability would indicate the file was validly signed.
thanks,
Mimi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Wed, 2013-02-13 at 10:30 -0500, Vivek Goyal wrote: On Wed, Feb 13, 2013 at 05:26:27PM +0200, Kasatkin, Dmitry wrote: On Wed, Feb 13, 2013 at 4:38 PM, Vivek Goyal vgo...@redhat.com wrote: On Wed, Feb 13, 2013 at 03:36:45PM +0200, Kasatkin, Dmitry wrote: It should not be the only line in the policy. Can you share full policy? I verified by putting some printk. There is only single rule in ima_policy_rules list after I have updated the rules through policy file. echo appraise fowner=0 func=BPRM_CHECK appraise_type=imasig_optional /sys/kernel/security/policy There is a default policy which has several rules. And when you do your echo you will replace all rules with that single rule. But ok, you have one rule only and it is fine to have even a single rule... Yep, I got that. Default policy gets overruled when a new policy is loaded. In secureboot mode, somehow above rule needs to take effect by default. One option would be that kernel can enforce above rule. (I guess by adding it to both default_list as well as policy list). The default policy is empty, but can be replaced with boot command line options. The existing options are ima_tcb and/ ima_appraise_tcb. Please feel free to define an additional policy. thanks, Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, 2013-02-12 at 13:52 -0500, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>
> [..]
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> > > > > integrity_iint_cache *iint,
> > > > > enum integrity_status status = INTEGRITY_UNKNOWN;
> > > > > const char *op = "appraise_data";
> > > > > char *cause = "unknown";
> > > > > - int rc;
> > > > > + int rc, audit_info = 0;
> > > > >
> > > > > if (!ima_appraise)
> > > > > return 0;
> > > > > - if (!inode->i_op->getxattr)
> > > > > + if (!inode->i_op->getxattr) {
> > > > > + /* getxattr not supported. file couldn't have been
> > > > > signed */
> > > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > > + return INTEGRITY_PASS;
> > > > > return INTEGRITY_UNKNOWN;
> > > > > + }
> > > > >
> > > >
> > > > Please don't change the result of the appraisal like this. A single
> > > > change can be made towards the bottom of process_measurement().
> > >
> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > > I can probably maintain a bool variable, say pass_appraisal, and set
> > > that here and at the end of function, parse that variable and change
> > > the status accordingly.
> >
> > process_measurement() is the only caller of ima_appraise_measurement().
> > Leave the results of ima_appraise_measurement() alone. There's already
> > code at the end of process_measurement() which decides what to return.
> > Just modify it based on the appraisal results.
>
> Ok, I can do that. There is a small concern though. That is what to do
> when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
>
> ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
> does not support xattrs or if security xattr is not enabled. In this
> case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
> set.
Right, 'INTEGRITY_UNKNOWN' means that we can't reason, for whatever
reason, about the integrity of the file.
> But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
> fails and returns -EOPNOTSUPP.
In this case, it is Kconfig based.
> I feel that in this case it is not very appropriate to pass appraisal and
> let executable run. If digital signatures are present but we can't verify
> those (Say some algorithm is not supported in kernel). In that case I
> think it makes sense to fail the signature.
>
>rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> xattr_value->digest, rc - 1,
> iint->ima_xattr.digest,
> IMA_DIGEST_SIZE);
> if (rc == -EOPNOTSUPP) {
> status = INTEGRITY_UNKNOWN;
>
>
> So how to handle this case.
>
> I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
> INTEGRITY_FAIL.
We still can't reason about the integrity of the file. For all we know,
it could be a validly signed file, just verification wasn't enabled.
> Will it make sense to fail signature in case of -EOPNOTSUPP.
>rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> xattr_value->digest, rc - 1,
> iint->ima_xattr.digest,
> IMA_DIGEST_SIZE);
> if (rc)
> status = INTEGRITY_FAIL;
> else
> status = INTEGRITY_PASS;
>
>
Please don't.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
>
> [..]
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> > > > > integrity_iint_cache *iint,
> > > > > enum integrity_status status = INTEGRITY_UNKNOWN;
> > > > > const char *op = "appraise_data";
> > > > > char *cause = "unknown";
> > > > > - int rc;
> > > > > + int rc, audit_info = 0;
> > > > >
> > > > > if (!ima_appraise)
> > > > > return 0;
> > > > > - if (!inode->i_op->getxattr)
> > > > > + if (!inode->i_op->getxattr) {
> > > > > + /* getxattr not supported. file couldn't have been
> > > > > signed */
> > > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > > + return INTEGRITY_PASS;
> > > > > return INTEGRITY_UNKNOWN;
> > > > > + }
> > > > >
> > > >
> > > > Please don't change the result of the appraisal like this. A single
> > > > change can be made towards the bottom of process_measurement().
> > >
> > > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > > I can probably maintain a bool variable, say pass_appraisal, and set
> > > that here and at the end of function, parse that variable and change
> > > the status accordingly.
> >
> > process_measurement() is the only caller of ima_appraise_measurement().
> > Leave the results of ima_appraise_measurement() alone. There's already
> > code at the end of process_measurement() which decides what to return.
> > Just modify it based on the appraisal results.
>
If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> > > > integrity_iint_cache *iint,
> > > > enum integrity_status status = INTEGRITY_UNKNOWN;
> > > > const char *op = "appraise_data";
> > > > char *cause = "unknown";
> > > > - int rc;
> > > > + int rc, audit_info = 0;
> > > >
> > > > if (!ima_appraise)
> > > > return 0;
> > > > - if (!inode->i_op->getxattr)
> > > > + if (!inode->i_op->getxattr) {
> > > > + /* getxattr not supported. file couldn't have been
> > > > signed */
> > > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > > + return INTEGRITY_PASS;
> > > > return INTEGRITY_UNKNOWN;
> > > > + }
> > > >
> > >
> > > Please don't change the result of the appraisal like this. A single
> > > change can be made towards the bottom of process_measurement().
> >
> > I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> > I can probably maintain a bool variable, say pass_appraisal, and set
> > that here and at the end of function, parse that variable and change
> > the status accordingly.
>
> process_measurement() is the only caller of ima_appraise_measurement().
> Leave the results of ima_appraise_measurement() alone. There's already
> code at the end of process_measurement() which decides what to return.
> Just modify it based on the appraisal results.
Ok, I can do that. There is a small concern though. That is what to do
when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
does not support xattrs or if security xattr is not enabled. In this
case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
set.
But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
fails and returns -EOPNOTSUPP.
I feel that in this case it is not very appropriate to pass appraisal and
let executable run. If digital signatures are present but we can't verify
those (Say some algorithm is not supported in kernel). In that case I
think it makes sense to fail the signature.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value->digest, rc - 1,
iint->ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
So how to handle this case.
I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
INTEGRITY_FAIL.
Will it make sense to fail signature in case of -EOPNOTSUPP.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value->digest, rc - 1,
iint->ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc)
status = INTEGRITY_FAIL;
else
status = INTEGRITY_PASS;
[..]
> > > > --- a/security/integrity/ima/ima_policy.c
> > > > +++ b/security/integrity/ima/ima_policy.c
> > > > @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
> > > > ima_rule_entry *entry)
> > > > ima_log_string(ab, "appraise_type",
> > > > args[0].from);
> > > > if ((strcmp(args[0].from, "imasig")) == 0)
> > > > entry->flags |= IMA_DIGSIG_REQUIRED;
> > > > + else if ((strcmp(args[0].from,
> > > > "imasig_optional")) == 0)
> > > > + entry->flags |= IMA_DIGSIG_OPTIONAL;
> > >
> > > By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
> > > up the code a bit more.
> >
> > I don't understand this part. So imasig_optional sets both
> > IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
> > quite contradictory for a reader.
>
> > We only add one extra line and that is when "hash" is detected in
> > security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
> > we are probably not saving on code.
> >
> > IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.
>
> 'imasig_optional' does not only mean that the signature is optional, but
> also implies that it has to be a digital signature, not a hash. This
> latter part is what IMA_DIGSIG_REQUIRED means.
>
> Remember the rule 'action' determines whether or not the file needs to
> be appraised.
Ok, I will change it and set IMA_DIGSIG_REQUIRED flag too.
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, 2013-02-12 at 09:26 -0500, Vivek Goyal wrote:
> On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
> > On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > > appraise_type=imasig_optional will allow appraisal to pass even if no
> > > signatures are present on the file. If signatures are present, then it
> > > has to be valid digital signature, otherwise appraisal will fail.
> > >
> > > This can allow to selectively sign executables in the system and based
> > > on appraisal results, signed executables with valid signatures can be
> > > given extra capability to perform priviliged operations in secureboot
> > > mode.
> > >
> > > Signed-off-by: Vivek Goyal
> >
> > Thanks, Vivek, the patch looks a lot better. Here are a couple of
> > suggestions:
> > - the patch description needs to start with the problem description, not
> > the solution.
>
> Sure will do.
>
> > - the patch name should reflect the problem.
>
> Will change.
>
> >
> > A few comments are inline below.
> >
> > thanks,
> >
> > Mimi
> >
> > > ---
> > > Documentation/ABI/testing/ima_policy |2 +-
> > > security/integrity/ima/ima_appraise.c | 24 +++-
> > > security/integrity/ima/ima_policy.c |2 ++
> > > security/integrity/integrity.h|1 +
> > > 4 files changed, 23 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/ima_policy
> > > b/Documentation/ABI/testing/ima_policy
> > > index de16de3..5ca0c23 100644
> > > --- a/Documentation/ABI/testing/ima_policy
> > > +++ b/Documentation/ABI/testing/ima_policy
> > > @@ -30,7 +30,7 @@ Description:
> > > uid:= decimal value
> > > fowner:=decimal value
> > > lsm:are LSM specific
> > > - option: appraise_type:= [imasig]
> > > + option: appraise_type:= [imasig] | [imasig_optional]
> > >
> > > default policy:
> > > # PROC_SUPER_MAGIC
> > > diff --git a/security/integrity/ima/ima_appraise.c
> > > b/security/integrity/ima/ima_appraise.c
> > > index 3710f44..222ade0 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> > > integrity_iint_cache *iint,
> > > enum integrity_status status = INTEGRITY_UNKNOWN;
> > > const char *op = "appraise_data";
> > > char *cause = "unknown";
> > > - int rc;
> > > + int rc, audit_info = 0;
> > >
> > > if (!ima_appraise)
> > > return 0;
> > > - if (!inode->i_op->getxattr)
> > > + if (!inode->i_op->getxattr) {
> > > + /* getxattr not supported. file couldn't have been signed */
> > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > + return INTEGRITY_PASS;
> > > return INTEGRITY_UNKNOWN;
> > > + }
> > >
> >
> > Please don't change the result of the appraisal like this. A single
> > change can be made towards the bottom of process_measurement().
>
> I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> I can probably maintain a bool variable, say pass_appraisal, and set
> that here and at the end of function, parse that variable and change
> the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
> >
> > > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)_value,
> > > 0, GFP_NOFS);
> > > if (rc <= 0) {
> > > /* File system does not support security xattr */
> > > - if (rc == -EOPNOTSUPP)
> > > + if (rc == -EOPNOTSUPP) {
> > > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > + return INTEGRITY_PASS;
> > > return INTEGRITY_UNKNOWN;
> > > + }
> >
> > ditto
>
> Will do.
>
> >
> > >
> > > if (rc && rc != -ENODATA)
> > > goto out;
> > > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> > > integrity_iint_cache *iint,
> > > }
> > > switch (xattr_value->type) {
> > > case IMA_XATTR_DIGEST:
> > > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> > > cause = "IMA signature required";
> > > status = INTEGRITY_FAIL;
> > > break;
> > > @@ -201,8 +209,14 @@ out:
> > > if (!ima_fix_xattr(dentry, iint))
> > > status = INTEGRITY_PASS;
> > > }
> > > + if (status == INTEGRITY_NOLABEL &&
> > > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> > > + status = INTEGRITY_PASS;
> > > + /* Don't flood audit logs with
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
> On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > appraise_type=imasig_optional will allow appraisal to pass even if no
> > signatures are present on the file. If signatures are present, then it
> > has to be valid digital signature, otherwise appraisal will fail.
> >
> > This can allow to selectively sign executables in the system and based
> > on appraisal results, signed executables with valid signatures can be
> > given extra capability to perform priviliged operations in secureboot
> > mode.
> >
> > Signed-off-by: Vivek Goyal
>
> Thanks, Vivek, the patch looks a lot better. Here are a couple of
> suggestions:
> - the patch description needs to start with the problem description, not
> the solution.
Sure will do.
> - the patch name should reflect the problem.
Will change.
>
> A few comments are inline below.
>
> thanks,
>
> Mimi
>
> > ---
> > Documentation/ABI/testing/ima_policy |2 +-
> > security/integrity/ima/ima_appraise.c | 24 +++-
> > security/integrity/ima/ima_policy.c |2 ++
> > security/integrity/integrity.h|1 +
> > 4 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/ima_policy
> > b/Documentation/ABI/testing/ima_policy
> > index de16de3..5ca0c23 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -30,7 +30,7 @@ Description:
> > uid:= decimal value
> > fowner:=decimal value
> > lsm:are LSM specific
> > - option: appraise_type:= [imasig]
> > + option: appraise_type:= [imasig] | [imasig_optional]
> >
> > default policy:
> > # PROC_SUPER_MAGIC
> > diff --git a/security/integrity/ima/ima_appraise.c
> > b/security/integrity/ima/ima_appraise.c
> > index 3710f44..222ade0 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> > integrity_iint_cache *iint,
> > enum integrity_status status = INTEGRITY_UNKNOWN;
> > const char *op = "appraise_data";
> > char *cause = "unknown";
> > - int rc;
> > + int rc, audit_info = 0;
> >
> > if (!ima_appraise)
> > return 0;
> > - if (!inode->i_op->getxattr)
> > + if (!inode->i_op->getxattr) {
> > + /* getxattr not supported. file couldn't have been signed */
> > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > + return INTEGRITY_PASS;
> > return INTEGRITY_UNKNOWN;
> > + }
> >
>
> Please don't change the result of the appraisal like this. A single
> change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
>
> > rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)_value,
> > 0, GFP_NOFS);
> > if (rc <= 0) {
> > /* File system does not support security xattr */
> > - if (rc == -EOPNOTSUPP)
> > + if (rc == -EOPNOTSUPP) {
> > + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > + return INTEGRITY_PASS;
> > return INTEGRITY_UNKNOWN;
> > + }
>
> ditto
Will do.
>
> >
> > if (rc && rc != -ENODATA)
> > goto out;
> > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> > integrity_iint_cache *iint,
> > }
> > switch (xattr_value->type) {
> > case IMA_XATTR_DIGEST:
> > - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> > cause = "IMA signature required";
> > status = INTEGRITY_FAIL;
> > break;
> > @@ -201,8 +209,14 @@ out:
> > if (!ima_fix_xattr(dentry, iint))
> > status = INTEGRITY_PASS;
> > }
> > + if (status == INTEGRITY_NOLABEL &&
> > + iint->flags & IMA_DIGSIG_OPTIONAL) {
> > + status = INTEGRITY_PASS;
> > + /* Don't flood audit logs with skipped appraise */
> > + audit_info = 1;
> > + }
> > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > - op, cause, rc, 0);
> > + op, cause, rc, audit_info);
> > } else {
> > ima_cache_flags(iint, func);
> > }
> > diff --git a/security/integrity/ima/ima_policy.c
> > b/security/integrity/ima/ima_policy.c
> > index 4adcd0f..8b8cd5f
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal vgo...@redhat.com
Thanks, Vivek, the patch looks a lot better. Here are a couple of
suggestions:
- the patch description needs to start with the problem description, not
the solution.
Sure will do.
- the patch name should reflect the problem.
Will change.
A few comments are inline below.
thanks,
Mimi
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value,
0, GFP_NOFS);
if (rc = 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
ditto
Will do.
if (rc rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL
+ iint-flags IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, 2013-02-12 at 09:26 -0500, Vivek Goyal wrote:
On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal vgo...@redhat.com
Thanks, Vivek, the patch looks a lot better. Here are a couple of
suggestions:
- the patch description needs to start with the problem description, not
the solution.
Sure will do.
- the patch name should reflect the problem.
Will change.
A few comments are inline below.
thanks,
Mimi
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value,
0, GFP_NOFS);
if (rc = 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
ditto
Will do.
if (rc rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL
+ iint-flags IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
Ok, I can do that. There is a small concern though. That is what to do
when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
does not support xattrs or if security xattr is not enabled. In this
case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
set.
But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
fails and returns -EOPNOTSUPP.
I feel that in this case it is not very appropriate to pass appraisal and
let executable run. If digital signatures are present but we can't verify
those (Say some algorithm is not supported in kernel). In that case I
think it makes sense to fail the signature.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
So how to handle this case.
I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
INTEGRITY_FAIL.
Will it make sense to fail signature in case of -EOPNOTSUPP.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc)
status = INTEGRITY_FAIL;
else
status = INTEGRITY_PASS;
[..]
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
ima_log_string(ab, appraise_type,
args[0].from);
if ((strcmp(args[0].from, imasig)) == 0)
entry-flags |= IMA_DIGSIG_REQUIRED;
+ else if ((strcmp(args[0].from,
imasig_optional)) == 0)
+ entry-flags |= IMA_DIGSIG_OPTIONAL;
By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
up the code a bit more.
I don't understand this part. So imasig_optional sets both
IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
quite contradictory for a reader.
We only add one extra line and that is when hash is detected in
security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
we are probably not saving on code.
IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.
'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash. This
latter part is what IMA_DIGSIG_REQUIRED means.
Remember the rule 'action' determines whether or not the file needs to
be appraised.
Ok, I will change it and set IMA_DIGSIG_REQUIRED flag too.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, Feb 12, 2013 at 01:52:03PM -0500, Vivek Goyal wrote:
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
If we do this, audit logs will be filled with integrity unknown failures.
As each unsigned executable file will fail appraisal with INTEGRITY_UNKNOWN
and an audit message will be logged.
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Tue, 2013-02-12 at 13:52 -0500, Vivek Goyal wrote:
On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
[..]
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been
signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
I can probably maintain a bool variable, say pass_appraisal, and set
that here and at the end of function, parse that variable and change
the status accordingly.
process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone. There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.
Ok, I can do that. There is a small concern though. That is what to do
when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
does not support xattrs or if security xattr is not enabled. In this
case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
set.
Right, 'INTEGRITY_UNKNOWN' means that we can't reason, for whatever
reason, about the integrity of the file.
But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
fails and returns -EOPNOTSUPP.
In this case, it is Kconfig based.
I feel that in this case it is not very appropriate to pass appraisal and
let executable run. If digital signatures are present but we can't verify
those (Say some algorithm is not supported in kernel). In that case I
think it makes sense to fail the signature.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
So how to handle this case.
I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
INTEGRITY_FAIL.
We still can't reason about the integrity of the file. For all we know,
it could be a validly signed file, just verification wasn't enabled.
Will it make sense to fail signature in case of -EOPNOTSUPP.
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
xattr_value-digest, rc - 1,
iint-ima_xattr.digest,
IMA_DIGEST_SIZE);
if (rc)
status = INTEGRITY_FAIL;
else
status = INTEGRITY_PASS;
Please don't.
Mimi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> appraise_type=imasig_optional will allow appraisal to pass even if no
> signatures are present on the file. If signatures are present, then it
> has to be valid digital signature, otherwise appraisal will fail.
>
> This can allow to selectively sign executables in the system and based
> on appraisal results, signed executables with valid signatures can be
> given extra capability to perform priviliged operations in secureboot
> mode.
>
> Signed-off-by: Vivek Goyal
Thanks, Vivek, the patch looks a lot better. Here are a couple of
suggestions:
- the patch description needs to start with the problem description, not
the solution.
- the patch name should reflect the problem.
A few comments are inline below.
thanks,
Mimi
> ---
> Documentation/ABI/testing/ima_policy |2 +-
> security/integrity/ima/ima_appraise.c | 24 +++-
> security/integrity/ima/ima_policy.c |2 ++
> security/integrity/integrity.h|1 +
> 4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index de16de3..5ca0c23 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -30,7 +30,7 @@ Description:
> uid:= decimal value
> fowner:=decimal value
> lsm:are LSM specific
> - option: appraise_type:= [imasig]
> + option: appraise_type:= [imasig] | [imasig_optional]
>
> default policy:
> # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 3710f44..222ade0 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
> integrity_iint_cache *iint,
> enum integrity_status status = INTEGRITY_UNKNOWN;
> const char *op = "appraise_data";
> char *cause = "unknown";
> - int rc;
> + int rc, audit_info = 0;
>
> if (!ima_appraise)
> return 0;
> - if (!inode->i_op->getxattr)
> + if (!inode->i_op->getxattr) {
> + /* getxattr not supported. file couldn't have been signed */
> + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> + return INTEGRITY_PASS;
> return INTEGRITY_UNKNOWN;
> + }
>
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
> rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)_value,
> 0, GFP_NOFS);
> if (rc <= 0) {
> /* File system does not support security xattr */
> - if (rc == -EOPNOTSUPP)
> + if (rc == -EOPNOTSUPP) {
> + if (iint->flags & IMA_DIGSIG_OPTIONAL)
> + return INTEGRITY_PASS;
> return INTEGRITY_UNKNOWN;
> + }
ditto
>
> if (rc && rc != -ENODATA)
> goto out;
> @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
> integrity_iint_cache *iint,
> }
> switch (xattr_value->type) {
> case IMA_XATTR_DIGEST:
> - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> + if (iint->flags & IMA_DIGSIG_REQUIRED ||
> + iint->flags & IMA_DIGSIG_OPTIONAL) {
> cause = "IMA signature required";
> status = INTEGRITY_FAIL;
> break;
> @@ -201,8 +209,14 @@ out:
> if (!ima_fix_xattr(dentry, iint))
> status = INTEGRITY_PASS;
> }
> + if (status == INTEGRITY_NOLABEL &&
> + iint->flags & IMA_DIGSIG_OPTIONAL) {
> + status = INTEGRITY_PASS;
> + /* Don't flood audit logs with skipped appraise */
> + audit_info = 1;
> + }
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> - op, cause, rc, 0);
> + op, cause, rc, audit_info);
> } else {
> ima_cache_flags(iint, func);
> }
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 4adcd0f..8b8cd5f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> ima_log_string(ab, "appraise_type", args[0].from);
> if ((strcmp(args[0].from, "imasig")) == 0)
> entry->flags |= IMA_DIGSIG_REQUIRED;
> + else if ((strcmp(args[0].from,
[PATCH 2/2] ima: Support appraise_type=imasig_optional
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = "appraise_data";
char *cause = "unknown";
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode->i_op->getxattr)
+ if (!inode->i_op->getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint->flags & IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)_value,
0, GFP_NOFS);
if (rc <= 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint->flags & IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
if (rc && rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value->type) {
case IMA_XATTR_DIGEST:
- if (iint->flags & IMA_DIGSIG_REQUIRED) {
+ if (iint->flags & IMA_DIGSIG_REQUIRED ||
+ iint->flags & IMA_DIGSIG_OPTIONAL) {
cause = "IMA signature required";
status = INTEGRITY_FAIL;
break;
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL &&
+ iint->flags & IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry
*entry)
ima_log_string(ab, "appraise_type", args[0].from);
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
+ else if ((strcmp(args[0].from, "imasig_optional")) == 0)
+ entry->flags |= IMA_DIGSIG_OPTIONAL;
else
result = -EINVAL;
break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 84c37c4..2ba736b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,6 +30,7 @@
#define IMA_ACTION_FLAGS 0xff00
#define IMA_DIGSIG 0x0100
#define IMA_DIGSIG_REQUIRED0x0200
+#define
[PATCH 2/2] ima: Support appraise_type=imasig_optional
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal vgo...@redhat.com
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value,
0, GFP_NOFS);
if (rc = 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
if (rc rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL
+ iint-flags IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry
*entry)
ima_log_string(ab, appraise_type, args[0].from);
if ((strcmp(args[0].from, imasig)) == 0)
entry-flags |= IMA_DIGSIG_REQUIRED;
+ else if ((strcmp(args[0].from, imasig_optional)) == 0)
+ entry-flags |= IMA_DIGSIG_OPTIONAL;
else
result = -EINVAL;
break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 84c37c4..2ba736b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,6 +30,7 @@
#define IMA_ACTION_FLAGS 0xff00
#define IMA_DIGSIG 0x0100
#define IMA_DIGSIG_REQUIRED0x0200
+#define IMA_DIGSIG_OPTIONAL
Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional
On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
appraise_type=imasig_optional will allow appraisal to pass even if no
signatures are present on the file. If signatures are present, then it
has to be valid digital signature, otherwise appraisal will fail.
This can allow to selectively sign executables in the system and based
on appraisal results, signed executables with valid signatures can be
given extra capability to perform priviliged operations in secureboot
mode.
Signed-off-by: Vivek Goyal vgo...@redhat.com
Thanks, Vivek, the patch looks a lot better. Here are a couple of
suggestions:
- the patch description needs to start with the problem description, not
the solution.
- the patch name should reflect the problem.
A few comments are inline below.
thanks,
Mimi
---
Documentation/ABI/testing/ima_policy |2 +-
security/integrity/ima/ima_appraise.c | 24 +++-
security/integrity/ima/ima_policy.c |2 ++
security/integrity/integrity.h|1 +
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index de16de3..5ca0c23 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,7 +30,7 @@ Description:
uid:= decimal value
fowner:=decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] | [imasig_optional]
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 3710f44..222ade0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
const char *op = appraise_data;
char *cause = unknown;
- int rc;
+ int rc, audit_info = 0;
if (!ima_appraise)
return 0;
- if (!inode-i_op-getxattr)
+ if (!inode-i_op-getxattr) {
+ /* getxattr not supported. file couldn't have been signed */
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
Please don't change the result of the appraisal like this. A single
change can be made towards the bottom of process_measurement().
rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value,
0, GFP_NOFS);
if (rc = 0) {
/* File system does not support security xattr */
- if (rc == -EOPNOTSUPP)
+ if (rc == -EOPNOTSUPP) {
+ if (iint-flags IMA_DIGSIG_OPTIONAL)
+ return INTEGRITY_PASS;
return INTEGRITY_UNKNOWN;
+ }
ditto
if (rc rc != -ENODATA)
goto out;
@@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct
integrity_iint_cache *iint,
}
switch (xattr_value-type) {
case IMA_XATTR_DIGEST:
- if (iint-flags IMA_DIGSIG_REQUIRED) {
+ if (iint-flags IMA_DIGSIG_REQUIRED ||
+ iint-flags IMA_DIGSIG_OPTIONAL) {
cause = IMA signature required;
status = INTEGRITY_FAIL;
break;
@@ -201,8 +209,14 @@ out:
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
+ if (status == INTEGRITY_NOLABEL
+ iint-flags IMA_DIGSIG_OPTIONAL) {
+ status = INTEGRITY_PASS;
+ /* Don't flood audit logs with skipped appraise */
+ audit_info = 1;
+ }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ op, cause, rc, audit_info);
} else {
ima_cache_flags(iint, func);
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 4adcd0f..8b8cd5f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
ima_log_string(ab, appraise_type, args[0].from);
if ((strcmp(args[0].from, imasig)) == 0)
entry-flags |= IMA_DIGSIG_REQUIRED;
+ else if ((strcmp(args[0].from, imasig_optional)) == 0)
+ entry-flags |= IMA_DIGSIG_OPTIONAL;
By setting
