[PATCH] KEYS: refcount bug fix

[Mimi Zohar]

This patch fixes the key_ref leak, removes the unnecessary KEY_FLAG_KEEP
test before setting the flag, and cleans up the if/then brackets style
introduced in commit:
d3600bc KEYS: prevent keys from being removed from specified keyrings

Reported-by: David Howells <dhowe...@redhat.com>
Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/keys/key.c|  3 +--
 security/keys/keyctl.c | 17 +++--
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 09ef276..07a8731 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -430,8 +430,7 @@ static int __key_instantiate_and_link(struct key *key,
 
/* and link it into the destination keyring */
if (keyring) {
-   if (test_bit(KEY_FLAG_KEEP, >flags))
-   set_bit(KEY_FLAG_KEEP, >flags);
+   set_bit(KEY_FLAG_KEEP, >flags);
 
__key_link(key, _edit);
}
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index e83ec6b..8f9f323 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -381,12 +381,11 @@ long keyctl_revoke_key(key_serial_t id)
}
 
key = key_ref_to_ptr(key_ref);
+   ret = 0;
if (test_bit(KEY_FLAG_KEEP, >flags))
-   return -EPERM;
-   else {
+   ret = -EPERM;
+   else
key_revoke(key);
-   ret = 0;
-   }
 
key_ref_put(key_ref);
 error:
@@ -432,12 +431,11 @@ long keyctl_invalidate_key(key_serial_t id)
 
 invalidate:
key = key_ref_to_ptr(key_ref);
+   ret = 0;
if (test_bit(KEY_FLAG_KEEP, >flags))
ret = -EPERM;
-   else {
+   else
key_invalidate(key);
-   ret = 0;
-   }
 error_put:
key_ref_put(key_ref);
 error:
@@ -1352,12 +1350,11 @@ long keyctl_set_timeout(key_serial_t id, unsigned 
timeout)
 
 okay:
key = key_ref_to_ptr(key_ref);
+   ret = 0;
if (test_bit(KEY_FLAG_KEEP, >flags))
ret = -EPERM;
-   else {
+   else
key_set_timeout(key, timeout);
-   ret = 0;
-   }
key_put(key);
 
 error:
-- 
2.1.0



--
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

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Mimi Zohar]

On Wed, 2016-01-06 at 13:21 +, David Howells wrote:
> Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> 
> > The x509_validate_trust() was originally added for IMA to ensure, on a
> > secure boot system, a certificate chain of trust rooted in hardware.
> > The IMA MOK keyring extends this certificate chain of trust to the
> > running system.
> 
> The problem is that because 'trusted' is a boolean, a key in the IMA MOK
> keyring will permit addition to the system keyring.

Once the builtin keys are loaded onto the system keyring, isn't the
system keyring locked?  Or is this the only mechanism used for locking?

Mimi

--
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

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Mimi Zohar]

On Tue, 2016-01-05 at 16:39 +, David Howells wrote:
> Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> 
> > You're missing Petko's patch:
> > 41c89b6 IMA: create machine owner and blacklist keyrings
> 
> Hmmm...  This is wrong.  x509_key_preparse() shouldn't be polling the IMA MOK
> keyring under all circumstances.

The x509_validate_trust() was originally added for IMA to ensure, on a
secure boot system, a certificate chain of trust rooted in hardware.
The IMA MOK keyring extends this certificate chain of trust to the
running system.

The IMA MOK keyring is a build configuration option that defaults to
off.

Mimi

--
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

Re: [PATCH] X.509: Partially revert patch to add validation against IMA MOK keyring

[Mimi Zohar]

On Thu, 2016-01-07 at 00:34 +, David Howells wrote:
> David Howells  wrote:
> 
> > Partially revert commit 41c89b64d7184a780f12f2cccdabe65cb2408893:
> > 
> > Author: Petko Manolov 
> > Date:   Wed Dec 2 17:47:55 2015 +0200
> > IMA: create machine owner and blacklist keyrings
> > 
> > The problem is that prep->trusted is a simple boolean and the additional
> > x509_validate_trust() call doesn't therefore distinguish levels of
> > trustedness, but is just OR'd with the result of validation against the
> > system trusted keyring.
> > 
> > However, setting the trusted flag means that this key may be added to *any*
> > trusted-only keyring - including the system trusted keyring.

Hm, I'm not able to add a key to the system keyring that is signed by a
key on either the system or the IMA MOK keyrings.  The system keyring
seems to be "locked".  A key that is signed by either a key on the
system or the IMA MOK keyring can be added to the IMA keyring.

keyctl show %keyring:.system_keyring
Keyring
 973688077 ---lswrv  0 0  keyring: .system_keyring

evmctl import m1-cert-signed.der 973688077
add_key failed
errno: Permission denied (13)

Mimi

> > Whilst I appreciate what the patch is trying to do, I don't think this is
> > quite the right solution.

> Please apply this to security/next.
> 
> Thanks,
> David


--
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

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Mimi Zohar]

On Tue, 2016-01-05 at 15:47 +, David Howells wrote:
> If a certificate is self-signed, don't bother checking the validity of the
> signature.  The cert cannot be checked by validation against the next one
> in the chain as this is the root of the chain.  Trust for this certificate
> can only be determined by whether we obtained it from a trusted location
> (ie. it was built into the kernel at compile time).
> 
> This also fixes a bug whereby certificates were being assumed to be
> self-signed if they had neither AKID nor SKID, the symptoms of which show
> up as an attempt to load a certificate failing with -ERANGE or -EBADMSG.
> This is produced from the RSA module when the result of calculating "m =
> s^e mod n" is checked.
> 
> Signed-off-by: David Howells <dhowe...@redhat.com>
> cc: David Woodhouse <david.woodho...@intel.com>
> cc: Mimi Zohar <zo...@linux.vnet.ibm.com>
> ---
> 
>  crypto/asymmetric_keys/x509_public_key.c |   25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/x509_public_key.c 
> b/crypto/asymmetric_keys/x509_public_key.c
> index 2a44b3752471..26e1937af7f4 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -255,6 +255,9 @@ static int x509_validate_trust(struct x509_certificate 
> *cert,
>   struct key *key;
>   int ret = 1;
> 
> + if (!cert->akid_id || !cert->akid_skid)
> + return 1;
> +
>   if (!trust_keyring)
>   return -EOPNOTSUPP;
> 
> @@ -312,17 +315,21 @@ static int x509_key_preparse(struct 
> key_preparsed_payload *prep)
>   cert->pub->algo = pkey_algo[cert->pub->pkey_algo];
>   cert->pub->id_type = PKEY_ID_X509;
> 
> - /* Check the signature on the key if it appears to be self-signed */
> - if ((!cert->akid_skid && !cert->akid_id) ||
> - asymmetric_key_id_same(cert->skid, cert->akid_skid) ||
> - asymmetric_key_id_same(cert->id, cert->akid_id)) {
> - ret = x509_check_signature(cert->pub, cert); /* self-signed */
> - if (ret < 0)
> - goto error_free_cert;
> - } else if (!prep->trusted) {
> + /* See if we can derive the trustability of this certificate.
> +  *
> +  * When it comes to self-signed certificates, we cannot evaluate
> +  * trustedness except by the fact that we obtained it from a trusted
> +  * location.  So we just rely on x509_validate_trust() failing in this
> +  * case.
> +  *
> +  * Note that there's a possibility of a self-signed cert matching a
> +  * cert that we have (most likely a duplicate that we already trust) -
> +  * in which case it will be marked trusted.
> +  */
> + if (!prep->trusted) {
>   ret = x509_validate_trust(cert, get_system_trusted_keyring());
>   if (!ret)
> - prep->trusted = 1;
> + prep->trusted = true;
>   }

You're missing Petko's patch:
41c89b6 IMA: create machine owner and blacklist keyrings

Mimi

> 
>   /* Propose a description */
> 


--
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

Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Tue, 2015-12-29 at 07:06 -0500, Mimi Zohar wrote:
> On Tue, 2015-12-29 at 16:21 +0800, Dave Young wrote:

> This policy flexibility is needed at least until all files come from
> software providers with file signatures.  (RPM has been modified to
> include file signatures.)  Even then, in terms of kexec, some distros
> generate the initramfs on the target host and,  therefore, can not sign
> the initramfs.  The local user could, however, sign the initramfs on
> their own system.

Sorry, instead of "local user" the "local system/host owner" would be
more appropriate.

Mimi

--
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

Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Tue, 2015-12-29 at 16:21 +0800, Dave Young wrote:
> Hi, Mimi
> 
> On 12/28/15 at 07:51am, Mimi Zohar wrote:
> > On Mon, 2015-12-28 at 10:08 +0800, Dave Young wrote:
> > > On 12/25/15 at 09:45am, Mimi Zohar wrote:
> > > > IMA calculates the file hash, in this case, based on the buffer
> > > > contents.   The hash is calculated once and used for both measurement
> > > > and appraisal.  If the file integrity appraisal fails (eg. hash
> > > > comparison or signature failure), IMA prevents the kexec files from
> > > > being used.
> > > > 
> > > 
> > > Ok, thanks for the explanatioin. But I have another question, why do we
> > > need a special hook for KEXEC? Shouldn't all files use same way to do the
> > > measurement and appraisal?
> > 
> > "By all files" are you referring to all files read by the kernel or all
> > files opened, executed or mmapped by the system?
> 
> Hmm, I means any kind of files read by the kernel.
> 
> > 
> > Currently IMA allocates a page sized buffer, reads a file a page chunk
> > at a time calculating the file hash as it does so, and then frees the
> > buffer before returning to the caller.  This method of calculating the
> > file hash is used for measuring and appraising files opened
> > (FILE_CHECK), executed (BPRM_CHECK) or mmapped (MMAP_CHECK) by the
> > system.
> > 
> > This patch set addresses files being read by kernel.  A single new
> > generic hook named ima_hash_and_process_file() is defined to not only
> > measure and appraise the kexec image and initramfs, but firmware and the
> > IMA policy.   As we identify other places that the kernel is reading
> > files, this hook would be called in those places as well.
> 
> What I can not understand is why IMA need know the caller information and
> why cann't introduce a generic interface. kexec and firmware and other
> caller all read files, so a common file based interface should be better?

The next patch set will define a common function for reading files by
the kernel.  Luis set up a wiki
http://kernelnewbies.org/KernelProjects/common-kernel-loader with some
details.

This patch set defines a generic interface for measuring and appraising
files being read by the kernel, with the ability to define a policy
based on the caller information.   For the details on expressing a
policy, refer to Documentation/ABI/testing/ima-policy.   For example,
the new rules could be expressed like:

measure func=KEXEC_CHECK
appraise func=KEXEC_CHECK appraise_type=imasig
#
measure func=INITRAMFS_CHECK
appraise func=INITRAMFS_CHECK appraise_type=imasig
#
measure func=FIRMWARE_CHECK
appraise func=FIRMWARE_CHECK appraise_type=imasig
#
measure func=POLICY_CHECK
appraise func=POLICY_CHECK appraise_type=imasig

This policy flexibility is needed at least until all files come from
software providers with file signatures.  (RPM has been modified to
include file signatures.)  Even then, in terms of kexec, some distros
generate the initramfs on the target host and,  therefore, can not sign
the initramfs.  The local user could, however, sign the initramfs on
their own system.

Mimi

--
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

Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Mon, 2015-12-28 at 10:08 +0800, Dave Young wrote:
> On 12/25/15 at 09:45am, Mimi Zohar wrote:
> > IMA calculates the file hash, in this case, based on the buffer
> > contents.   The hash is calculated once and used for both measurement
> > and appraisal.  If the file integrity appraisal fails (eg. hash
> > comparison or signature failure), IMA prevents the kexec files from
> > being used.
> > 
> 
> Ok, thanks for the explanatioin. But I have another question, why do we
> need a special hook for KEXEC? Shouldn't all files use same way to do the
> measurement and appraisal?

"By all files" are you referring to all files read by the kernel or all
files opened, executed or mmapped by the system?

Currently IMA allocates a page sized buffer, reads a file a page chunk
at a time calculating the file hash as it does so, and then frees the
buffer before returning to the caller.  This method of calculating the
file hash is used for measuring and appraising files opened
(FILE_CHECK), executed (BPRM_CHECK) or mmapped (MMAP_CHECK) by the
system.

This patch set addresses files being read by kernel.  A single new
generic hook named ima_hash_and_process_file() is defined to not only
measure and appraise the kexec image and initramfs, but firmware and the
IMA policy.   As we identify other places that the kernel is reading
files, this hook would be called in those places as well.

Mimi

--
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

Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Mon, 2015-12-28 at 10:08 +0800, Dave Young wrote:
> On 12/25/15 at 09:45am, Mimi Zohar wrote:
> > IMA calculates the file hash, in this case, based on the buffer
> > contents.   The hash is calculated once and used for both measurement
> > and appraisal.  If the file integrity appraisal fails (eg. hash
> > comparison or signature failure), IMA prevents the kexec files from
> > being used.
> > 
> 
> Ok, thanks for the explanatioin. But I have another question, why do we
> need a special hook for KEXEC? Shouldn't all files use same way to do the
> measurement and appraisal?

"By all files" are you referring to all files read by the kernel or all
files opened, executed or mmapped by the system?

Currently IMA allocates a page sized buffer, reads a file a page chunk
at a time calculating the file hash as it does so, and then frees the
buffer before returning to the caller.  This method of calculating the
file hash is used for measuring and appraising files opened
(FILE_CHECK), executed (BPRM_CHECK) or mmapped (MMAP_CHECK) by the
system.

This patch set addresses files being read by kernel.  A single new
generic hook named ima_hash_and_process_file() is defined to not only
measure and appraise the kexec image and initramfs, but firmware and the
IMA policy.   As we identify other places that the kernel is reading
files, this hook would be called in those places as well.

Mimi

--
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

Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Fri, 2015-12-25 at 13:33 +0800, Dave Young wrote:
> Hi, Mimi
> 
> CCing kexec list, not all kexec people subscribed to IMA list.
> I just subscribed to it since Vivek CCed me last time about the V1 of this
> series.

Thanks!

> On 12/23/15 at 06:55pm, Mimi Zohar wrote:
> > This patch defines a new IMA hook ima_hash_and_process_file() for
> > measuring and appraising files read by the kernel.  The caller loads
> > the file into memory before calling this function, which calculates
> > the hash followed by the normal IMA policy based processing.
> > 
> > Two new IMA policy functions named KEXEC_CHECK and INITRAMFS_CHECK
> > are defined for measuring, appraising or auditing the kexec image
> > and initramfs.
> 
> Could you help us understand why do we need it first.

IMA can be viewed as extending secure and trusted boot to the running
system in a uniform and consistent manner.   As files are accessed,
based on policy, IMA measures them, appends the file measurements to the
running measurement list (/ima/ascii_runtime_measurements)
and appraises the file's integrity, based on either the file's hash or
signature, which are stored as extended attributes in "security.ima".

There are still a couple of file measurement and appraisal gaps that
need to be closed.

> I think I do not really understand the purpose of the IMA handling
> about kexec kernel and initramfs.

One of those measurement and appraisal gaps are files that are read by
the kernel, like the kexec image and initramfs.

[There is a lot of code duplication in the kernel for reading a file and
verifying its signature.   Each place does it just a bit differently
than the other.  I'm working with Luis Rodriguez on defining a single,
common function  - https://lkml.org/lkml/2015/12/21/478.]

> * Does the files in disk space have already contains some hash values 
> and when kernel load it IMA functions will do some checking? But seems I do 
> not
> see such handling..

IMA sits on a number of the LSM hooks, where they exist, and in other
places defines its own hook.   This patch set defines a new IMA hook for
measuring and appraising files being read by the kernel.

> * Does it try to calculate the hash of the file buffer after copying,

IMA calculates the file hash, in this case, based on the buffer
contents.   The hash is calculated once and used for both measurement
and appraisal.  If the file integrity appraisal fails (eg. hash
comparison or signature failure), IMA prevents the kexec files from
being used.

> and IMA will avoid future modification based on the hash calculated?
> If this is the purpose I think it should be wrong because kexe file buffers  
> will be freed at the end of kexec_file_load syscall.

The ima_hash_and _process_file() call calculates the file's hash, adds
the hash to the IMA measurement list and appraises the file's integrity.
On integrity failure, in this case, it prevents the kexec files from
being used, causing the buffers to be freed.

Mimi

> > 
> > Changelog v2:
> > - Calculate the file hash from the in memory buffer (suggested by Dave 
> > Young)
> > - Rename ima_read_and_process_file() to ima_hash_and_process_file()
> > - replace individual case statements with range:
> > KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1
> > v1:
> > - Instead of ima_read_and_process_file() allocating memory, the caller
> > allocates and frees the memory.
> > - Moved the kexec measurement/appraisal call to copy_file_from_fd(). The
> > same call now measures and appraises both the kexec image and initramfs.
> > 
> > Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> > ---
> >  Documentation/ABI/testing/ima_policy  |  2 +-
> >  include/linux/ima.h   | 16 ++
> >  kernel/kexec_file.c   | 24 
> >  security/integrity/iint.c |  1 +
> >  security/integrity/ima/ima.h  | 21 --
> >  security/integrity/ima/ima_api.c  |  6 +++--
> >  security/integrity/ima/ima_appraise.c | 11 --
> >  security/integrity/ima/ima_main.c | 41 
> > ---
> >  security/integrity/ima/ima_policy.c   | 38 
> >  security/integrity/integrity.h|  7 --
> >  10 files changed, 127 insertions(+), 40 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/ima_policy 
> > b/Documentation/ABI/testing/ima_policy
> > index 0a378a8..e80f767 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -26,7 +26,7 @@ Description:
> > option: [[appraise_type=]] [permit_directio]
> >  
&g

[PATCH v2 2/7] ima: load policy using path

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasat...@samsung.com>

We currently cannot do appraisal or signature vetting of IMA policies
since we currently can only load IMA policies by writing the contents
of the policy directly in, as follows:

cat policy-file > /ima/policy

If we provide the kernel the path to the IMA policy so it can load
the policy itself it'd be able to later appraise or vet the file
signature if it has one.  This patch adds support to load the IMA
policy with a given path as follows:

echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy

Changelog v2:
- Patch description re-written by Luis R. Rodriguez

Signed-off-by: Dmitry Kasatkin <d.kasat...@samsung.com>
Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/iint.c   |  4 +---
 security/integrity/ima/ima_fs.c | 39 ++-
 security/integrity/integrity.h  |  2 +-
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 2de9c82..54b51a4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -203,10 +203,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
  * This is function opens a file, allocates the buffer of required
  * size, read entire file content to the buffer and closes the file
  *
- * It is used only by init code.
- *
  */
-int __init integrity_read_file(const char *path, char **data)
+int integrity_read_file(const char *path, char **data)
 {
struct file *file;
loff_t size;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index eebb985..f902b6b 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -258,6 +258,40 @@ static const struct file_operations 
ima_ascii_measurements_ops = {
.release = seq_release,
 };
 
+static ssize_t ima_read_policy(char *path)
+{
+   char *data, *datap;
+   int rc, size, pathlen = strlen(path);
+   char *p;
+
+   /* remove \n */
+   datap = path;
+   strsep(, "\n");
+
+   rc = integrity_read_file(path, );
+   if (rc < 0)
+   return rc;
+
+   size = rc;
+   datap = data;
+
+   while (size > 0 && (p = strsep(, "\n"))) {
+   pr_debug("rule: %s\n", p);
+   rc = ima_parse_add_rule(p);
+   if (rc < 0)
+   break;
+   size -= rc;
+   }
+
+   kfree(data);
+   if (rc < 0)
+   return rc;
+   else if (size)
+   return -EINVAL;
+   else
+   return pathlen;
+}
+
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
size_t datalen, loff_t *ppos)
 {
@@ -288,7 +322,10 @@ static ssize_t ima_write_policy(struct file *file, const 
char __user *buf,
if (copy_from_user(data, buf, datalen))
goto out;
 
-   result = ima_parse_add_rule(data);
+   if (data[0] == '/')
+   result = ima_read_policy(data);
+   else
+   result = ima_parse_add_rule(data);
 out:
if (result < 0)
valid_policy = 0;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 5efe2ec..5413f22 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -122,7 +122,7 @@ struct integrity_iint_cache *integrity_iint_find(struct 
inode *inode);
 
 int integrity_kernel_read(struct file *file, loff_t offset,
  char *addr, unsigned long count);
-int __init integrity_read_file(const char *path, char **data);
+int integrity_read_file(const char *path, char **data);
 
 #define INTEGRITY_KEYRING_EVM  0
 #define INTEGRITY_KEYRING_IMA  1
-- 
2.1.0

--
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

[PATCH v2 7/7] ima: require signed IMA policy

[Mimi Zohar]

Require the IMA policy to be signed when additional rules can be added.

Changelog v1:
- initialize the policy flag
- include IMA_APPRAISE_POLICY in the policy flag

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_policy.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 3715c9e..8d5fd0b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -131,6 +131,10 @@ static struct ima_rule_entry default_appraise_rules[] = {
{.action = DONT_APPRAISE, .fsmagic = SELINUX_MAGIC, .flags = 
IMA_FSMAGIC},
{.action = DONT_APPRAISE, .fsmagic = NSFS_MAGIC, .flags = IMA_FSMAGIC},
{.action = DONT_APPRAISE, .fsmagic = CGROUP_SUPER_MAGIC, .flags = 
IMA_FSMAGIC},
+#ifdef CONFIG_IMA_WRITE_POLICY
+   {.action = APPRAISE, .read_func = POLICY_CHECK,
+   .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
 #ifndef CONFIG_IMA_APPRAISE_SIGNED_INIT
{.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .flags = IMA_FOWNER},
 #else
@@ -414,9 +418,12 @@ void __init ima_init_policy(void)
for (i = 0; i < appraise_entries; i++) {
list_add_tail(_appraise_rules[i].list,
  _default_rules);
+   if (default_appraise_rules[i].read_func == POLICY_CHECK)
+   temp_ima_appraise |= IMA_APPRAISE_POLICY;
}
 
ima_rules = _default_rules;
+   ima_update_policy_flag();
 }
 
 /**
-- 
2.1.0

--
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

[PATCH v2 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

Instead of reading the firmware twice, once for measuring/appraising
the firmware and again reading the file contents into memory, this
patch reads the firmware once.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 drivers/base/firmware_class.c |  5 +
 include/linux/ima.h   |  1 +
 security/integrity/ima/ima.h  |  1 -
 security/integrity/ima/ima_appraise.c |  8 
 security/integrity/ima/ima_main.c |  2 +-
 security/integrity/ima/ima_policy.c   | 24 +++-
 security/integrity/integrity.h| 11 ---
 7 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450..3ca96a6 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -311,6 +312,10 @@ static int fw_read_file_contents(struct file *file, struct 
firmware_buf *fw_buf)
rc = -EIO;
goto fail;
}
+   rc = ima_hash_and_process_file(file, buf, size, FIRMWARE_CHECK);
+   if (rc)
+   goto fail;
+
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
goto fail;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 020de0f..c40fb3d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -16,6 +16,7 @@ struct linux_binprm;
 enum ima_read_hooks {
KEXEC_CHECK = 1,
INITRAMFS_CHECK,
+   FIRMWARE_CHECK,
IMA_MAX_READ_CHECK
 };
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index fa801fa..7fb0ab7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -165,7 +165,6 @@ enum ima_hooks {
MMAP_CHECK,
BPRM_CHECK,
MODULE_CHECK,
-   FIRMWARE_CHECK,
POST_SETATTR
 };
 
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 3adf937..57b1ad1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -76,8 +76,6 @@ enum integrity_status ima_get_cache_status(struct 
integrity_iint_cache *iint,
return iint->ima_bprm_status;
case MODULE_CHECK:
return iint->ima_module_status;
-   case FIRMWARE_CHECK:
-   return iint->ima_firmware_status;
case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
return iint->ima_read_status;
case FILE_CHECK:
@@ -99,9 +97,6 @@ static void ima_set_cache_status(struct integrity_iint_cache 
*iint,
case MODULE_CHECK:
iint->ima_module_status = status;
break;
-   case FIRMWARE_CHECK:
-   iint->ima_firmware_status = status;
-   break;
case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
iint->ima_read_status = status;
break;
@@ -124,9 +119,6 @@ static void ima_cache_flags(struct integrity_iint_cache 
*iint, int func)
case MODULE_CHECK:
iint->flags |= (IMA_MODULE_APPRAISED | IMA_APPRAISED);
break;
-   case FIRMWARE_CHECK:
-   iint->flags |= (IMA_FIRMWARE_APPRAISED | IMA_APPRAISED);
-   break;
case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
break;
case FILE_CHECK:
diff --git a/security/integrity/ima/ima_main.c 
b/security/integrity/ima/ima_main.c
index cd9c576..c55ee06 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -345,7 +345,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t 
size)
return -EACCES; /* INTEGRITY_UNKNOWN */
return 0;
}
-   return process_measurement(file, NULL, 0, MAY_EXEC, FIRMWARE_CHECK, 0);
+   return 0;
 }
 
 /**
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 26e24df..6ac25c5 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -102,7 +102,7 @@ static struct ima_rule_entry original_measurement_rules[] = 
{
{.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
 .uid = GLOBAL_ROOT_UID, .flags = IMA_FUNC | IMA_MASK | IMA_UID},
{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
-   {.action = MEASURE, .func = FIRMWARE_CHECK, .flags = IMA_FUNC},
+   {.action = MEASURE, .read_func = FIRMWARE_CHECK, .flags = IMA_FUNC},
 };
 
 static struct ima_rule_entry default_measurement_rules[] = {
@@ -115,7 +115,7 @@ static struct ima_rule_entry default_measurement_rules[] = {
{.action = MEASURE, .func = FILE_CHECK, .mask = MAY_READ,
 .uid = GLOBAL_ROOT_UID, .flags = IMA_FUNC | IMA_INMASK | IMA_UID},
{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
-   {.action = MEASURE, .func = FIRMWARE_CHECK, .fl

[PATCH v2 0/7] ima: measuring/appraising files read by the kernel

[Mimi Zohar]

This patch set closes a number of measurement/appraisal gaps by defining
a generic function named ima_hash_and_process_file() for measuring and
appraising files read by the kernel (eg. kexec image and initramfs,
firmware, IMA policy).

To differentiate between callers of ima_hash_and_process_file() in the
IMA policy, a new enumeration is defined named ima_read_hooks, which
initially includes KEXEC_CHECK, INITRAMFS_CHECK, FIRMWARE_CHECK, and
POLICY_CHECK.

Changelog v2:
- Calculate the file hash from an in memory buffer (suggested by Dave Young)
- Rename ima_read_and_process_file() to ima_hash_and_process_file() to
reflect doing a buffer hash.
- 
Changelog v1:
- Instead of ima_read_and_process_file() allocating memory, the caller
allocates and frees the memory.
- Moved the kexec measurement/appraisal call to copy_file_from_fd(). The
same call now measures and appraises both the kexec image and initramfs.

Mimi

Dmitry Kasatkin (3):
  ima: separate 'security.ima' reading functionality from collect
  ima: load policy using path
  ima: provide buffer hash calculation function

Mimi Zohar (4):
  ima: measure and appraise kexec image and initramfs
  ima: measure and appraise firmware (improvement)
  ima: measure and appraise the IMA policy itself
  ima: require signed IMA policy

 Documentation/ABI/testing/ima_policy  |  2 +-
 drivers/base/firmware_class.c |  5 +++
 include/linux/ima.h   | 18 
 kernel/kexec_file.c   | 24 ++
 security/integrity/digsig.c   |  2 +-
 security/integrity/iint.c | 17 ---
 security/integrity/ima/ima.h  | 36 +--
 security/integrity/ima/ima_api.c  | 19 +++-
 security/integrity/ima/ima_appraise.c | 38 
 security/integrity/ima/ima_crypto.c   | 13 --
 security/integrity/ima/ima_fs.c   | 45 ++-
 security/integrity/ima/ima_init.c |  2 +-
 security/integrity/ima/ima_main.c | 50 -
 security/integrity/ima/ima_policy.c   | 73 +++
 security/integrity/ima/ima_template.c |  2 -
 security/integrity/ima/ima_template_lib.c |  1 -
 security/integrity/integrity.h| 14 +++---
 17 files changed, 255 insertions(+), 106 deletions(-)

-- 
2.1.0

--
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

[PATCH v2 6/7] ima: measure and appraise the IMA policy itself

[Mimi Zohar]

Call ima_hash_and_process_file() to measure and appraise the IMA policy.
This patch defines a new policy hook named POLICY_CHECK.

Changelog v2:
- remove S_ISREG() test

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 include/linux/ima.h |  1 +
 security/integrity/digsig.c |  2 +-
 security/integrity/iint.c   | 14 ++
 security/integrity/ima/ima.h|  1 +
 security/integrity/ima/ima_fs.c | 12 +---
 security/integrity/ima/ima_policy.c | 14 --
 security/integrity/integrity.h  |  4 +++-
 7 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index c40fb3d..1080623 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -17,6 +17,7 @@ enum ima_read_hooks {
KEXEC_CHECK = 1,
INITRAMFS_CHECK,
FIRMWARE_CHECK,
+   POLICY_CHECK,
IMA_MAX_READ_CHECK
 };
 
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 8ef1511..e58a5f6 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -104,7 +104,7 @@ int __init integrity_load_x509(const unsigned int id, const 
char *path)
if (!keyring[id])
return -EINVAL;
 
-   rc = integrity_read_file(path, );
+   rc = integrity_read_file(path, , 0);
if (rc < 0)
return rc;
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 8a45576..24776f7 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -205,7 +205,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
  * size, read entire file content to the buffer and closes the file
  *
  */
-int integrity_read_file(const char *path, char **data)
+int integrity_read_file(const char *path, char **data,
+   enum ima_read_hooks read_func)
 {
struct file *file;
loff_t size;
@@ -233,12 +234,17 @@ int integrity_read_file(const char *path, char **data)
}
 
rc = integrity_kernel_read(file, 0, buf, size);
+   if (rc > 0 && rc != size)
+   rc = -EIO;
+   else if (rc > 0)
+   rc = ima_hash_and_process_file(file, buf, size, read_func);
+
if (rc < 0)
kfree(buf);
-   else if (rc != size)
-   rc = -EIO;
-   else
+   else {
+   rc = size;
*data = buf;
+   }
 out:
fput(file);
return rc;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7fb0ab7..ef380f7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -185,6 +185,7 @@ int ima_policy_show(struct seq_file *m, void *v);
 #define IMA_APPRAISE_LOG   0x04
 #define IMA_APPRAISE_MODULES   0x08
 #define IMA_APPRAISE_FIRMWARE  0x10
+#define IMA_APPRAISE_POLICY0x20
 
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index f902b6b..fdc5326 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -268,13 +268,12 @@ static ssize_t ima_read_policy(char *path)
datap = path;
strsep(, "\n");
 
-   rc = integrity_read_file(path, );
+   rc = integrity_read_file(path, , POLICY_CHECK);
if (rc < 0)
return rc;
 
size = rc;
datap = data;
-
while (size > 0 && (p = strsep(, "\n"))) {
pr_debug("rule: %s\n", p);
rc = ima_parse_add_rule(p);
@@ -324,7 +323,14 @@ static ssize_t ima_write_policy(struct file *file, const 
char __user *buf,
 
if (data[0] == '/')
result = ima_read_policy(data);
-   else
+   else if (ima_appraise & IMA_APPRAISE_POLICY) {
+   pr_err("IMA: signed policy required\n");
+   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
+   "policy_update", "signed policy required",
+   1, 0);
+   if (ima_appraise & IMA_APPRAISE_ENFORCE)
+   result = -EACCES;
+   } else
result = ima_parse_add_rule(data);
 out:
if (result < 0)
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 6ac25c5..3715c9e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -116,6 +116,7 @@ static struct ima_rule_entry default_measurement_rules[] = {
 .uid = GLOBAL_ROOT_UID, .flags = IMA_FUNC | IMA_INMASK | IMA_UID},
{.action = MEASURE, .func = MODULE_CHECK, .flags = IMA_FUNC},
{.action = MEASURE, .read_func = FIRMWARE_CHECK, .flags = IMA_FUNC},
+   {.action = MEASURE, .read_func = POLICY_CHECK, .flags = IMA_FUNC},
 };
 
 sta

[PATCH v2 1/7] ima: separate 'security.ima' reading functionality from collect

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasat...@samsung.com>

Instead of passing pointers to pointers to ima_collect_measurent() to
read and return the 'security.ima' xattr value, this patch moves the
functionality to the calling process_measurement() to directly read
the xattr and pass only the hash algo to the ima_collect_measurement().

Signed-off-by: Dmitry Kasatkin <d.kasat...@samsung.com>
Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h  | 15 +++
 security/integrity/ima/ima_api.c  | 15 +++
 security/integrity/ima/ima_appraise.c | 25 ++---
 security/integrity/ima/ima_crypto.c   |  2 +-
 security/integrity/ima/ima_init.c |  2 +-
 security/integrity/ima/ima_main.c | 11 +++
 security/integrity/ima/ima_template.c |  2 --
 security/integrity/ima/ima_template_lib.c |  1 -
 8 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 917407f..9ebfec1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "../integrity.h"
 
@@ -140,9 +141,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 int ima_get_action(struct inode *inode, int mask, int function);
 int ima_must_measure(struct inode *inode, int mask, int function);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len);
+   struct file *file, enum hash_algo algo);
 void ima_store_measurement(struct integrity_iint_cache *iint, struct file 
*file,
   const unsigned char *filename,
   struct evm_ima_xattr_data *xattr_value,
@@ -187,8 +186,8 @@ int ima_must_appraise(struct inode *inode, int mask, enum 
ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
   int func);
-void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len,
-  struct ima_digest_data *hash);
+enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
+int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
   struct evm_ima_xattr_data **xattr_value);
 
@@ -220,10 +219,10 @@ static inline enum integrity_status 
ima_get_cache_status(struct integrity_iint_c
return INTEGRITY_UNKNOWN;
 }
 
-static inline void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
-int xattr_len,
-struct ima_digest_data *hash)
+static inline enum hash_algo
+ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
 {
+   return ima_hash_algo;
 }
 
 static inline int ima_read_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1d950fb..e7c7a5d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include 
+
 #include "ima.h"
 
 /*
@@ -188,9 +188,7 @@ int ima_get_action(struct inode *inode, int mask, int 
function)
  * Return 0 on success, error code otherwise
  */
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len)
+   struct file *file, enum hash_algo algo)
 {
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
@@ -201,9 +199,6 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
char digest[IMA_MAX_DIGEST_SIZE];
} hash;
 
-   if (xattr_value)
-   *xattr_len = ima_read_xattr(file->f_path.dentry, xattr_value);
-
if (!(iint->flags & IMA_COLLECTED)) {
u64 i_version = file_inode(file)->i_version;
 
@@ -213,11 +208,7 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
goto out;
}
 
-   /* use default hash algorithm */
-   hash.hdr.algo = ima_hash_algo;
-
-   if (xattr_value)
-   ima_get_hash_algo(*xattr_value, *xattr_len, );
+   hash.hdr.algo = algo;
 
result = ima_calc_file_hash(file, );
if (!result) {
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 1873b55..9c2b46b 100644
--- a/security/integrity/ima/ima_appraise

[PATCH v2 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

This patch defines a new IMA hook ima_hash_and_process_file() for
measuring and appraising files read by the kernel.  The caller loads
the file into memory before calling this function, which calculates
the hash followed by the normal IMA policy based processing.

Two new IMA policy functions named KEXEC_CHECK and INITRAMFS_CHECK
are defined for measuring, appraising or auditing the kexec image
and initramfs.

Changelog v2:
- Calculate the file hash from the in memory buffer (suggested by Dave Young)
- Rename ima_read_and_process_file() to ima_hash_and_process_file()
- replace individual case statements with range:
KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1
v1:
- Instead of ima_read_and_process_file() allocating memory, the caller
allocates and frees the memory.
- Moved the kexec measurement/appraisal call to copy_file_from_fd(). The
same call now measures and appraises both the kexec image and initramfs.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/ima_policy  |  2 +-
 include/linux/ima.h   | 16 ++
 kernel/kexec_file.c   | 24 
 security/integrity/iint.c |  1 +
 security/integrity/ima/ima.h  | 21 --
 security/integrity/ima/ima_api.c  |  6 +++--
 security/integrity/ima/ima_appraise.c | 11 --
 security/integrity/ima/ima_main.c | 41 ---
 security/integrity/ima/ima_policy.c   | 38 
 security/integrity/integrity.h|  7 --
 10 files changed, 127 insertions(+), 40 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy 
b/Documentation/ABI/testing/ima_policy
index 0a378a8..e80f767 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -26,7 +26,7 @@ Description:
option: [[appraise_type=]] [permit_directio]
 
base:   func:= 
[BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
-   [FIRMWARE_CHECK]
+   [FIRMWARE_CHECK] [KEXEC_CHECK] [INITRAMFS_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
   [[^]MAY_EXEC]
fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 120ccc5..020de0f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -13,6 +13,12 @@
 #include 
 struct linux_binprm;
 
+enum ima_read_hooks {
+   KEXEC_CHECK = 1,
+   INITRAMFS_CHECK,
+   IMA_MAX_READ_CHECK
+};
+
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask, int opened);
@@ -20,6 +26,9 @@ extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
 extern int ima_fw_from_file(struct file *file, char *buf, size_t size);
+extern int ima_hash_and_process_file(struct file *file,
+void *buf, size_t size,
+enum ima_read_hooks read_func);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -52,6 +61,13 @@ static inline int ima_fw_from_file(struct file *file, char 
*buf, size_t size)
return 0;
 }
 
+static inline int ima_hash_and_process_file(struct file *file,
+   void *buf, size_t size,
+   enum ima_read_hooks read_func)
+{
+   return 0;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b70ada0..1d0d998 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
 
 static int kexec_calculate_store_digests(struct kimage *image);
 
-static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
+static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len,
+enum ima_read_hooks read_func)
 {
struct fd f = fdget(fd);
int ret;
@@ -70,9 +72,8 @@ static int copy_file_from_fd(int fd, void **buf, unsigned 
long *buf_len)
bytes = kernel_read(f.file, pos, (char *)(*buf) + pos,
stat.size - pos);
if (bytes < 0) {
-   vfree(*buf);
ret = bytes;
-   goto out;
+   goto out_free;
}
 
if (bytes == 0)
@@ -80,13 +81,17 @@ static int copy_file_from_fd(int fd, void **buf, unsigned 
long *buf_len)
pos += bytes;
}
 
-   if (pos != stat.size) {
+   if (pos != stat.size)
ret = -EBADF;
+
+ 

[PATCH v2 3/7] ima: provide buffer hash calculation function

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasat...@samsung.com>

This patch provides convenient buffer hash calculation function.

Signed-off-by: Dmitry Kasatkin <d.kasat...@samsung.com>
Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h|  2 ++
 security/integrity/ima/ima_crypto.c | 11 +--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9ebfec1..e3a5b7b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -107,6 +107,8 @@ int ima_add_template_entry(struct ima_template_entry 
*entry, int violation,
   const char *op, struct inode *inode,
   const unsigned char *filename);
 int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
+int ima_calc_buffer_hash(const void *buf, int len,
+struct ima_digest_data *hash);
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
  struct ima_template_desc *desc, int num_fields,
  struct ima_digest_data *hash);
diff --git a/security/integrity/ima/ima_crypto.c 
b/security/integrity/ima/ima_crypto.c
index fb30ce4..aad1998 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -478,13 +478,13 @@ static int ima_calc_field_array_hash_tfm(struct 
ima_field_data *field_data,
u8 *data_to_hash = field_data[i].data;
u32 datalen = field_data[i].len;
 
-   if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
+   if (td && strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
rc = crypto_shash_update(shash,
(const u8 *) _data[i].len,
sizeof(field_data[i].len));
if (rc)
break;
-   } else if (strcmp(td->fields[i]->field_id, "n") == 0) {
+   } else if (td && strcmp(td->fields[i]->field_id, "n") == 0) {
memcpy(buffer, data_to_hash, datalen);
data_to_hash = buffer;
datalen = IMA_EVENT_NAME_LEN_MAX + 1;
@@ -519,6 +519,13 @@ int ima_calc_field_array_hash(struct ima_field_data 
*field_data,
return rc;
 }
 
+int ima_calc_buffer_hash(const void *buf, int len, struct ima_digest_data 
*hash)
+{
+   struct ima_field_data fd = { .data = (u8 *)buf, .len = len };
+
+   return ima_calc_field_array_hash(, NULL, 1, hash);
+}
+
 static void __init ima_pcrread(int idx, u8 *pcr)
 {
if (!ima_used_chip)
-- 
2.1.0

--
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

Re: [PATCH] IMA: policy can be updated zero times

[Mimi Zohar]

On Wed, 2015-12-23 at 13:47 +0200, Petko Manolov wrote:

> On 15-12-22 16:50:01, Sasha Levin wrote:
> > On 12/22/2015 04:40 PM, Petko Manolov wrote:
> > >> Thanks, Sasha.  By the time ima_update_policy() is called
> > >> >ima_release_policy() has already output the policy update status
> > >> >message.  I guess an empty policy could be considered a valid policy.
> > >> >Could you add a msg indicating that the new policy was empty?
> > > 
> > > As far as I can say we can't get to ima_update_policy() with empty 
> > > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in 
> > > case 
> > > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > > too.
> > 
> > This is based on an actual crash rather than code analysis.
> 
> I was able to reproduce the crash with: echo "" > 
> /sys/kernel/security/ima/policy
> 
> It turns out ima_parse_add_rule() returns 1, even though the string is empty 
> This logic may be part of "empty policy is a valid policy" or something else. 
>  
> As it is more dangerous to change the behavior at this point i assume your 
> patch 
> is the right solution for the problem.
> 
> Acked-by: Petko Manolov 
> 
> Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
> much work?

ima_parse_add_rules() has no way of knowing if the policy as a whole is
valid.  I would define a new function in ima_policy.c to return the
number of rules being added and call it at the beginning of
ima_release_policy() before the status message.  That way the number of
rules added can be included in the status message.

For now, the function could just return have rules or no rules, instead
of the number of rules.

Mimi

--
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

Re: [Linux-ima-devel] [PATCH] IMA: policy can be updated zero times

[Mimi Zohar]

On Wed, 2015-12-23 at 07:24 -0500, Mimi Zohar wrote:
> On Wed, 2015-12-23 at 13:47 +0200, Petko Manolov wrote:
> 
> > On 15-12-22 16:50:01, Sasha Levin wrote:
> > > On 12/22/2015 04:40 PM, Petko Manolov wrote:
> > > >> Thanks, Sasha.  By the time ima_update_policy() is called
> > > >> >ima_release_policy() has already output the policy update status
> > > >> >message.  I guess an empty policy could be considered a valid policy.
> > > >> >Could you add a msg indicating that the new policy was empty?
> > > > 
> > > > As far as I can say we can't get to ima_update_policy() with empty 
> > > > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in 
> > > > case 
> > > > of an empty rule.  I'll double check it tomorrow, but please you do 
> > > > that 
> > > > too.
> > > 
> > > This is based on an actual crash rather than code analysis.
> > 
> > I was able to reproduce the crash with: echo "" > 
> > /sys/kernel/security/ima/policy
> > 
> > It turns out ima_parse_add_rule() returns 1, even though the string is 
> > empty 
> > This logic may be part of "empty policy is a valid policy" or something 
> > else.  
> > As it is more dangerous to change the behavior at this point i assume your 
> > patch 
> > is the right solution for the problem.
> > 
> > Acked-by: Petko Manolov <pet...@mip-labs.com>
> > 
> > Mimi, shall we change ima_parse_add_rule's behavior in the future or it's 
> > too 
> > much work?
> 
> ima_parse_add_rules() has no way of knowing if the policy as a whole is
> valid.  I would define a new function in ima_policy.c to return the
> number of rules being added and call it at the beginning of
> ima_release_policy() before the status message.  That way the number of
> rules added can be included in the status message.
> 
> For now, the function could just return have rules or no rules, instead
> of the number of rules.

Sasha, could you make your fix a separate function (above
ima_update_policy) and call it from ima_release_policy()?

Thanks!

Mimi

--
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

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

On Mon, 2015-12-21 at 22:44 +0100, Luis R. Rodriguez wrote:
> 
> Eventually, once we generalize a common read perhaps we should stuff this
> into VFS common code and provide arguments to enable callers to provide 
> restrictions or requirements. Let's work together on that after the holidays.
> 
> Let's consolidate notes here:
> 
> http://kernelnewbies.org/KernelProjects/common-kernel-loader

Sounds good!

Mimi


--
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

[GIT PULL] linux-integrity changes for 4.5

[Mimi Zohar]

Hi James,

Lots of changes this time.  This pull request adds support, by Dmitry
Kasatkin, for: making the EVM keyring a trusted keyring, such that only
keys signed by a key on the system keyring can be loaded onto the EVM
keyring, loading the EVM keys onto the EVM trusted keyring by the
kernel, enabling EVM when either the x509 or symmetric keys are
available and loading the EVM symmetric key from hardware.

As described by Mark Baushke and Petko Manalov at LSS 2015 in their talk
"IMA/EVM: Real Applications for Embedded Networking Systems", this pull
request includes support for two new IMA trusted keyrings named .ima_mok
and .ima_blacklist.  Keys being loaded on either the EVM or IMA trusted
keyrings can be validated against either the system trusted keyring or
the intermediary .ima_mok keyring and prevented from being loaded if on
the .ima_blacklist keyring.

Lastly, support for extending and displaying the IMA policy.

Thanks!

Mimi

The following changes since commit ebd68df3f24b318d391d15c458d6f43f340ba36a:

  Sync to Linus v4.4-rc2 for LSM developers. (2015-11-23 22:46:28 +1100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next

for you to fetch changes up to 92cc916638a48f285736cd5541536e2e1b73ecf8:

  security/integrity: make ima/ima_mok.c explicitly non-modular (2015-12-15 
10:01:43 -0500)


Arnd Bergmann (1):
  evm: EVM_LOAD_X509 depends on EVM

Dmitry Kasatkin (5):
  integrity: define '.evm' as a builtin 'trusted' keyring
  evm: load an x509 certificate from the kernel
  evm: enable EVM when X509 certificate is loaded
  evm: provide a function to set the EVM key from the kernel
  evm: reset EVM status when file attributes change

Mimi Zohar (3):
  KEYS: prevent keys from being removed from specified keyrings
  IMA: prevent keys on the .ima_blacklist from being removed
  ima: update appraise flags after policy update completes

Paul Gortmaker (1):
  security/integrity: make ima/ima_mok.c explicitly non-modular

Petko Manolov (3):
  IMA: policy can now be updated multiple times
  IMA: create machine owner and blacklist keyrings
  IMA: allow reading back the current IMA policy

 crypto/asymmetric_keys/x509_public_key.c |   2 +
 include/keys/system_keyring.h|  24 +++
 include/linux/evm.h  |   7 +
 include/linux/key.h  |   1 +
 security/integrity/Kconfig   |  11 ++
 security/integrity/digsig.c  |  14 +-
 security/integrity/digsig_asymmetric.c   |  14 ++
 security/integrity/evm/Kconfig   |  17 ++
 security/integrity/evm/evm.h |   3 +
 security/integrity/evm/evm_crypto.c  |  54 +-
 security/integrity/evm/evm_main.c|  32 +++-
 security/integrity/evm/evm_secfs.c   |  12 +-
 security/integrity/iint.c|   1 +
 security/integrity/ima/Kconfig   |  44 -
 security/integrity/ima/Makefile  |   1 +
 security/integrity/ima/ima.h |  23 ++-
 security/integrity/ima/ima_fs.c  |  42 -
 security/integrity/ima/ima_init.c|   2 +-
 security/integrity/ima/ima_mok.c |  55 ++
 security/integrity/ima/ima_policy.c  | 293 +++
 security/integrity/integrity.h   |  13 +-
 security/keys/key.c  |   6 +-
 security/keys/keyctl.c   |  56 --
 23 files changed, 643 insertions(+), 84 deletions(-)
 create mode 100644 security/integrity/ima/ima_mok.c


--
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

Re: [PATCH v1 6/7] ima: measure and appraise the IMA policy itself

[Mimi Zohar]

On Thu, 2015-12-17 at 23:03 +0100, Luis R. Rodriguez wrote:
> On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index 8a45576..4d149c9 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char **data)
> > return rc;
> > }
> >  
> > +   if (!S_ISREG(file_inode(file)->i_mode)) {
> > +   rc = -EACCES;
> > +   goto out;
> > +   }
> > +
> > size = i_size_read(file_inode(file));
> > if (size <= 0)
> > goto out;
> 
> This hunk seems to be unrelated to this patch? If so can it be split out?

Yes, sure.   Up to now, 'cat' was used to load the IMA policy.   A lot
of the problems related to opening and reading a file were hidden.  So
besides making sure that only a regular file is opened, what other
things should we be checking?   For example,  do we permit the kernel to
read NFS mounted files?   Should the kernel be limited to opening only
local files?   Answering these questions becomes important as we move to
a single kernel file read function.

> > @@ -232,13 +238,18 @@ int integrity_read_file(const char *path, char **data)
> > goto out;
> > }
> >  
> > -   rc = integrity_kernel_read(file, 0, buf, size);
> > +   rc = ima_read_and_process_file(file, read_func, buf, size);
> > +   if (rc == -EOPNOTSUPP) {
> > +   rc = integrity_kernel_read(file, 0, buf, size);
> > +   if (rc > 0 && rc != size)
> > +   rc = -EIO;
> > +   }
> > if (rc < 0)
> > kfree(buf);
> > -   else if (rc != size)
> > -   rc = -EIO;
> > -   else
> > +   else {
> > +   rc = size;
> > *data = buf;
> > +   }
> >  out:
> > fput(file);
> > return rc;
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 548b258..40a24c3 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -180,6 +180,7 @@ int ima_policy_show(struct seq_file *m, void *v);
> >  #define IMA_APPRAISE_LOG   0x04
> >  #define IMA_APPRAISE_MODULES   0x08
> >  #define IMA_APPRAISE_FIRMWARE  0x10
> > +#define IMA_APPRAISE_POLICY0x20
> >  
> >  #ifdef CONFIG_IMA_APPRAISE
> >  int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > diff --git a/security/integrity/ima/ima_appraise.c 
> > b/security/integrity/ima/ima_appraise.c
> > index b83049b..1e1a759 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -79,6 +79,7 @@ enum integrity_status ima_get_cache_status(struct 
> > integrity_iint_cache *iint,
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> > return iint->ima_read_status;
> > case FILE_CHECK:
> > default:
> 
> Hrm this uses an int for the func.
> 
> > @@ -102,6 +103,7 @@ static void ima_set_cache_status(struct 
> > integrity_iint_cache *iint,
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> > iint->ima_read_status = status;
> > break;
> > case FILE_CHECK:
> 
> This uses an enum.
> 
> > @@ -126,6 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache 
> > *iint, int func)
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> > iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
> > break;
> > case FILE_CHECK:
> 
> This uses an enum.
> 
> All of these have a common set of funcs that will do similar things, what 
> about
> just OR'ing them up in one place? That would make future additions one line
> instead of 3.

Ok

Mimi

--
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

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

On Thu, 2015-12-17 at 22:06 +0100, Luis R. Rodriguez wrote:
> On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8524450..dcd902f 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  
> > @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> > struct firmware_buf *fw_buf)
> > buf = vmalloc(size);
> > if (!buf)
> > return -ENOMEM;
> > -   rc = kernel_read(file, 0, buf, size);
> > -   if (rc != size) {
> > -   if (rc > 0)
> > -   rc = -EIO;
> > +
> > +   rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> > +   if (rc == -EIO)
> > goto fail;
> > +   else if (rc != -EOPNOTSUPP) {
> > +   rc = kernel_read(file, 0, buf, size);
> > +   if (rc != size) {
> > +   if (rc > 0)
> > +   rc = -EIO;
> > +   goto fail;
> > +   }
> > }
> > rc = security_kernel_fw_from_file(file, buf, size);
> > if (rc)
> 
> This is one way, the other way is to generalize the kernel-read from path
> routine. I have some changes which help generalize this routine a bit so
> help on review there would be appreciated. 

Sure.  Where are the patches?

> I'm personally indifferent
> as to needing or not *now* a generic kernel read routine that is shared
> for this purpose *but* since this patch set *also* seems to be adding
> yet-another file reading I'm more inclined to wish for that to be addressed
> now instead.
> 
> Please let me know if this logic is fair.

Commit e3c4abb - "integrity: define a new function
integrity_read_file()" defined a method of reading a file from the
kernel.  It's used to load an x509 key onto the IMA keyring for systems
without an initramfs.   Dmitry's patch, included in this patch set,
calls this function to load the IMA policy as well.  So this patch set
isn't defining a new function for reading a file from the kernel.  It's
using an existing one.

FYI, sound/sound_firmware.c: do_mod_firmware_load() also reads a file.

Mimi

--
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

Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Thu, 2015-12-17 at 14:45 +0800, Dave Young wrote:
> On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> > On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> > > On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
> > > 
> > > [..]
> > > >  #ifdef CONFIG_IMA_APPRAISE
> > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > index b70ada0..18c4a84 100644
> > > > --- a/kernel/kexec_file.c
> > > > +++ b/kernel/kexec_file.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> > > >  
> > > >  static int kexec_calculate_store_digests(struct kimage *image);
> > > >  
> > > > -static int copy_file_from_fd(int fd, void **buf, unsigned long 
> > > > *buf_len)
> > > > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > > > +void **buf, unsigned long *buf_len)
> > > >  {
> > > > struct fd f = fdget(fd);
> > > > int ret;
> > > > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf, 
> > > > unsigned long *buf_len)
> > > > goto out;
> > > > }
> > > >  
> > > > +   ret = ima_read_and_process_file(f.file, read_func, *buf, 
> > > > stat.size);
> > > > +   if (ret != -EOPNOTSUPP)
> > > > +   goto out_free;
> > > > +
> > > 
> > > Hi Mimi,
> > > 
> > > I am wondering why are we designing this function to also read the file.
> > 
> > > Looks like we just want an hook which calls into ima so that ima can
> > > apply its *additional* policies on kernel and initramfs. If caller is
> > > allocating the buffer, then caller can continue to read the file as well.
> > > In fact that simplifies the code. Now caller which need to check
> > > -EOPNOTSUPP and continue to read the file anyway.
> > 
> > IMA is calculating the file hash as it reads the file.  The file hash is
> > then used for normal IMA processing - adding the measurement to the
> > measurement list and verifying the file's integrity.
> > 
> > > IOW, if caller still has to maintain the code to read the file, then it
> > > is better that ima exports a hook which callers calls after reading the
> > > file and ima can do its own verification.
> > 
> > There's no sense in reading the file twice.  The file hash should be
> > calculated as the file is being read.  Either the existing function to
> > read the file needs to support calculating the file hash or it should
> > call IMA.
> 
> Can IMA provide a function to calculate the hash from a buffer?

Yes, I think Dmitry might already have a buffer hash function.  If we
use the buffer hash, we could then move the ima_read_and_process_file()
from before to after the existing file read function.  The
ima_read_and_process_file() would need to be renamed, but the parameters
would remain the same.  I think that would work.

> > 
> > There's a lot of code duplication for reading a file by the kernel (eg.
> > kexec, firmware, kernel modules, ...).   Each place does it just a bit
> > differently than the other.   There should be a single function for
> > reading and calculating the file hash at the same time.
> 
> If you want to address the duplication for reading file, IMHO you can
> introduce a general interface to read file in kernel space. But I do not
> think the reading + calculating only interface is a good way.

Ok.  As described above, the call would read the buffer into memory and
then call IMA to calculate the buffer hash.

(If someone else is interested in getting involved in kernel
development, cleaning up this code duplication is a good, relatively
small, self contained project.)
 
> > 
> > > Also why do we call second parameter as "read_func". I am really not
> > > passing a function read function. May be something lile file_type might
> > > be better.
> > 
> > The read_func identifies the caller of ima_read_and_process_file().
> > The IMA policy would then look like:
> > 
> > measure func=KEXEC_CHECK
> > appraise func=KEXEC_CHECK appraise_type=imasig
> > #
> > measure func=INITRAMFS_CHECK
> > appraise func=INITRAMFS_CHECK appraise_type=imasig
> > #
>

Re: [PATCH v2 3/3] keys, trusted: seal with a TPM2 authorization policy

[Mimi Zohar]

On Sun, 2015-12-13 at 17:42 +0200, Jarkko Sakkinen wrote:
> TPM2 supports authorization policies, which are essentially
> combinational logic statements repsenting the conditions where the data
> can be unsealed based on the TPM state. This patch enables to use
> authorization policies to seal trusted keys.
> 
> Two following new options have been added for trusted keys:
> 
> * 'policydigest=': provide an auth policy digest for sealing.
> * 'policyhandle=': provide a policy session handle for unsealing.
> 
> If 'hash=' option is supplied after 'policydigest=' option, this
> will result an error because the state of the option would become
> mixed.
> 
> Signed-off-by: Jarkko Sakkinen 
> Tested-by: Colin Ian King 
> ---
>  Documentation/security/keys-trusted-encrypted.txt | 34 
> +--
>  drivers/char/tpm/tpm2-cmd.c   | 24 +---
>  include/keys/trusted-type.h   |  4 +++
>  security/keys/trusted.c   | 26 +
>  4 files changed, 70 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/security/keys-trusted-encrypted.txt 
> b/Documentation/security/keys-trusted-encrypted.txt
> index fd2565b..324ddf5 100644
> --- a/Documentation/security/keys-trusted-encrypted.txt
> +++ b/Documentation/security/keys-trusted-encrypted.txt
> @@ -27,20 +27,26 @@ Usage:
>  keyctl print keyid
> 
>  options:
> -   keyhandle= ascii hex value of sealing key default 0x4000 (SRK)
> -   keyauth=ascii hex auth for sealing key default 0x00...i
> -   (40 ascii zeros)
> -   blobauth=  ascii hex auth for sealed data default 0x00...
> -   (40 ascii zeros)
> -   blobauth=  ascii hex auth for sealed data default 0x00...
> -   (40 ascii zeros)
> -   pcrinfo=ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
> -   pcrlock=pcr number to be extended to "lock" blob
> -   migratable= 0|1 indicating permission to reseal to new PCR values,
> -   default 1 (resealing allowed)
> -   hash=  hash algorithm name as a string. For TPM 1.x the only
> -  allowed value is sha1. For TPM 2.x the allowed values
> -   are sha1, sha256, sha384, sha512 and sm3-256.
> +   keyhandle=ascii hex value of sealing key default 0x4000 (SRK)
> +   keyauth=   ascii hex auth for sealing key default 0x00...i
> + (40 ascii zeros)
> +   blobauth= ascii hex auth for sealed data default 0x00...
> + (40 ascii zeros)
> +   blobauth= ascii hex auth for sealed data default 0x00...
> + (40 ascii zeros)
> +   pcrinfo=   ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
> +   pcrlock=   pcr number to be extended to "lock" blob
> +   migratable=   0|1 indicating permission to reseal to new PCR values,
> + default 1 (resealing allowed)
> +   hash= hash algorithm name as a string. For TPM 1.x the only
> + allowed value is sha1. For TPM 2.x the allowed values
> + are sha1, sha256, sha384, sha512 and sm3-256.
> +   policydigest= digest for the authorization policy. must be calculated
> + with the same hash algorithm as specified by the 'hash='
> + option.
> +   policyhandle= handle to an authorization policy session that defines 
> the
> + same policy and with the same hash algorithm as was 
> used to
> + seal the key.
> 
>  "keyctl print" returns an ascii hex copy of the sealed key, which is in 
> standard
>  TPM_STORED_DATA format.  The key length for new keys are always in bytes.
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index d9d0822..45a6340 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>   tpm_buf_append_u8(, payload->migratable);
> 
>   /* public */
> - tpm_buf_append_u16(, 14);
> + if (options->policydigest)
> + tpm_buf_append_u16(, 14 + options->digest_len);
> + else
> + tpm_buf_append_u16(, 14);
> 
>   tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH);
>   tpm_buf_append_u16(, hash);
> - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH);
> - tpm_buf_append_u16(, 0); /* policy digest size */
> +
> + /* policy */
> + if (options->policydigest) {
> + tpm_buf_append_u32(, 0);
> + tpm_buf_append_u16(, options->digest_len);
> + tpm_buf_append(, options->policydigest,
> +options->digest_len);
> + } else {
> + tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH);
> + tpm_buf_append_u16(, 0);
> + }
> +
> + /* 

Re: [PATCH v2 1/3] keys, trusted: fix: *do not* allow duplicate key options

[Mimi Zohar]

On Sun, 2015-12-13 at 17:42 +0200, Jarkko Sakkinen wrote:
> The trusted keys option parsing allows specifying the same option
> multiple times. The last option value specified is used.
> 
> This can be seen as a regression because:
> 
> * No gain.
> * Could be problematic if there is be options dependent on other
>   options.

Thanks, Jarkko.   Although it should be obvious that patch limits the
number of times an option can be specified, you should explicitly
mention it in the patch description.

Mimi

> Reported-by: James Morris James Morris 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  security/keys/trusted.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 903dace..7c183c7 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -736,11 +736,14 @@ static int getoptions(char *c, struct 
> trusted_key_payload *pay,
>   int res;
>   unsigned long handle;
>   unsigned long lock;
> + unsigned long token_mask = 0;
> 
>   while ((p = strsep(, " \t"))) {
>   if (*p == '\0' || *p == ' ' || *p == '\t')
>   continue;
>   token = match_token(p, key_tokens, args);
> + if (test_and_set_bit(token, _mask))
> + return -EINVAL;
> 
>   switch (token) {
>   case Opt_pcrinfo:


--
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

Re: [PATCH v2 2/2] integrity: convert digsig to akcipher api

[Mimi Zohar]

On Sat, 2015-12-12 at 18:26 -0800, Tadeusz Struk wrote:
> Convert asymmetric_verify to akcipher api.
> 
> Signed-off-by: Tadeusz Struk 
> ---
>  security/integrity/Kconfig |1 +
>  security/integrity/digsig_asymmetric.c |   10 +++---
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 73c457b..f0b2463 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -36,6 +36,7 @@ config INTEGRITY_ASYMMETRIC_KEYS
>  select ASYMMETRIC_KEY_TYPE
>  select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
>  select PUBLIC_KEY_ALGO_RSA
> +select CRYPTO_RSA
>  select X509_CERTIFICATE_PARSER
>   help
> This option enables digital signature verification using
> diff --git a/security/integrity/digsig_asymmetric.c 
> b/security/integrity/digsig_asymmetric.c
> index 4fec181..5629372 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -92,13 +92,9 @@ int asymmetric_verify(struct key *keyring, const char *sig,
>   pks.pkey_hash_algo = hdr->hash_algo;
>   pks.digest = (u8 *)data;
>   pks.digest_size = datalen;
> - pks.nr_mpi = 1;
> - pks.rsa.s = mpi_read_raw_data(hdr->sig, siglen);
> -
> - if (pks.rsa.s)
> - ret = verify_signature(key, );
> -
> - mpi_free(pks.rsa.s);
> + pks.s = hdr->sig;

Thanks!  With this change, my system is now able to boot.

Mimi 

> + pks.s_size = siglen;
> + ret = verify_signature(key, );
>   key_put(key);
>   pr_debug("%s() = %d\n", __func__, ret);
>   return ret;

--
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

Re: [PATCH 0/2] crypto: KEYS: convert public key to akcipher api

[Mimi Zohar]

On Thu, 2015-12-10 at 10:39 -0800, Tadeusz Struk wrote:
> Hi Mimi,
> On 12/10/2015 10:25 AM, Mimi Zohar wrote:
> >> This patch set converts the module verification and digital signature
> >> > code to the new akcipher API.
> >> > RSA implementation has been removed from crypto/asymmetric_keys and the
> >> > new API is used for cryptographic primitives.
> >> > There is no need for MPI above the akcipher API anymore.
> >> > Modules can be verified with software as well as HW RSA implementations.
> > With these two patches my system doesn't even boot.  Digging deeper...
> > 
> 
> It needs RSA implementation built-in.
> Could you check if you have CONFIG_CRYPTO_RSA=y

The diff between this config and the previous one:
< CONFIG_CRYPTO_AKCIPHER=y
< CONFIG_CRYPTO_RSA=y
---
> # CONFIG_CRYPTO_RSA is not set

Mimi



--
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

Re: [PATCH v1 7/7] ima: require signed IMA policy

[Mimi Zohar]

On Thu, 2015-12-10 at 21:12 +0200, Petko Manolov wrote:

> On 15-12-08 13:01:24, Mimi Zohar wrote:
> > Require the IMA policy to be signed when additional rules can be added.
> > 
> > Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> > ---
> >  security/integrity/ima/ima_policy.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c 
> > b/security/integrity/ima/ima_policy.c
> > index 87614a6..6248ae23 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -131,6 +131,10 @@ static struct ima_rule_entry default_appraise_rules[] 
> > = {
> > {.action = DONT_APPRAISE, .fsmagic = SELINUX_MAGIC, .flags = 
> > IMA_FSMAGIC},
> > {.action = DONT_APPRAISE, .fsmagic = NSFS_MAGIC, .flags = IMA_FSMAGIC},
> > {.action = DONT_APPRAISE, .fsmagic = CGROUP_SUPER_MAGIC, .flags = 
> > IMA_FSMAGIC},
> > +#ifdef CONFIG_IMA_WRITE_POLICY
> > +   {.action = APPRAISE, .read_func = POLICY_CHECK,
> > +   .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> > +#endif
> 
> The only time this is not going to work is when there is no IMA key in the 
> keyring and there is no default policy so you need to load one at boot time.  
> This case does not make much sense, however, so i assume the patch is fine.

Up to now, the policy could be replaced only once, which was normally
done in the initramfs.  With the ability to extend the IMA policy on a
running system, it is important that these policy extensions be signed.

To clarify, this patch modifies the builtin appraise policy, so that the
subsequent policy needs to be signed.  If the system is not booted with
the built-in appraise policy  (not a good idea), then the policy being
loaded won't need to be signed.

It is safe for the certificate file not to be signed, as the cert itself
must be signed by a key on either of the trusted (eg. system or ima_mok)
keyrings for it to be added to the .ima keyring.

Thank you reviewing the patch and making  sure it doesn't break your
usecase scenario.

Mimi

> >  #ifndef CONFIG_IMA_APPRAISE_SIGNED_INIT
> > {.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .flags = IMA_FOWNER},
> >  #else
> 
> 
>   Petko
> 


--
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

Re: [PATCH 0/2] crypto: KEYS: convert public key to akcipher api

[Mimi Zohar]

On Thu, 2015-12-10 at 14:37 -0500, Mimi Zohar wrote:
> On Thu, 2015-12-10 at 10:39 -0800, Tadeusz Struk wrote:
> > Hi Mimi,
> > On 12/10/2015 10:25 AM, Mimi Zohar wrote:
> > >> This patch set converts the module verification and digital signature
> > >> > code to the new akcipher API.
> > >> > RSA implementation has been removed from crypto/asymmetric_keys and the
> > >> > new API is used for cryptographic primitives.
> > >> > There is no need for MPI above the akcipher API anymore.
> > >> > Modules can be verified with software as well as HW RSA 
> > >> > implementations.
> > > With these two patches my system doesn't even boot.  Digging deeper...
> > > 
> > 
> > It needs RSA implementation built-in.
> > Could you check if you have CONFIG_CRYPTO_RSA=y
> 
> The diff between this config and the previous one:
> < CONFIG_CRYPTO_AKCIPHER=y
> < CONFIG_CRYPTO_RSA=y
> ---
> > # CONFIG_CRYPTO_RSA is not set

FYI, dracut loaded the keys on the IMA keyring properly.  When we try to
pivot root, real root /sbin/init fails appraisal.  The audit subsystem
is showing "invalid-signature".

There's no additional debugging information with the boot command line
options rd.debug or systemd.log_level=debug.

Mimi


--
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

Re: [PATCH 0/2] crypto: KEYS: convert public key to akcipher api

[Mimi Zohar]

On Wed, 2015-12-09 at 15:52 -0800, Tadeusz Struk wrote:
> This patch set converts the module verification and digital signature
> code to the new akcipher API.
> RSA implementation has been removed from crypto/asymmetric_keys and the
> new API is used for cryptographic primitives.
> There is no need for MPI above the akcipher API anymore.
> Modules can be verified with software as well as HW RSA implementations.

With these two patches my system doesn't even boot.  Digging deeper...

Mimi

> Patches generated against cryptodev-2.6
> ---
> 
> Tadeusz Struk (2):
>   crypto: KEYS: convert public key to the akcipher api
>   integrity: convert digsig to akcipher api
> 
> 
>  crypto/asymmetric_keys/Kconfig|2 
>  crypto/asymmetric_keys/Makefile   |7 -
>  crypto/asymmetric_keys/pkcs7_parser.c |   12 +-
>  crypto/asymmetric_keys/pkcs7_trust.c  |2 
>  crypto/asymmetric_keys/pkcs7_verify.c |2 
>  crypto/asymmetric_keys/public_key.c   |   64 +++--
>  crypto/asymmetric_keys/public_key.h   |   36 -
>  crypto/asymmetric_keys/rsa.c  |  211 
> +++--
>  crypto/asymmetric_keys/x509_cert_parser.c |   37 +
>  crypto/asymmetric_keys/x509_public_key.c  |   17 +-
>  crypto/asymmetric_keys/x509_rsakey.asn1   |4 -
>  include/crypto/public_key.h   |   49 ++-
>  security/integrity/digsig_asymmetric.c|   10 -
>  13 files changed, 138 insertions(+), 315 deletions(-)
>  delete mode 100644 crypto/asymmetric_keys/public_key.h
>  delete mode 100644 crypto/asymmetric_keys/x509_rsakey.asn1
> 
> --
> TS
> --
> 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-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Mimi Zohar]

On Wed, 2015-12-09 at 16:24 +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote:
> > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote:
> > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> > > > > 
> > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > 
> > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > > > 
> > > > > > > > > > }
> > > > > > > > > > break;
> > > > > > > > > > +   case Opt_policydigest:
> > > > > > > > > > +   if (!tpm2 ||
> > > > > > > > > > +   strlen(args[0].from) != (2 * 
> > > > > > > > > > opt->digest_len))
> > > > > > > > > > +   return -EINVAL;
> > > > > > > > > > +   kfree(opt->policydigest);
> > > > > > > > > > +   opt->policydigest = 
> > > > > > > > > > kzalloc(opt->digest_len,
> > > > > > > > > > +   GFP_KERNEL);
> > 
> > You're allocating the exact amount of storage needed.  There's no reason
> > to use kzalloc here or elsewhere in the patch.
> 
> Yup. I'll change this.
> 
> > > > > > > > > 
> > > > > > > > > Is it correct to kfree opt->policydigest here before 
> > > > > > > > > allocating it?
> > > > > > > > 
> > > > > > > > I think so. The same option might be encountered multiple times.
> > > > > > > 
> > > > > > > This would surely signify an error?
> > > > > > 
> > > > > > I'm following the semantics of other options. That's why I 
> > > > > > implemented
> > > > > > it that way for example:
> > > > > > 
> > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 
> > > > > > keyhandle=0x8000"
> > > > > > 
> > > > > > is perfectly OK. I just thought that it'd be more odd if this option
> > > > > > behaved in a different way...
> > > > > 
> > > > > It seems broken to me -- if you're messing up keyctl commands you 
> > > > > might 
> > > > > want to know about it, but we should remain consistent.
> > > > 
> > > > So should I return error if policyhandle/digest appears a second time? I
> > > > agree that it'd be better to return -EINVAL.
> > > > 
> > > > The existing behavior is such that any option can appear multiple times
> > > > and I chose to be consistent with that.
> > > 
> > > Mimi, David?
> > 
> > I don't have a problem with changing the existing behavior to allow the
> > options to be specified only once.
> 
> I don't think this patch is right place to change the behavior as it
> should be done for other options too.

I think the easiest way of checking if a token has already been seen
would be to define
 a flag and use test_and_set_bit(token, ) after the following code
snippet.

  while ((p = strsep(, " \t"))) {
if (*p == '\0' || *p == ' ' || *p == '\t')
continue;
token = match_token(p, key_tokens, args);

Having a separate patch is probably a good idea.

Mimi

--
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

[PATCH v1 7/7] ima: require signed IMA policy

[Mimi Zohar]

Require the IMA policy to be signed when additional rules can be added.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_policy.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 87614a6..6248ae23 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -131,6 +131,10 @@ static struct ima_rule_entry default_appraise_rules[] = {
{.action = DONT_APPRAISE, .fsmagic = SELINUX_MAGIC, .flags = 
IMA_FSMAGIC},
{.action = DONT_APPRAISE, .fsmagic = NSFS_MAGIC, .flags = IMA_FSMAGIC},
{.action = DONT_APPRAISE, .fsmagic = CGROUP_SUPER_MAGIC, .flags = 
IMA_FSMAGIC},
+#ifdef CONFIG_IMA_WRITE_POLICY
+   {.action = APPRAISE, .read_func = POLICY_CHECK,
+   .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
 #ifndef CONFIG_IMA_APPRAISE_SIGNED_INIT
{.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .flags = IMA_FOWNER},
 #else
-- 
2.1.0

--
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

[PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

Instead of reading the firmware twice, once for measuring/appraising
the firmware and again reading the file contents into memory, this
patch reads the firmware once.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 drivers/base/firmware_class.c | 15 +++
 include/linux/ima.h   |  2 +-
 security/integrity/ima/ima.h  |  2 +-
 security/integrity/ima/ima_appraise.c |  5 -
 security/integrity/ima/ima_main.c |  2 +-
 security/integrity/ima/ima_policy.c   | 23 +++
 security/integrity/integrity.h| 11 ---
 7 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450..dcd902f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
struct firmware_buf *fw_buf)
buf = vmalloc(size);
if (!buf)
return -ENOMEM;
-   rc = kernel_read(file, 0, buf, size);
-   if (rc != size) {
-   if (rc > 0)
-   rc = -EIO;
+
+   rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
+   if (rc == -EIO)
goto fail;
+   else if (rc != -EOPNOTSUPP) {
+   rc = kernel_read(file, 0, buf, size);
+   if (rc != size) {
+   if (rc > 0)
+   rc = -EIO;
+   goto fail;
+   }
}
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 5d83ecf..7bd4e07 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -13,7 +13,7 @@
 #include 
 struct linux_binprm;
 
-enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, IMA_MAX_READ_CHECK};
+enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, FIRMWARE_CHECK, 
IMA_MAX_READ_CHECK};
 
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8dc9077..548b258 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -160,7 +160,7 @@ void ima_free_template_entry(struct ima_template_entry 
*entry);
 const char *ima_d_path(struct path *path, char **pathbuf);
 
 /* IMA policy related functions */
-enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR};
+enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, POST_SETATTR};
 
 int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 int flags);
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index e58df45..b83049b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -77,7 +77,6 @@ enum integrity_status ima_get_cache_status(struct 
integrity_iint_cache *iint,
case MODULE_CHECK:
return iint->ima_module_status;
case FIRMWARE_CHECK:
-   return iint->ima_firmware_status;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
return iint->ima_read_status;
@@ -101,8 +100,6 @@ static void ima_set_cache_status(struct 
integrity_iint_cache *iint,
iint->ima_module_status = status;
break;
case FIRMWARE_CHECK:
-   iint->ima_firmware_status = status;
-   break;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
iint->ima_read_status = status;
@@ -127,8 +124,6 @@ static void ima_cache_flags(struct integrity_iint_cache 
*iint, int func)
iint->flags |= (IMA_MODULE_APPRAISED | IMA_APPRAISED);
break;
case FIRMWARE_CHECK:
-   iint->flags |= (IMA_FIRMWARE_APPRAISED | IMA_APPRAISED);
-   break;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
diff --git a/security/integrity/ima/ima_main.c 
b/security/integrity/ima/ima_main.c
index b8b0b8c..f9206cd 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -375,7 +375,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t 
size)
return -EACCES; /* INTEGRITY_UNKNOWN */
return 0;
}
-   return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, NULL, 0);
+   return 0;
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 6fe0ab9..a65cb2a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -102,7 +102,7 @@ static struct im

[PATCH v1 0/7] ima: measuring/appraising files read by the kernel

[Mimi Zohar]

This patch set closes a number of measurement/appraisal gaps by defining
a generic function named ima_read_and_process_file() for measuring and
appraising files read by the kernel (eg. kexec image and initramfs,
firmware, IMA policy).

To differentiate between callers of ima_read_and_process_file() in the
IMA policy, a new enumeration is defined named ima_read_hooks, which
initially includes KEXEC_CHECK, INITRAMFS_CHECK, FIRMWARE_CHECK, and
POLICY_CHECK.

Changelog v1:
- Instead of ima_read_and_process_file() allocating memory, the caller
allocates and frees the memory.
- Moved the kexec measurement/appraisal call to copy_file_from_fd(). The
same call now measures and appraises both the kexec image and initramfs.
- Support for measuring and appraising the IMA policy.
- Restored the original IMA firmware hook to detect loading unsigned firmware.

Mimi

Dmitry Kasatkin (2):
  ima: separate 'security.ima' reading functionality from collect
  ima: load policy using path

Mimi Zohar (5):
  ima: update appraise flags after policy update completes
  ima: measure and appraise kexec image and initramfs
  ima: measure and appraise firmware (improvement)
  ima: measure and appraise the IMA policy itself
  ima: require signed IMA policy

 Documentation/ABI/testing/ima_policy  |  2 +-
 drivers/base/firmware_class.c | 15 +--
 include/linux/ima.h   | 12 +
 kernel/kexec_file.c   | 28 +++-
 security/integrity/digsig.c   |  2 +-
 security/integrity/iint.c | 24 +++---
 security/integrity/ima/ima.h  | 24 +-
 security/integrity/ima/ima_api.c  | 51 +++--
 security/integrity/ima/ima_appraise.c | 40 +++--
 security/integrity/ima/ima_crypto.c   | 56 
 security/integrity/ima/ima_fs.c   | 45 ++-
 security/integrity/ima/ima_init.c |  2 +-
 security/integrity/ima/ima_main.c | 55 ++-
 security/integrity/ima/ima_policy.c   | 73 ---
 security/integrity/ima/ima_template.c |  2 -
 security/integrity/ima/ima_template_lib.c |  3 +-
 security/integrity/integrity.h| 14 +++---
 17 files changed, 329 insertions(+), 119 deletions(-)

-- 
2.1.0

--
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

[PATCH v1 2/7] ima: separate 'security.ima' reading functionality from collect

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasat...@samsung.com>

Instead of passing pointers to pointers to ima_collect_measurent() to
read and return the 'security.ima' xattr value, this patch moves the
functionality to the calling process_measurement() to directly read
the xattr and pass only the hash algo to the ima_collect_measurement().

Signed-off-by: Dmitry Kasatkin <d.kasat...@samsung.com>
Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h  | 15 +++
 security/integrity/ima/ima_api.c  | 15 +++
 security/integrity/ima/ima_appraise.c | 25 ++---
 security/integrity/ima/ima_crypto.c   |  2 +-
 security/integrity/ima/ima_init.c |  2 +-
 security/integrity/ima/ima_main.c | 11 +++
 security/integrity/ima/ima_template.c |  2 --
 security/integrity/ima/ima_template_lib.c |  1 -
 8 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 917407f..9ebfec1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "../integrity.h"
 
@@ -140,9 +141,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 int ima_get_action(struct inode *inode, int mask, int function);
 int ima_must_measure(struct inode *inode, int mask, int function);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len);
+   struct file *file, enum hash_algo algo);
 void ima_store_measurement(struct integrity_iint_cache *iint, struct file 
*file,
   const unsigned char *filename,
   struct evm_ima_xattr_data *xattr_value,
@@ -187,8 +186,8 @@ int ima_must_appraise(struct inode *inode, int mask, enum 
ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
   int func);
-void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len,
-  struct ima_digest_data *hash);
+enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
+int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
   struct evm_ima_xattr_data **xattr_value);
 
@@ -220,10 +219,10 @@ static inline enum integrity_status 
ima_get_cache_status(struct integrity_iint_c
return INTEGRITY_UNKNOWN;
 }
 
-static inline void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
-int xattr_len,
-struct ima_digest_data *hash)
+static inline enum hash_algo
+ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
 {
+   return ima_hash_algo;
 }
 
 static inline int ima_read_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1d950fb..e7c7a5d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include 
+
 #include "ima.h"
 
 /*
@@ -188,9 +188,7 @@ int ima_get_action(struct inode *inode, int mask, int 
function)
  * Return 0 on success, error code otherwise
  */
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len)
+   struct file *file, enum hash_algo algo)
 {
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
@@ -201,9 +199,6 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
char digest[IMA_MAX_DIGEST_SIZE];
} hash;
 
-   if (xattr_value)
-   *xattr_len = ima_read_xattr(file->f_path.dentry, xattr_value);
-
if (!(iint->flags & IMA_COLLECTED)) {
u64 i_version = file_inode(file)->i_version;
 
@@ -213,11 +208,7 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
goto out;
}
 
-   /* use default hash algorithm */
-   hash.hdr.algo = ima_hash_algo;
-
-   if (xattr_value)
-   ima_get_hash_algo(*xattr_value, *xattr_len, );
+   hash.hdr.algo = algo;
 
result = ima_calc_file_hash(file, );
if (!result) {
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 1873b55..9c2b46b 100644
--- a/security/integrity/ima/ima_appraise

Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs

[Mimi Zohar]

On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
> 
> [..]
> >  #ifdef CONFIG_IMA_APPRAISE
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index b70ada0..18c4a84 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -18,6 +18,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> >  
> >  static int kexec_calculate_store_digests(struct kimage *image);
> >  
> > -static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > +void **buf, unsigned long *buf_len)
> >  {
> > struct fd f = fdget(fd);
> > int ret;
> > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf, 
> > unsigned long *buf_len)
> > goto out;
> > }
> >  
> > +   ret = ima_read_and_process_file(f.file, read_func, *buf, stat.size);
> > +   if (ret != -EOPNOTSUPP)
> > +   goto out_free;
> > +
> 
> Hi Mimi,
> 
> I am wondering why are we designing this function to also read the file.

> Looks like we just want an hook which calls into ima so that ima can
> apply its *additional* policies on kernel and initramfs. If caller is
> allocating the buffer, then caller can continue to read the file as well.
> In fact that simplifies the code. Now caller which need to check
> -EOPNOTSUPP and continue to read the file anyway.

IMA is calculating the file hash as it reads the file.  The file hash is
then used for normal IMA processing - adding the measurement to the
measurement list and verifying the file's integrity.

> IOW, if caller still has to maintain the code to read the file, then it
> is better that ima exports a hook which callers calls after reading the
> file and ima can do its own verification.

There's no sense in reading the file twice.  The file hash should be
calculated as the file is being read.  Either the existing function to
read the file needs to support calculating the file hash or it should
call IMA.

There's a lot of code duplication for reading a file by the kernel (eg.
kexec, firmware, kernel modules, ...).   Each place does it just a bit
differently than the other.   There should be a single function for
reading and calculating the file hash at the same time.

> Also why do we call second parameter as "read_func". I am really not
> passing a function read function. May be something lile file_type might
> be better.

The read_func identifies the caller of ima_read_and_process_file().
The IMA policy would then look like:

measure func=KEXEC_CHECK
appraise func=KEXEC_CHECK appraise_type=imasig
#
measure func=INITRAMFS_CHECK
appraise func=INITRAMFS_CHECK appraise_type=imasig
#
measure func=FIRMWARE_CHECK
appraise func=FIRMWARE_CHECK appraise_type=imasig
#
measure func=POLICY_CHECK
appraise func=POLICY_CHECK appraise_type=imasig

> So how does this additional IMA specific policies help (on top of existing
> signature verification mechanism we have for kernel). 

The existing kexec signature verification is limited to verifying the
kexec image on x86_64.  It does not verify the kexec image on other
architectures, nor does it verify the initramfs.  The original method
for verifying a files' integrity was IMA.   We have need for verifying
both the kexec image and initramfs.

Mimi

--
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

Re: [RFC] KEYS: Exposing {a,}symmetric key ops to userspace and other bits

[Mimi Zohar]

On Sun, 2015-11-22 at 09:41 -0500, Mimi Zohar wrote:
> On Fri, 2015-11-20 at 11:07 +, David Howells wrote:
> > 
> >  (*) Add Mimi's patches to allow keys/keyrings to be marked undeletable.  
> > This
> >  is for the purpose of creating blacklists and to prevent people from
> >  removing entries in the blacklist.  Note that only the kernel can 
> > create
> >  a blacklist - we don't want userspace generating them as a way to take 
> > up
> >  kernel space.
> > 
> >  I think the right way to do this is to not allow marked keys to be
> >  unlinked from marked keyrings, but to allow marked keys to be unlinked
> >  from ordinary keyrings.
> > 
> >  The reason the 'keep' mark is required on individual keys is to prevent
> >  the keys from being directly revoked, expired or invalidated by keyctl
> >  without reference to the keyring.  Marked keys that are set expirable
> >  when they're created will still expire and be subsequently removed and 
> > if
> >  a marked key or marked keyring loses all its references it still gets
> >  gc'd.
> 
> Agreed.  I'll fix and re-post soon.

In addition to Petko's 3 patches, the ima-keyrings branch
(git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git) 
contains these two patches.

d939a88 IMA: prevent keys on the .ima_blacklist from being removed
77f33b5 KEYS: prevent keys from being removed from specified keyrings

As the IMA patch is dependent on the KEYS patch, do you mind if the KEYS
patch would be upstreamed together with this patch set?

Mimi

--
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

Re: [PATCH v6 0/3] IMA policy read/write and new IMA keyrings;

[Mimi Zohar]

On Wed, 2015-12-02 at 17:47 +0200, Petko Manolov wrote:
> Difference since v5 of the patches:
> 
>  - better description of patch #3;
>  - added missing IMA_DIGSIG_REQUIRED & IMA_PERMIT_DIRECTIO flags;
> 
> This patch-set consists of three separate patches that do the following:
> 
> 1) Allows multiple writes to the IMA policy.  This is considered useful to do 
> in
> a long lived systems with multiple tenants and where reboots are not
> recommended.  The new IMA rules are appended to the existing ones, effectively
> forming a queue.  The code also replaces the mutexes with RCU read locks.
> 
> 2) Adds two more system keyrings - .ima_mok, which is used to create a simple 
> CA
> hierarchy for the trusted IMA keyring and .ima_blacklist, which keeps all
> revoked IMA keys.  When the IMA_TRUSTED_KEYRING is enabled it is impossible to
> import a key into .ima if it has not been signed by a key in either .system or
> .ima_mok keyrings.  Before performing signature checks .ima_blacklist is
> consulted first and if an offending key is found the requested operation is
> rejected.
> 
> 3) Allows reading back the current IMA policy.It is often useful to be able to
> read back the IMA policy.  It is even more important after introducing
> CONFIG_IMA_WRITE_POLICY. This option allows the root user to see the current
> policy rules.

Thank you for the patches.   I've taken the liberty to prefix the patch
names with the subsystem.

IMA: allow reading back the current IMA policy
IMA: create machine owner and blacklist keyrings
IMA: policy can now be updated multiple times

The patches are available from:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
ima-keyrings.

Mimi

--
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

Re: keyring timestamps

[Mimi Zohar]

On Tue, 2015-12-01 at 21:58 +0200, Petko Manolov wrote:

> First off, this is not a real patch rather than my idea in a C form.  I feel 
> uncertain about a few points:
> 
>   0) does keyrings keep a timestamp when created or last updated?  David?
> 
>   1) is jiffies(_64) the best thing to use for timestamping?  
>  sched_clock() is known to stop at suspend/sleep.

"struct key" defines a couple of time stamps, but none of them are what
we need.

union {
time_t  expiry; /* time at which key
expires (or 0) */
time_t  revoked_at; /* time at which key was
revoked */
};
time_t  last_used_at;   /* last time used for
LRU keyring discard */

>   2) the code below is not optimal - it removes the node from the RB tree 
>  and then walks it again to find the right place.  Mimi, any 
>  objections to restructure integrity_inode_get() for speed when 
>  dealing with timestamps?

Instead of removing the iint's from the RB tree, how about just clearing
the cached appraised info in the iint.   Something like: 
iint->flags &= ~(IMA_APPRAISED | IMA_APPRAISED_SUBMASK);

> 0) is crucial.  If there is no such thing as "time of the last update" for 
> keyrings i guess we'll either have to implement it or use another mechanism 
> to 
> get similar result.

For this usecase, we need a timestamp for the last time a key was added
to the keyring.

Mimi


--
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

Re: [PATCH 1/2] KEYS: Reserve an extra certificate symbol for inserting without recompiling

[Mimi Zohar]

On Tue, 2015-11-24 at 16:18 -0500, Mehmet Kayaalp wrote:
> Place a system_extra_cert buffer of configurable size, right after the
> system_certificate_list, so that inserted keys can be readily processed by
> the existing mechanism. Added script takes a key file and a kernel image
> and inserts its contents to the reserved area. The
> system_certificate_list_size is also adjusted accordingly.
> 
> Call the script as:
> 
> scripts/insert-sys-cert -b  -c 
> 
> If vmlinux has no symbol table, supply System.map file with -s flag.
> Subsequent runs replace the previously inserted key, instead of appending
> the new one.
> 
> Signed-off-by: Mehmet Kayaalp <mkaya...@linux.vnet.ibm.com>

Thanks, Mehmet!

Although the expected usecase scenario for inserting certificates in the
image is for modifying an existing kernel image, not for building a
kernel,  you might want to modify "make" to compare the uncompressed and
compressed image time stamps.  If the compressed image time stamp is
earlier than the uncompressed one, output a message indicating the image
changed and compress the image again.

For now, anyone attempting to insert an additional or different
certificate needs to remove the existing compressed image.

Acked-by:  Mimi Zohar <zo...@linux.vnet.ibm.com>

> ---
>  certs/Kconfig   |  16 ++
>  certs/system_certificates.S |  12 ++
>  scripts/.gitignore  |   1 +
>  scripts/Makefile|   1 +
>  scripts/insert-sys-cert.c   | 410 
> 
>  5 files changed, 440 insertions(+)
>  create mode 100644 scripts/insert-sys-cert.c
> 
> diff --git a/certs/Kconfig b/certs/Kconfig
> index b030b9c7ed34..f0f8a4433685 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -39,4 +39,20 @@ config SYSTEM_TRUSTED_KEYS
> form of DER-encoded *.x509 files in the top-level build directory,
> those are no longer used. You will need to set this option instead.
> 
> +config SYSTEM_EXTRA_CERTIFICATE
> + bool "Reserve area for inserting a certificate without recompiling"
> + depends on SYSTEM_TRUSTED_KEYRING
> + help
> +   If set, space for an extra certificate will be reserved in the kernel
> +   image. This allows introducing a trusted certificate to the default
> +   system keyring without recompiling the kernel.
> +
> +config SYSTEM_EXTRA_CERTIFICATE_SIZE
> + int "Number of bytes to reserve for the extra certificate"
> + depends on SYSTEM_EXTRA_CERTIFICATE
> + default 4096
> + help
> +   This is the number of bytes reserved in the kernel image for a
> +   certificate to be inserted.
> +
>  endmenu
> diff --git a/certs/system_certificates.S b/certs/system_certificates.S
> index 9216e8c81764..f82e1b22eac4 100644
> --- a/certs/system_certificates.S
> +++ b/certs/system_certificates.S
> @@ -13,6 +13,18 @@ __cert_list_start:
>   .incbin "certs/x509_certificate_list"
>  __cert_list_end:
> 
> +#ifdef CONFIG_SYSTEM_EXTRA_CERTIFICATE
> + .globl VMLINUX_SYMBOL(system_extra_cert)
> + .size system_extra_cert, CONFIG_SYSTEM_EXTRA_CERTIFICATE_SIZE
> +VMLINUX_SYMBOL(system_extra_cert):
> + .fill CONFIG_SYSTEM_EXTRA_CERTIFICATE_SIZE, 1, 0
> +
> + .globl VMLINUX_SYMBOL(system_extra_cert_used)
> +VMLINUX_SYMBOL(system_extra_cert_used):
> + .int 0
> +
> +#endif /* CONFIG_SYSTEM_EXTRA_CERTIFICATE */
> +
>   .align 8
>   .globl VMLINUX_SYMBOL(system_certificate_list_size)
>  VMLINUX_SYMBOL(system_certificate_list_size):
> diff --git a/scripts/.gitignore b/scripts/.gitignore
> index 1f78169d4254..e063daa3ec4a 100644
> --- a/scripts/.gitignore
> +++ b/scripts/.gitignore
> @@ -13,3 +13,4 @@ sortextable
>  asn1_compiler
>  extract-cert
>  sign-file
> +insert-sys-cert
> diff --git a/scripts/Makefile b/scripts/Makefile
> index fd0d53d4a234..822ab4a6a4aa 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -19,6 +19,7 @@ hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
>  hostprogs-$(CONFIG_ASN1)  += asn1_compiler
>  hostprogs-$(CONFIG_MODULE_SIG)+= sign-file
>  hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
> +hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
> 
>  HOSTCFLAGS_sortextable.o = -I$(srctree)/tools/include
>  HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
> diff --git a/scripts/insert-sys-cert.c b/scripts/insert-sys-cert.c
> new file mode 100644
> index ..8902836c2342
> --- /dev/null
> +++ b/scripts/insert-sys-cert.c
> @@ -0,0 +1,410 @@
> +/* Write the contents of the  into kernel symbol system_extra_cert
> + *
> + * Copyright (C) IBM Corporation, 2015
&

[PATCH 5/5] ima: read firmware only once

[Mimi Zohar]

Instead of reading the firmware twice, once for measuring/appraising
the firmware and again loading it, this patch reads the firmware
once.  This patch removes ima_fw_from_file() and replaces it with a
new hook named ima_read_file_contents().

As ima_read_file_contents() re-appraises the file each time it is
read, there's no need for the firmware specific cache status.  This
patch removes the firmware specific cache status and replaces it
with the generic read status.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 drivers/base/firmware_class.c |  7 ++-
 include/linux/ima.h   | 10 +++---
 security/integrity/ima/ima.h  |  2 +-
 security/integrity/ima/ima_appraise.c |  5 -
 security/integrity/ima/ima_main.c | 10 ++
 security/integrity/ima/ima_policy.c   | 11 +--
 security/integrity/integrity.h| 11 ---
 security/security.c   |  6 +-
 8 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450..54361ea 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -350,7 +351,11 @@ static int fw_get_filesystem_firmware(struct device 
*device,
file = filp_open(path, O_RDONLY, 0);
if (IS_ERR(file))
continue;
-   rc = fw_read_file_contents(file, buf);
+   
+   rc = ima_read_file_contents(file, FIRMWARE_CHECK,
+   >data, >size);
+   if (rc == -EOPNOTSUPP)
+   rc = fw_read_file_contents(file, buf);
fput(file);
if (rc)
dev_warn(device, "firmware, attempted to load %s, but 
failed with error %d\n",
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3f8d130..84a4ef3 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -13,7 +13,7 @@
 #include 
 struct linux_binprm;
 
-enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, IMA_MAX_READ_CHECK};
+enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, FIRMWARE_CHECK, 
IMA_MAX_READ_CHECK};
 
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
@@ -22,6 +22,8 @@ extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
 extern int ima_fw_from_file(struct file *file, char *buf, size_t size);
+extern int ima_read_file_contents(struct file *file, enum ima_read_hooks func,
+ void **buf, size_t *size);
 extern int ima_read_file_from_fd(int fd, enum ima_read_hooks func,
 void **buf, size_t *size);
 
@@ -51,9 +53,11 @@ static inline int ima_module_check(struct file *file)
return 0;
 }
 
-static inline int ima_fw_from_file(struct file *file, char *buf, size_t size)
+static inline int ima_read_file_contents(struct file *file,
+enum ima_read_hooks func,
+void **buf, size_t *size)
 {
-   return 0;
+   return -EOPNOTSUPP;
 }
 
 static inline int ima_read_file_from_fd(int fd, enum ima_read_hooks func,
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3337d14..f7e8a92 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -160,7 +160,7 @@ void ima_free_template_entry(struct ima_template_entry 
*entry);
 const char *ima_d_path(struct path *path, char **pathbuf);
 
 /* IMA policy related functions */
-enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR};
+enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, POST_SETATTR};
 
 int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 int flags);
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index e58df45..b83049b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -77,7 +77,6 @@ enum integrity_status ima_get_cache_status(struct 
integrity_iint_cache *iint,
case MODULE_CHECK:
return iint->ima_module_status;
case FIRMWARE_CHECK:
-   return iint->ima_firmware_status;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
return iint->ima_read_status;
@@ -101,8 +100,6 @@ static void ima_set_cache_status(struct 
integrity_iint_cache *iint,
iint->ima_module_status = status;
break;
case FIRMWARE_CHECK:
-   iint->ima_firmware_status = status;
-   break;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
 

[PATCH 3/5] ima: ignore the kexec cache status

[Mimi Zohar]

Each time kexec loads an image, ignore the kexec cached status
and re-measure/re-appraise the image.  This patch replaces the
iint kexec status with a generic read status in preparation for
measuring/verifying other files.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_api.c  |  1 +
 security/integrity/ima/ima_appraise.c |  6 +++---
 security/integrity/ima/ima_policy.c   |  2 +-
 security/integrity/integrity.h| 10 +-
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 2187cb4..65d8f26 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -253,6 +253,7 @@ int ima_collected_measurement(struct integrity_iint_cache 
*iint,
iint->ima_hash = tmpbuf;
memcpy(iint->ima_hash, hash, length);
iint->version = file_inode(file)->i_version;
+   iint->flags &= ~(IMA_MEASURED | IMA_AUDITED| IMA_READ_APPRAISED);
iint->flags |= IMA_COLLECTED;
result = 0;
 
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index c74c1de..07bc4e4 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -79,7 +79,7 @@ enum integrity_status ima_get_cache_status(struct 
integrity_iint_cache *iint,
case FIRMWARE_CHECK:
return iint->ima_firmware_status;
case KEXEC_CHECK:
-   return iint->ima_kexec_status;
+   return iint->ima_read_status;
case FILE_CHECK:
default:
return iint->ima_file_status;
@@ -103,7 +103,7 @@ static void ima_set_cache_status(struct 
integrity_iint_cache *iint,
iint->ima_firmware_status = status;
break;
case KEXEC_CHECK:
-   iint->ima_kexec_status = status;
+   iint->ima_read_status = status;
break;
case FILE_CHECK:
default:
@@ -128,7 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache 
*iint, int func)
iint->flags |= (IMA_FIRMWARE_APPRAISED | IMA_APPRAISED);
break;
case KEXEC_CHECK:
-   iint->flags |= (IMA_KEXEC_APPRAISED | IMA_APPRAISED);
+   iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
break;
case FILE_CHECK:
default:
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 008693c..4e5aec9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -305,7 +305,7 @@ static int get_subaction(struct ima_rule_entry *rule, int 
func)
case FIRMWARE_CHECK:
return IMA_FIRMWARE_APPRAISE;
case KEXEC_CHECK:
-   return IMA_KEXEC_APPRAISE;
+   return IMA_READ_APPRAISE;
case FILE_CHECK:
default:
return IMA_FILE_APPRAISE;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 56c571e..9a0ea4c 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -49,14 +49,14 @@
 #define IMA_MODULE_APPRAISED   0x8000
 #define IMA_FIRMWARE_APPRAISE  0x0001
 #define IMA_FIRMWARE_APPRAISED 0x0002
-#define IMA_KEXEC_APPRAISE 0x0004
-#define IMA_KEXEC_APPRAISED0x0008
+#define IMA_READ_APPRAISE  0x0004
+#define IMA_READ_APPRAISED 0x0008
 #define IMA_APPRAISE_SUBMASK   (IMA_FILE_APPRAISE | IMA_MMAP_APPRAISE | \
 IMA_BPRM_APPRAISE | IMA_MODULE_APPRAISE | \
-IMA_FIRMWARE_APPRAISE | IMA_KEXEC_APPRAISE)
+IMA_FIRMWARE_APPRAISE | IMA_READ_APPRAISE)
 #define IMA_APPRAISED_SUBMASK  (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
 IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED | \
-IMA_FIRMWARE_APPRAISED | IMA_KEXEC_APPRAISED)
+IMA_FIRMWARE_APPRAISED | IMA_READ_APPRAISED)
 
 enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
@@ -113,7 +113,7 @@ struct integrity_iint_cache {
enum integrity_status ima_bprm_status:4;
enum integrity_status ima_module_status:4;
enum integrity_status ima_firmware_status:4;
-   enum integrity_status ima_kexec_status:4;
+   enum integrity_status ima_read_status:4;
enum integrity_status evm_status:4;
struct ima_digest_data *ima_hash;
 };
-- 
2.1.0

--
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

[PATCH 2/5] ima: measure and appraise kexec image

[Mimi Zohar]

ima_process_measurements() determines whether or not a file is in policy
before calculating the file hash.  Instead of reading the file once for
calculating the file hash and possibly again for loading the file into
memory, this patch defines a new IMA hook named ima_read_file_from_fd()
to read, and at the same time hash, a kexec'ed image.  After reading and
hashing the file, ima_read_file_from_fd() calls ima_process_measurement()
to measure and appraise the file.

This patch defines a new policy "func" named KEXEC_CHECK to measure
and/or appraise the kexec image.

Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/ima_policy  |  2 +-
 include/linux/ima.h   |  6 
 kernel/kexec_file.c   |  9 --
 security/integrity/ima/ima.h  |  7 ++--
 security/integrity/ima/ima_api.c  | 35 +++-
 security/integrity/ima/ima_appraise.c |  8 +
 security/integrity/ima/ima_crypto.c   | 42 ++--
 security/integrity/ima/ima_main.c | 54 +++
 security/integrity/ima/ima_policy.c   |  4 +++
 security/integrity/ima/ima_template_lib.c |  2 +-
 security/integrity/integrity.h|  7 ++--
 11 files changed, 150 insertions(+), 26 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy 
b/Documentation/ABI/testing/ima_policy
index 0a378a8..5ae0be1 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -26,7 +26,7 @@ Description:
option: [[appraise_type=]] [permit_directio]
 
base:   func:= 
[BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
-   [FIRMWARE_CHECK]
+   [FIRMWARE_CHECK] [KEXEC_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
   [[^]MAY_EXEC]
fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 120ccc5..244452b 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,6 +20,7 @@ extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_module_check(struct file *file);
 extern int ima_fw_from_file(struct file *file, char *buf, size_t size);
+extern int ima_read_file_from_fd(int fd, void **buf, size_t *size);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -52,6 +53,11 @@ static inline int ima_fw_from_file(struct file *file, char 
*buf, size_t size)
return 0;
 }
 
+static inline int ima_read_file_from_fd(int fd, void **buf, size_t *size)
+{
+   return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b70ada0..fd703b8 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -181,15 +182,17 @@ kimage_file_prepare_segments(struct kimage *image, int 
kernel_fd, int initrd_fd,
int ret = 0;
void *ldata;
 
-   ret = copy_file_from_fd(kernel_fd, >kernel_buf,
-   >kernel_buf_len);
+   ret = ima_read_file_from_fd(kernel_fd, >kernel_buf,
+   >kernel_buf_len);
+   if (ret == -EOPNOTSUPP)
+   ret = copy_file_from_fd(kernel_fd, >kernel_buf,
+   >kernel_buf_len);
if (ret)
return ret;
 
/* Call arch image probe handlers */
ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
image->kernel_buf_len);
-
if (ret)
goto out;
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d46bc6b..3389bb3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -106,7 +106,8 @@ int ima_fs_init(void);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
   const char *op, struct inode *inode,
   const unsigned char *filename);
-int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
+int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash,
+  void **buf, unsigned long *buf_len);
 int ima_calc_field_array_hash(struct ima_field_data *field_data,
  struct ima_template_desc *desc, int num_fields,
  struct ima_digest_data *hash);
@@ -142,6 +143,8 @@ int ima_get_action(struct inode *inode, int mask, int 
function);
 int ima_must_measure(struct inode *inode, int mask, int function);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, enum hash_algo algo

[PATCH 1/5] ima: separate 'security.ima' reading functionality from collect

[Mimi Zohar]

From: Dmitry Kasatkin 

Instead of playing with setting and passing pointers to pointers to the
ima_collect_measurent() to read and return 'security.ima' xattr value,
this patch moves functionality to the calling process_measurement()
to directly read xattr and pass only hash algo to the ima_collect_measurement().
Code looks clearer this way.

Signed-off-by: Dmitry Kasatkin 
---
 security/integrity/ima/ima.h  | 15 +++
 security/integrity/ima/ima_api.c  | 15 +++
 security/integrity/ima/ima_appraise.c | 25 ++---
 security/integrity/ima/ima_crypto.c   |  2 +-
 security/integrity/ima/ima_init.c |  2 +-
 security/integrity/ima/ima_main.c | 11 +++
 security/integrity/ima/ima_template.c |  2 --
 security/integrity/ima/ima_template_lib.c |  1 -
 8 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9e82367..d46bc6b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "../integrity.h"
 
@@ -140,9 +141,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 int ima_get_action(struct inode *inode, int mask, int function);
 int ima_must_measure(struct inode *inode, int mask, int function);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len);
+   struct file *file, enum hash_algo algo);
 void ima_store_measurement(struct integrity_iint_cache *iint, struct file 
*file,
   const unsigned char *filename,
   struct evm_ima_xattr_data *xattr_value,
@@ -183,8 +182,8 @@ int ima_must_appraise(struct inode *inode, int mask, enum 
ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
   int func);
-void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len,
-  struct ima_digest_data *hash);
+enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
+int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
   struct evm_ima_xattr_data **xattr_value);
 
@@ -216,10 +215,10 @@ static inline enum integrity_status 
ima_get_cache_status(struct integrity_iint_c
return INTEGRITY_UNKNOWN;
 }
 
-static inline void ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
-int xattr_len,
-struct ima_digest_data *hash)
+static inline enum hash_algo
+ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
 {
+   return ima_hash_algo;
 }
 
 static inline int ima_read_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1d950fb..e7c7a5d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -18,7 +18,7 @@
 #include 
 #include 
 #include 
-#include 
+
 #include "ima.h"
 
 /*
@@ -188,9 +188,7 @@ int ima_get_action(struct inode *inode, int mask, int 
function)
  * Return 0 on success, error code otherwise
  */
 int ima_collect_measurement(struct integrity_iint_cache *iint,
-   struct file *file,
-   struct evm_ima_xattr_data **xattr_value,
-   int *xattr_len)
+   struct file *file, enum hash_algo algo)
 {
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
@@ -201,9 +199,6 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
char digest[IMA_MAX_DIGEST_SIZE];
} hash;
 
-   if (xattr_value)
-   *xattr_len = ima_read_xattr(file->f_path.dentry, xattr_value);
-
if (!(iint->flags & IMA_COLLECTED)) {
u64 i_version = file_inode(file)->i_version;
 
@@ -213,11 +208,7 @@ int ima_collect_measurement(struct integrity_iint_cache 
*iint,
goto out;
}
 
-   /* use default hash algorithm */
-   hash.hdr.algo = ima_hash_algo;
-
-   if (xattr_value)
-   ima_get_hash_algo(*xattr_value, *xattr_len, );
+   hash.hdr.algo = algo;
 
result = ima_calc_file_hash(file, );
if (!result) {
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 1873b55..9c2b46b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -15,7 +15,6 @@
 

Re: [PATCH v5 2/3] Create IMA machine owner and blacklist keyrings;

[Mimi Zohar]

On Mon, 2015-11-02 at 00:32 +0200, Petko Manolov wrote:
> This option creates IMA MOK and blacklist keyrings.  IMA MOK is an
> intermediate keyring that sits between .system and .ima keyrings,
> effectively forming a simple CA hierarchy.  To successfully import a key
> into .ima_mok it must be signed by a key which CA is in .system keyring.
> On turn any key that needs to go in .ima keyring must be signed by CA in
> either .system or .ima_mok keyrings. IMA MOK is empty at kernel boot.
> 
> IMA blacklist keyring contains all revoked IMA keys.  It is consulted
> before any other keyring.  If the search is successful the requested
> operation is rejected and error is returned to the caller.

Adding a key to the black list will prevent any new file integrity
verification from succeeding.  This does not address files that have
already been verified and the results are already in the iint cache.

The iint is a red-black tree.  One method would be to walk the entire
tree clearing the cache info.  Another method would be to include a
timestamp in the iint and lazily clear the iint cache info when next
accessed.  Are there any other solutions?

Mimi


> Signed-off-by: Petko Manolov 
> ---
>  crypto/asymmetric_keys/x509_public_key.c |  2 ++
>  include/keys/system_keyring.h| 24 ++
>  security/integrity/digsig_asymmetric.c   | 14 +
>  security/integrity/ima/Kconfig   | 18 +++
>  security/integrity/ima/Makefile  |  1 +
>  security/integrity/ima/ima_mok.c | 54 
> 
>  6 files changed, 113 insertions(+)
>  create mode 100644 security/integrity/ima/ima_mok.c
> 
> diff --git a/crypto/asymmetric_keys/x509_public_key.c 
> b/crypto/asymmetric_keys/x509_public_key.c
> index 1970966..66dcf30 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -319,6 +319,8 @@ static int x509_key_preparse(struct key_preparsed_payload 
> *prep)
>   goto error_free_cert;
>   } else if (!prep->trusted) {
>   ret = x509_validate_trust(cert, get_system_trusted_keyring());
> + if (ret)
> + ret = x509_validate_trust(cert, get_ima_mok_keyring());
>   if (!ret)
>   prep->trusted = 1;
>   }
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index b20cd88..39fd38c 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -35,4 +35,28 @@ extern int system_verify_data(const void *data, unsigned 
> long len,
> enum key_being_used_for usage);
>  #endif
> 
> +#ifdef CONFIG_IMA_MOK_KEYRING
> +extern struct key *ima_mok_keyring;
> +extern struct key *ima_blacklist_keyring;
> +
> +static inline struct key *get_ima_mok_keyring(void)
> +{
> + return ima_mok_keyring;
> +}
> +static inline struct key *get_ima_blacklist_keyring(void)
> +{
> + return ima_blacklist_keyring;
> +}
> +#else
> +static inline struct key *get_ima_mok_keyring(void)
> +{
> + return NULL;
> +}
> +static inline struct key *get_ima_blacklist_keyring(void)
> +{
> + return NULL;
> +}
> +#endif /* CONFIG_IMA_MOK_KEYRING */
> +
> +
>  #endif /* _KEYS_SYSTEM_KEYRING_H */
> diff --git a/security/integrity/digsig_asymmetric.c 
> b/security/integrity/digsig_asymmetric.c
> index 4fec181..5ade2a7 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "integrity.h"
> 
> @@ -32,9 +33,22 @@ static struct key *request_asymmetric_key(struct key 
> *keyring, uint32_t keyid)
> 
>   pr_debug("key search: \"%s\"\n", name);
> 
> + key = get_ima_blacklist_keyring();
> + if (key) {
> + key_ref_t kref;
> +
> + kref = keyring_search(make_key_ref(key, 1),
> +  _type_asymmetric, name);
> + if (!IS_ERR(kref)) {
> + pr_err("Key '%s' is in ima_blacklist_keyring\n", name);
> + return ERR_PTR(-EKEYREJECTED);
> + }
> + }
> +
>   if (keyring) {
>   /* search in specific keyring */
>   key_ref_t kref;
> +
>   kref = keyring_search(make_key_ref(keyring, 1),
> _type_asymmetric, name);
>   if (IS_ERR(kref))
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 59e215f..a489340 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -142,6 +142,24 @@ config IMA_TRUSTED_KEYRING
>  This option requires that all keys added to the .ima
>  keyring be signed by a key on the system trusted keyring.
> 
> +config IMA_MOK_KEYRING
> + bool "Create IMA machine owner keys (MOK) and blacklist keyrings"
> + select SYSTEM_TRUSTED_KEYRING

Re: [PATCH v5 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-11-10 at 18:01 +0200, Petko Manolov wrote:
> On 15-11-09 09:30:58, Mimi Zohar wrote:
> > On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
> > 
> > > +
> > > +#ifdef   CONFIG_IMA_READ_POLICY
> > > +enum {
> > > + mask_err = -1,
> > > + mask_exec = 1, mask_write, mask_read, mask_append
> > > +};
> > > +
> > > +static match_table_t mask_tokens = {
> > > + {mask_exec, "MAY_EXEC"},
> > > + {mask_write, "MAY_WRITE"},
> > > + {mask_read, "MAY_READ"},
> > > + {mask_append, "MAY_APPEND"},
> > > + {mask_err, NULL}
> > > +};
> > > +
> > > +enum {
> > > + func_err = -1,
> > > + func_file = 1, func_mmap, func_bprm,
> > > + func_module, func_firmware, func_post
> > > +};
> > > +
> > > +static match_table_t func_tokens = {
> > > + {func_file, "FILE_CHECK"},
> > > + {func_mmap, "MMAP_CHECK"},
> > > + {func_bprm, "BPRM_CHECK"},
> > > + {func_module, "MODULE_CHECK"},
> > > + {func_firmware, "FIRMWARE_CHECK"},
> > > + {func_post, "POST_SETATTR"},
> > > + {func_err, NULL}
> > > +};
> > 
> > Why are we using match_table_t?  Why not define an array of strings which 
> > corresponds to the function hooks or use the __stringify macro?
> 
> Because you used match_table_t in your code.  Having too many style 
> differences 
> does not help it's readability.

We're using match_token() for parsing the policy, which requires
match_table_t.  

> > static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
> > "BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
> > 
> > In the first case, to display the function hook string would be 
> > "ima_hooks_string[func]".  Using __stringify requires the hook name (eg. 
> > __stringify(FILE_CHECK)).
> > 
> > In either case, there would be a lot less code.
> 
> It is always speed vs size.  It is going to be more code or more data.  It is 
> a 
> matter of taste which one we'll chose.
> 
> If we look at ima_policy.c after the original IMA policy read patch we'll end 
> up 
> with a source that is littered with strings like "fowner=%s", "fowner" and 
> "fowner=%d".  I assumed you want this cleaned up, which i did.
> 
> Another example, "FILE_CHECK" was used just once prior to introducing of 
> policy 
> read.  Now we use it twice.  Do we want to rely on GCC literals optimization 
> and 
> use the same string multiple times or hand-optimize it?  What do we do with 
> almost the same strings like "fowner=%d", "fowner=%s" and "fowner"?
> 
> The answer to the above will determine whether we use an array of strings or 
> __stringify.  I kind of lean towards the first option but as a maintainer the 
> choice is up to you.

>From what I've seen playing with __stringify, it has its own drawbacks.
I would probably use an enumeration of strings arrays and remove the
switch/case statements as much as possible.

Mimi

--
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

Re: [PATCH v5 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:

> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +enum {
> + mask_err = -1,
> + mask_exec = 1, mask_write, mask_read, mask_append
> +};
> +
> +static match_table_t mask_tokens = {
> + {mask_exec, "MAY_EXEC"},
> + {mask_write, "MAY_WRITE"},
> + {mask_read, "MAY_READ"},
> + {mask_append, "MAY_APPEND"},
> + {mask_err, NULL}
> +};
> +
> +enum {
> + func_err = -1,
> + func_file = 1, func_mmap, func_bprm,
> + func_module, func_firmware, func_post
> +};
> +
> +static match_table_t func_tokens = {
> + {func_file, "FILE_CHECK"},
> + {func_mmap, "MMAP_CHECK"},
> + {func_bprm, "BPRM_CHECK"},
> + {func_module, "MODULE_CHECK"},
> + {func_firmware, "FIRMWARE_CHECK"},
> + {func_post, "POST_SETATTR"},
> + {func_err, NULL}
> +};

Why are we using match_table_t?  Why not define an array of strings
which corresponds to the function hooks or use the __stringify macro?

static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
"BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};

In the first case, to display the function hook string would be
"ima_hooks_string[func]".  Using __stringify requires the hook name (eg.
__stringify(FILE_CHECK)).

In either case, there would be a lot less code.

Mimi

--
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

Re: [PATCHv3 0/6] integrity: few EVM patches

[Mimi Zohar]

On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote:
> Hi,
> 
> IMA module provides functionality to load x509 certificates into the
> trusted '.ima' keyring. This is patchset adds the same functionality
> to the EVM as well. Also it provides functionality to set EVM key from
> the kernel crypto HW driver. This is an update for the patchset which was
> previously sent for review few months ago. Please refer to the patch
> descriptions for details.

Other than patch "evm: define EVM key max and min sizes", which prevents
existing EVM keys from being loaded, the patches are queued
http://git.kernel.org/cgit/linux/kernel/git/zohar/linux-integrity.git/next-for-4.5.

Thanks!

Mimi

> BR,
>  
> Dmitry
> 
> Dmitry Kasatkin (6):
>   integrity: define '.evm' as a builtin 'trusted' keyring
>   evm: load x509 certificate from the kernel
>   evm: enable EVM when X509 certificate is loaded
>   evm: provide a function to set EVM key from the kernel
>   evm: define EVM key max and min sizes
>   evm: reset EVM status when file attributes changes
> 
>  include/linux/evm.h | 10 +++
>  security/integrity/Kconfig  | 11 
>  security/integrity/digsig.c | 14 --
>  security/integrity/evm/Kconfig  | 17 
>  security/integrity/evm/evm.h|  3 +++
>  security/integrity/evm/evm_crypto.c | 54 
> ++---
>  security/integrity/evm/evm_main.c   | 32 +++---
>  security/integrity/evm/evm_secfs.c  | 12 +++--
>  security/integrity/iint.c   |  1 +
>  security/integrity/ima/Kconfig  |  5 +++-
>  security/integrity/ima/ima.h| 12 -
>  security/integrity/ima/ima_init.c   |  2 +-
>  security/integrity/integrity.h  | 13 ++---
>  13 files changed, 146 insertions(+), 40 deletions(-)
> 


--
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

Re: [PATCH v2 1/3] keys, trusted: select the hash algorithm

[Mimi Zohar]

On Tue, 2015-11-03 at 09:39 +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> > On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> > 
> > > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct 
> > > trusted_key_payload *pay,
> > >   return -EINVAL;
> > >   opt->pcrlock = lock;
> > >   break;
> > > + case Opt_hash:
> > > + for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > + if (!strcmp(args[0].from, hash_algo_name[i])) {
> > > + opt->hash = i;
> > > + break;
> > > + }
> > > + }
> > > + res = tpm_is_tpm2(TPM_ANY_NUM);
> > 
> > While looking at this, I wanted to verify that chips are still added to
> > the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> > two-phase chip management functions" reverted David Howell's commit
> > "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> > inadvertent change of dev".
> > 
> > > + if (res < 0)
> > > + return res;
> > > + if (i == HASH_ALGO__LAST ||
> > > + (!res && i != HASH_ALGO_SHA1))
> > > + return -EINVAL;
> > > + break;
> > 
> > If the first TPM registered is a TPM 1.2, then changing the default TPM
> > 2.0 hash algorithm will fail.
> 
> Now that we are going fix this issue in 4.3 and 4.4 do you find this
> patch otherwise acceptable?
> 
> PS. In other options that we don't support in TPM2 I'm planning to
> submit a fix that they will return -EINVAL (like pcrinfo).

I don't have a problem failing the request, but I do suggest adding some
sort of error message.  Different systems might behavior differently
without any explanation.

Mimi

--
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

Re: [PATCH v2 1/3] keys, trusted: select the hash algorithm

[Mimi Zohar]

On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:

> @@ -787,6 +791,20 @@ static int getoptions(char *c, struct 
> trusted_key_payload *pay,
>   return -EINVAL;
>   opt->pcrlock = lock;
>   break;
> + case Opt_hash:
> + for (i = 0; i < HASH_ALGO__LAST; i++) {
> + if (!strcmp(args[0].from, hash_algo_name[i])) {
> + opt->hash = i;
> + break;
> + }
> + }
> + res = tpm_is_tpm2(TPM_ANY_NUM);

While looking at this, I wanted to verify that chips are still added to
the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
two-phase chip management functions" reverted David Howell's commit
"770ab65 TPM: Add new TPMs to the tail of the list to prevent
inadvertent change of dev".

> + if (res < 0)
> + return res;
> + if (i == HASH_ALGO__LAST ||
> + (!res && i != HASH_ALGO_SHA1))
> + return -EINVAL;
> + break;

If the first TPM registered is a TPM 1.2, then changing the default TPM
2.0 hash algorithm will fail.

Mimi

>   default:
>   return -EINVAL;
>   }
 

--
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

Re: [PATCH RFC] tpm: seal with a policy

[Mimi Zohar]

On Sat, 2015-10-31 at 17:53 +0200, Jarkko Sakkinen wrote:
> Support for sealing with a policy.
> 
> Two new options for trusted keys:
> 
> * 'policydigest=': provide a policydigest for the seal operation.
> * 'policyhandle=': provide handle for a policy session for unsealing.

Please expand the patch description explaining the motivation for these
new options.  In what cases are they needed?  Are they system or session
policies? 

Mimi

> 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  drivers/char/tpm/Kconfig|  1 +
>  drivers/char/tpm/tpm2-cmd.c | 20 +---
>  include/keys/trusted-type.h |  3 +++
>  security/keys/trusted.c | 26 --
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 3b84a8b..bd86261 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -6,6 +6,7 @@ menuconfig TCG_TPM
>   tristate "TPM Hardware Support"
>   depends on HAS_IOMEM
>   select SECURITYFS
> + select CRYPTO_HASH_INFO
>   ---help---
> If you have a TPM security chip in your system, which
> implements the Trusted Computing Group's specification,
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index b08a0b4..6f567c3 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -463,6 +463,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>   return -EINVAL;
>   }
> 
> + if (options->policydigest_len > hash_digest_size[options->hash])
> + return -EINVAL;
> +
>   rc = tpm_buf_init(, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
>   if (rc)
>   return rc;
> @@ -488,8 +491,17 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> 
>   tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH);
>   tpm_buf_append_u16(, hash);
> - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH);
> - tpm_buf_append_u16(, 0); /* policy digest size */
> +
> + if (options->policydigest_len) {
> + tpm_buf_append_u32(, 0);
> + tpm_buf_append_u16(, options->policydigest_len);
> + tpm_buf_append(, options->policydigest,
> +options->policydigest_len);
> + } else {
> + tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH);
> + tpm_buf_append_u16(, 0);
> + }
> +
>   tpm_buf_append_u16(, TPM2_ALG_NULL);
>   tpm_buf_append_u16(, 0);
> 
> @@ -617,7 +629,9 @@ static int tpm2_unseal(struct tpm_chip *chip,
>   return rc;
> 
>   tpm_buf_append_u32(, blob_handle);
> - tpm2_buf_append_auth(, TPM2_RS_PW,
> + tpm2_buf_append_auth(,
> +  options->policyhandle ?
> +  options->policyhandle : TPM2_RS_PW,
>NULL /* nonce */, 0,
>0 /* session_attributes */,
>options->blobauth /* hmac */,
> diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
> index a6a1008..e4beeca 100644
> --- a/include/keys/trusted-type.h
> +++ b/include/keys/trusted-type.h
> @@ -37,6 +37,9 @@ struct trusted_key_options {
>   unsigned char pcrinfo[MAX_PCRINFO_SIZE];
>   int pcrlock;
>   uint32_t hash;
> + uint32_t policydigest_len;
> + unsigned char *policydigest;
> + uint32_t policyhandle;
>  };
> 
>  extern struct key_type key_type_trusted;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 7a87bcd..ea043ff 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -713,6 +713,8 @@ enum {
>   Opt_keyhandle, Opt_keyauth, Opt_blobauth,
>   Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
>   Opt_hash,
> + Opt_policydigest,
> + Opt_policyhandle,
>  };
> 
>  static const match_table_t key_tokens = {
> @@ -726,6 +728,8 @@ static const match_table_t key_tokens = {
>   {Opt_pcrlock, "pcrlock=%s"},
>   {Opt_migratable, "migratable=%s"},
>   {Opt_hash, "hash=%s"},
> + {Opt_policydigest, "policydigest=%s"},
> + {Opt_policyhandle, "policyhandle=%s"},
>   {Opt_err, NULL}
>  };
> 
> @@ -804,6 +808,17 @@ static int getoptions(char *c, struct 
> trusted_key_payload *pay,
>   if (i == HASH_ALGO__LAST ||
>   (!res && i != HASH_ALGO_SHA1))
>   return -EINVAL;
> + case Opt_policydigest:
> + opt->policydigest_len = strlen(args[0].from);
> + opt->policydigest = kstrdup(args[0].from, GFP_KERNEL);
> + if (!opt->policydigest)
> + return -ENOMEM;
> + break;
> + case Opt_policyhandle:
> + res = kstrtoul(args[0].from, 16, );
> + if (res < 0)
> + return -EINVAL;
> + opt->policyhandle = 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Sat, 2015-10-24 at 17:04 +0300, Dmitry Kasatkin wrote:
> On Sat, Oct 24, 2015 at 3:28 PM, Petko Manolov  wrote:
> > On 15-10-23 20:13:41, Dmitry Kasatkin wrote:
> >> On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov  wrote:
> >> >
> >> > I was actually going to get rid of IMA_FS_BUSY.  It is less flexible with
> >> > respect to user-space tools.  If the flag is up then the policy upload 
> >> > will
> >> > fail.  The user script or program must check the error and repeat the
> >> > operation after some time.  Seems kind of pointless to me, unless i am
> >> > missing something.
> >> >
> >> > With a semaphore the second process will block and write the policy once 
> >> > the
> >> > first writer leave the operation.  No need to repeat it unless there's 
> >> > some
> >> > real error.
> >> >
> >> > I was trying to be careful and not break the code in case the new
> >> > functionality is not selected in Kconfig.  However, with your most recent
> >> > patch-set i guess we'll have to revisit a few things. :)
> >>
> >> Obviously in original situation it will be only a "single" policy writer -
> >> which IMA init script or binary. If some other script tries to write 
> >> policy at
> >> the same time I would consider it as someone exploit would trying to do 
> >> nasty
> >> things.
> >
> > You've got a point here.  If we get rid of the busy flag we'll have to 
> > block at
> > open() instead of write() in order to keep the "original" semantics.
> >
> 
> No, that was not my point.
> The point was that there is no another/simultaneous  policy writer in reality.
> 
> >> With possibility to update policy I also do not see any need for multiple
> >> writers, when second waits first to finish update and it is not aware about
> >> coming another update. It would be some integrity manager doing policy 
> >> updates
> >> sequentially from time to time.
> >
> > Nope, that's not the only scenario.  Imagine a machine with multiple 
> > tenants and
> > huge uptime.  Imagine also that these tenants want to run their own 
> > software on
> > this machine.  Policy write may occur at any time.
> >
> 
> No, I cannot imagine that unrelated processes can write/update policy
> simultaneously at any time.
> I can imagine that some device/system management software does policy update.
> 
> May be you could give more exact use case.

His usage of the term "multi-tenant" is not the traditional one of
having to protect one tenant from another tenant, but where all tenants
are equally trusted.  Each tenant is allowed to update their own keys on
the .ima_mok/.ima keyrings system and append new rules to the policy.
The tenants are protecting against a file of unknown origin from being
executed, or possibly accessed.

Think of a system owned by a company/university with different
departments all wanting to run code on it.  Not only does the system
owner not want to get involved each time a new piece of code has to be
signed, but wants to easily be able to associate the code on the system
with a dept.  So the system owner delegates authority to the different
departments by signing their certificate.

Mimi

--
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

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Tue, 2015-10-27 at 00:03 +0200, Petko Manolov wrote:
> On 15-10-26 22:39:28, Dmitry Kasatkin wrote:

> > Can you please still explain when multiple policy writers can content? I 
> > 100% 
> > understand the role of mutex
> 
> Ignore the high level requirements for the moment.  Every time you have a 
> contended resource you need to protect it from concurrent writers.  IMA 
> policy 
> is read way more frequently than it is been written.  Just once in the past, 
> now 
> a few times more.

Right.  We all agree that only one process can append new rules at a
time.  The open currently fails with -EBUSY.  If the policy isn't being
updated frequently and there isn't any contention for writing the
policy, the question is why change the existing behavior (by defining a
new mutex)?

Mimi

--
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

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Mon, 2015-10-26 at 16:01 +0200, Petko Manolov wrote:
> On 15-10-25 07:50:32, Mimi Zohar wrote:
> > On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> > 
> > > > @@ -171,9 +172,8 @@ static int __init 
> > > > default_appraise_policy_setup(char *str)
> > > >  __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > > >
> > > >  /*
> > > > - * Although the IMA policy does not change, the LSM policy can be
> > > > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > > > - * stale LSM policy.
> > 
> > Please don't remove this comment.
> 
> OK, i will leave it, but the beginning of the first sentence should be 
> changed 
> to reflect that the IMA policy _may_ change.

Right, please start the sentence with "The LSM policy can be ...".

> 
> > > > + * Blocking here is not legal as we hold an RCU lock.  
> > > > ima_update_policy()
> > > > + * should make it safe to walk the list at any time.
> > > >   
> > 
> > I'm not sure this comment is needed.  If it is needed, this should be moved 
> > to 
> > below the function description.
> 
> From personal experience, i like it when i see such comments, especially at 
> critical code section.  I'll move it down below if you don't like it there.

I prefer to start with the reason for the function.

> > > >   * Update the IMA LSM based rules to reflect the reloaded LSM policy.
> > > >   * We assume the rules still exist; and BUG_ON() if they don't.
> > > > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > > > int result;
> > > > int i;
> > > >
> > > > -   mutex_lock(_rules_mutex);
> > > > list_for_each_entry_safe(entry, tmp, _policy_rules, list) {
> > > 
> > > 
> > > Use of "list_for_each_entry_safe" makes me having a doubt.
> > > 
> > > "safe" versions are used when entries are removed while walk.
> > > 
> > > If it is so, then entire RCU case is impossible?
> > 
> > Taking the lock here was to prevent modifying the LSM rule number multiple 
> > times.  Using the "safe" version was never needed.  The worst that can 
> > happen 
> > here is that the LSM rule number is updated multiple times.
> 
> "safe" is a remnant from the original version of the code.  I don't think it 
> is 
> necessary, but didn't want to make too many changes in one go.  In the worst 
> case the list traversal is a bit slower, which should not happen too often 
> anyway.

Right, this was my mistake.  Please feel free to change it.

Mimi

--
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

Re: [PATCH] keys, trusted: select TPM2 hash algorithm

[Mimi Zohar]

On Sat, 2015-10-24 at 15:42 +0300, Jarkko Sakkinen wrote:
> Added 'hashalg=' option for selecting the hash algorithm.
> 
> Currently available options are:
> 
> * sha1
> * sha256
> * sha384
> * sha512
> * sm3_256

Please consider using crypto/hash_info.c: hash_algo_name[], which
already define the algorithm string names.  Use
include/crypto/hash_info.c to include a reference to this array.

Boot command line options should be prefixed with the subsystem name.
So instead of hashalg, please use tpm_hashalg.  The boot command line
option needs to be documented in Documentation/kernel-parameters.txt.

Mimi

--
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

Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring

[Mimi Zohar]

On Fri, 2015-10-23 at 16:05 +0300, Petko Manolov wrote:
> On 15-10-22 21:49:25, Dmitry Kasatkin wrote:

> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index df30334..a292b88 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -123,14 +123,17 @@ config IMA_APPRAISE
> >   If unsure, say N.
> >  
> >  config IMA_TRUSTED_KEYRING
> > -   bool "Require all keys on the .ima keyring be signed"
> > +   bool "Require all keys on the .ima keyring be signed (deprecated)"
> > depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> > depends on INTEGRITY_ASYMMETRIC_KEYS
> > +   select INTEGRITY_TRUSTED_KEYRING
> > default y
> > help
> >This option requires that all keys added to the .ima
> >keyring be signed by a key on the system trusted keyring.
> >  
> > +  This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> > +
> >  config IMA_LOAD_X509
> > bool "Load X509 certificate onto the '.ima' trusted keyring"
> > depends on IMA_TRUSTED_KEYRING
> 
> 
> I guess we may as well remove this switch.  Otherwise somebody have to 
> remember 
> to post a patch that does so a few kernel releases after this one goes 
> mainline.

There's no harm in leaving the "IMA_TRUSTED_KEYRING" Kconfig option for
a couple of releases (or perhaps until it is enabled in a long term
release), so that the INTEGRITY_TRUSTED_KEYRING Kconfig option is
enabled automatically.

Mimi

--
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

Re: [PATCHv3 4/6] evm: provide a function to set EVM key from the kernel

[Mimi Zohar]

On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote:
> Crypto HW kernel module can possibly initialize EVM key from the
> kernel __init code to enable EVM before calling 'init' process.
> This patch provide a function evm_set_key() which can be used to
> set custom key directly to EVM without using KEY subsystem.

Thanks, Dmitry.  There's a minor comment inline.
> 
> Changes in v3:
> * error reporting moved to evm_set_key
> * EVM_INIT_HMAC moved to evm_set_key
> * added bitop to prevent key setting race
> 
> Changes in v2:
> * use size_t for key size instead of signed int
> * provide EVM_MAX_KEY_SIZE macro in 
> * provide EVM_MIN_KEY_SIZE macro in 
> 
> Signed-off-by: Dmitry Kasatkin 
> ---
>  include/linux/evm.h |  7 ++
>  security/integrity/evm/evm_crypto.c | 47 
> +++--
>  security/integrity/evm/evm_secfs.c  | 10 +++-
>  3 files changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 1fcb88c..35ed9a8 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -14,6 +14,7 @@
>  struct integrity_iint_cache;
> 
>  #ifdef CONFIG_EVM
> +extern int evm_set_key(void *key, size_t keylen);
>  extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>const char *xattr_name,
>void *xattr_value,
> @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname)
>  }
>  #endif
>  #else
> +
> +static inline int evm_set_key(void *key, size_t keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> +
>  #ifdef CONFIG_INTEGRITY
>  static inline enum integrity_status evm_verifyxattr(struct dentry *dentry,
>   const char *xattr_name,
> diff --git a/security/integrity/evm/evm_crypto.c 
> b/security/integrity/evm/evm_crypto.c
> index 34e1a6f..7aec93e 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include "evm.h"
> @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm;
> 
>  static DEFINE_MUTEX(mutex);
> 
> +#define EVM_SET_KEY_BUSY 0
> +
> +static unsigned long evm_set_key_flags;
> +
> +/* evm_set_key - set EVM HMAC key from the kernel
> + *

For exported functions this should be "kernel-doc" format.

Mimi  

> + * This function allows to set EVM HMAC key from the kernel
> + * without using key subsystem 'encrypted' keys. It can be used
> + * by the crypto HW kernel module which has own way of managing
> + * keys.
> + *
> + * key length should be between 32 and 128 bytes long
> + */
> +int evm_set_key(void *key, size_t keylen)
> +{
> + int rc;
> +
> + rc = -EBUSY;
> + if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags))
> + goto busy;
> + rc = -EINVAL;
> + if (keylen > MAX_KEY_SIZE)
> + goto inval;
> + memcpy(evmkey, key, keylen);
> + evm_initialized |= EVM_INIT_HMAC;
> + pr_info("key initialized\n");
> + return 0;
> +inval:
> + clear_bit(EVM_SET_KEY_BUSY, _set_key_flags);
> +busy:
> + pr_err("key initialization failed\n");
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(evm_set_key);
> +
>  static struct shash_desc *init_desc(char type)
>  {
>   long rc;
> @@ -242,7 +278,7 @@ int evm_init_key(void)
>  {
>   struct key *evm_key;
>   struct encrypted_key_payload *ekp;
> - int rc = 0;
> + int rc;
> 
>   evm_key = request_key(_type_encrypted, EVMKEY, NULL);
>   if (IS_ERR(evm_key))
> @@ -250,12 +286,9 @@ int evm_init_key(void)
> 
>   down_read(_key->sem);
>   ekp = evm_key->payload.data;
> - if (ekp->decrypted_datalen > MAX_KEY_SIZE) {
> - rc = -EINVAL;
> - goto out;
> - }
> - memcpy(evmkey, ekp->decrypted_data, ekp->decrypted_datalen);
> -out:
> +
> + rc = evm_set_key(ekp->decrypted_data, ekp->decrypted_datalen);
> +
>   /* burn the original key contents */
>   memset(ekp->decrypted_data, 0, ekp->decrypted_datalen);
>   up_read(_key->sem);
> diff --git a/security/integrity/evm/evm_secfs.c 
> b/security/integrity/evm/evm_secfs.c
> index 3f775df..c8dccd5 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -62,7 +62,7 @@ static ssize_t evm_write_key(struct file *file, const char 
> __user *buf,
>size_t count, loff_t *ppos)
>  {
>   char temp[80];
> - int i, error;
> + int i;
> 
>   if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC))
>   return -EPERM;
> @@ -78,12 +78,8 @@ static ssize_t evm_write_key(struct file *file, const char 
> __user *buf,
>   if ((sscanf(temp, "%d", ) != 1) || (i != 1))
>   return -EINVAL;
> 
> - error = evm_init_key();
> - if (!error) {
> - evm_initialized |= 

Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted

[Mimi Zohar]

On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote:
> Here's a set of patches that changes how keys are determined to be trusted
> - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon
> it.  A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that
> indicates that only keys with this flag set may be added to that keyring.
> 
> Further, any time an X.509 certificate is instantiated without this flag
> set, the certificate is judged against the contents of the system trusted
> keyring to determine whether KEY_FLAG_TRUSTED should be set upon it.
> 
> With these patches, KEY_FLAG_TRUSTED is removed.  The kernel may add
> implicitly trusted keys to a trusted-only keyring by asserting
> KEY_ALLOC_TRUSTED when the key is created, 

Ok, but only the x509 certificates built into the kernel image should be
automatically trusted and can be added to a trusted keyring, because the
kernel itself was signed (and verified).  These certificates extend the
(UEFI) certificate chain of trust that is rooted in hardware to the OS.

Other keys that the kernel reads and loads should not automatically be
trusted (eg. ima_load_x509).  They need to be validated against a
trusted key.

> but otherwise the key will only
> be allowed to be added to the keyring if it can be verified by a key
> already in that keyring.  The system trusted keyring is not then special in
> this sense and other trusted keyrings can be set up that are wholly
> independent of it.

We already went down this path of "transitive trust" back when we first
introduced the concept of trusted keys and keyrings.   Just because a
key is on a trusted keyring, doesn't imply that it should be permitted
to load other keys on the same trusted keyring.  In the case of
IMA-appraisal, the key should only be used to verify the file data
signature, not other keys.

The trusted keys used for verifying other certificates should be stored
on a separate keyring, not the target keyring.   Petko's patches define
a new IMA keyring named .ima_mok for this purpose.

Mimi

> To make this work, we have to retain sufficient data from the X.509
> certificate that we can then verify the signature at need.
> 
> The patches can be found here also:
> 
>   
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-trust
> 
> and are tagged with:
> 
>   keys-trust-20151021
> 


--
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

Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted

[Mimi Zohar]

On Wed, 2015-10-21 at 13:21 -0400, Josh Boyer wrote:
> On Wed, Oct 21, 2015 at 1:02 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> > On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote:
> >> Here's a set of patches that changes how keys are determined to be trusted
> >> - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon
> >> it.  A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that
> >> indicates that only keys with this flag set may be added to that keyring.
> >>
> >> Further, any time an X.509 certificate is instantiated without this flag
> >> set, the certificate is judged against the contents of the system trusted
> >> keyring to determine whether KEY_FLAG_TRUSTED should be set upon it.
> >>
> >> With these patches, KEY_FLAG_TRUSTED is removed.  The kernel may add
> >> implicitly trusted keys to a trusted-only keyring by asserting
> >> KEY_ALLOC_TRUSTED when the key is created,
> >
> > Ok, but only the x509 certificates built into the kernel image should be
> > automatically trusted and can be added to a trusted keyring, because the
> > kernel itself was signed (and verified).  These certificates extend the
> > (UEFI) certificate chain of trust that is rooted in hardware to the OS.
> 
> That doesn't sound accurate to me.  The cert built into the kernel
> image doesn't extend the UEFI certificates.  In most cases, it is a
> ephemeral cert that is automatically generated at kernel build time
> and then discarded.  It is not chained to or derived from any of the
> UEFI certs stored in the db (or mok) variables.  The built-in cert is
> used for module loading verification.  I agree that it should be
> trusted, but not really for the reason you list.  Perhaps you meant
> the key that the PE image of the kernel is signed with?  If so, the
> kernel doesn't load that.  Only shim (and grub2 via shim) read that
> key.

This is similar to the concept of the MoK DB.  Keys added to the MoK
aren't signed by a UEFI key, yet they extend the UEFI secure boot
certificate chain of trust.  Similarly, the certificates built into the
kernel image don't need to be signed by a UEFI/MoK key for it to extend
the certificate chain of trust.

> However, that does bring up the UEFI db/mok certs and how to deal with
> those.  The out-of-tree patches we have add them to the system keyring
> as trusted keys.  We can modify the patches to use KEY_ALLOC_TRUSTED
> to preserve that functionality I suppose.

Certificates are use case specific.  Just because a key was trusted at
the UEFI layer doesn't mean it should be trusted by the kernel (eg.
Microsoft key).  To illustrate this point, David Howells/David Woodhouse
recently posted/upstreamed patches to differentiate how keys loaded onto
the system keyring may be used. (Reference needed.)

Mimi

--
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

Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted

[Mimi Zohar]

On Wed, 2015-10-21 at 14:21 -0400, Josh Boyer wrote:
> On Wed, Oct 21, 2015 at 2:11 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> > On Wed, 2015-10-21 at 13:21 -0400, Josh Boyer wrote:
> >> On Wed, Oct 21, 2015 at 1:02 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> 
> >> wrote:
> >> > On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote:
> >> >> Here's a set of patches that changes how keys are determined to be 
> >> >> trusted
> >> >> - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set 
> >> >> upon
> >> >> it.  A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that
> >> >> indicates that only keys with this flag set may be added to that 
> >> >> keyring.
> >> >>
> >> >> Further, any time an X.509 certificate is instantiated without this flag
> >> >> set, the certificate is judged against the contents of the system 
> >> >> trusted
> >> >> keyring to determine whether KEY_FLAG_TRUSTED should be set upon it.
> >> >>
> >> >> With these patches, KEY_FLAG_TRUSTED is removed.  The kernel may add
> >> >> implicitly trusted keys to a trusted-only keyring by asserting
> >> >> KEY_ALLOC_TRUSTED when the key is created,
> >> >
> >> > Ok, but only the x509 certificates built into the kernel image should be
> >> > automatically trusted and can be added to a trusted keyring, because the
> >> > kernel itself was signed (and verified).  These certificates extend the
> >> > (UEFI) certificate chain of trust that is rooted in hardware to the OS.
> >>
> >> That doesn't sound accurate to me.  The cert built into the kernel
> >> image doesn't extend the UEFI certificates.  In most cases, it is a
> >> ephemeral cert that is automatically generated at kernel build time
> >> and then discarded.  It is not chained to or derived from any of the
> >> UEFI certs stored in the db (or mok) variables.  The built-in cert is
> >> used for module loading verification.  I agree that it should be
> >> trusted, but not really for the reason you list.  Perhaps you meant
> >> the key that the PE image of the kernel is signed with?  If so, the
> >> kernel doesn't load that.  Only shim (and grub2 via shim) read that
> >> key.
> >
> > This is similar to the concept of the MoK DB.  Keys added to the MoK
> > aren't signed by a UEFI key, yet they extend the UEFI secure boot
> > certificate chain of trust.  Similarly, the certificates built into the
> 
> Right, because UEFI is verifying shim, which verifies grub2, which
> verifies the kernel.  I get that.  However, it's irrelevant.
> 
> > kernel image don't need to be signed by a UEFI/MoK key for it to extend
> > the certificate chain of trust.
> 
> The certificates built _into_ the kernel need to be trusted in all
> cases.  It is how module signing is done.  So a user not using Secure
> Boot, or even not using UEFI, still needs those embedded certs trusted
> so that they can load modules.  It has nothing to do with UEFI or some
> single-root-of-trust.
> 
> At any rate, I believe we are both saying the embedded cert needs to
> be trusted so there's little point in debating further.  I just wanted
> to point out that this need has nothing to do with UEFI.

Right, the embedded certs need to trusted.  But that trust needs to be
based on something.  One method of establishing that trust is (UEFI)
secure boot, which verifies the kernel image signature, including the
embedded certs.

Mimi

--
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

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Wed, 2015-10-21 at 11:50 +0100, David Howells wrote:
> Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> 
> > Thinking about the blacklist keyring some more...
> 
> Are we talking about a blacklist keyring that userspace can use - or can it be
> only usable by the kernel?

The blacklist is referenced by the kernel before using a key on either
the .ima or the new .ima_mok keyring to validate a file signature.

> > My concern is more that keys can be added and removed at run time from
> > either of the .ima or the ima_mok keyrings.  The need for a blacklist
> > keyring is to prevent the key from being removed and at a later point
> > re-added.  Unfortunately, keys can be added and removed similarly from the
> > blacklist keyring as well.  Unless keys can be added, without the ability of
> > removing them, I'm not sure of the benefit of a blacklist keyring.  I assume
> > adding and removing keys requires the same write privilege.
> 
> The operations that modify the contents of a keyring in some way (link,
> unlink, clear) all operate under Write privilege.  That said, we could add a
> flag that suppresses unlink and clear on a keyring.  This could also suppress
> garbage collection of revoked and invalidated keys.

Adding the semantics at the keyring level would be better than at the
individual key level.   This new flag would prevent keys on the
blacklist from being removed.  I like this solution.

Mimi

--
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

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Wed, 2015-10-21 at 11:52 +0100, David Howells wrote:
> Petko Manolov  wrote:
> 
> > As far as i know there is no concept of write-once to a keyring in the
> > kernel.  David will correct me if i am wrong.  I wonder how hard would it be
> > to add such functionality, in case it is missing?
> 
> Not hard, particularly if it's only an attribute that the kernel can set.

So the new flag would be set at keyring creation, similarly to
KEY_FLAG_TRUSTED_ONLY.

Mimi


--
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

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Wed, 2015-10-21 at 11:55 +0100, David Howells wrote:
> Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> 
> > > I need to think about this.  Should -EKEYREVOKED be the same as -ENOKEY in
> > > this case?  I guess the end result is pretty much the same from IMA view
> > > point, but there may be a requirement to list all revoked keys...
> > 
> > When checking the blacklist, getting -EKEYREVOKED is definitely
> > different than -ENOKEY.
> 
> Actually, I misspoke earlier.  Revoked keys are only skipped by the search if
> a live key is found.  Should all the keys in the blacklist just be revoked so
> that the search of the list returns either -ENOKEY (no key there) or
> -EKEYREVOKED (the key is blacklisted)?  That might be getting too
> over-complicated though.

Right, your suggestion of adding a new flag on the keyring itself is
definitely preferable.

Mimi

--
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

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> On 15-10-19 14:21:42, Mimi Zohar wrote:
> > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > When in development it is useful to read back the IMA policy.  This patch
> > > provides the functionality.  However, this is a potential security hole so
> > > it should not be used in production-grade kernels.
> >  
> > Like the other IMA securityfs files, only root would be able to read it.
> > Once we start allowing additional rules to be appended to the policy,
> > being able to view the resulting policy is important.  Is there a reason
> > for limiting this option to development?
> 
> I have not considered allowing non-root users to read the policy - i was 
> merely 
> cleaning up the Zbigniew's patch.  I guess it might be useful to be able to 
> read 
> the policy when in development mode.

I guess I wasn't clear.  I don't have a problem with the patch itself,
just with the patch description.  What is this "security hole" that the
option should ONLY be configured for development?Only privileged
users can view the policy.  I don't see the problem with configuring it
in general.  Please remove the comment.

Since responding, I've enabled this feature.  Very nice!

Mimi

--
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

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Tue, 2015-10-20 at 18:33 +0300, Petko Manolov wrote:
> On 15-10-20 11:21:43, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 17:43 +0300, Petko Manolov wrote:

> 
> > Thinking about the blacklist keyring some more...  My concern is more that 
> > keys can be added and removed at run time from either of the .ima or the 
> > ima_mok keyrings.  The need for a blacklist keyring is to prevent the key 
> > from 
> > being removed and at a later point re-added. Unfortunately, keys can be 
> > added 
> > and removed similarly from the blacklist keyring as well.  Unless keys can 
> > be 
> > added, without the ability of removing them, I'm not sure of the benefit of 
> > a 
> > blacklist keyring.  I assume adding and removing keys requires the same 
> > write 
> > privilege.  (cc'ing David Howells)
> 
> As far as i know there is no concept of write-once to a keyring in the 
> kernel.  
> David will correct me if i am wrong.  I wonder how hard would it be to add 
> such 
> functionality, in case it is missing?
> 
> Ideally a revoked key should stay in .blacklist until it expire or the system 
> is 
> rebooted.

Keys currently revoked return -EKEYREVOKED for a certain amount of time,
before being garbage collected.  Perhaps for trusted keys we could piggy
back on this option, returning -EKEYREVOKED, but prevent them from being
garbage collected?

Mimi

--
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

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Tue, 2015-10-20 at 21:42 +0300, Petko Manolov wrote:
> On 15-10-20 14:32:10, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 18:33 +0300, Petko Manolov wrote:
> > > 
> > > As far as i know there is no concept of write-once to a keyring in the 
> > > kernel.  David will correct me if i am wrong.  I wonder how hard would it 
> > > be 
> > > to add such functionality, in case it is missing?
> > > 
> > > Ideally a revoked key should stay in .blacklist until it expire or the 
> > > system is rebooted.
> > 
> > Keys currently revoked return -EKEYREVOKED for a certain amount of time, 
> > before being garbage collected.  Perhaps for trusted keys we could piggy 
> > back 
> > on this option, returning -EKEYREVOKED, but prevent them from being garbage 
> > collected?
> 
> I need to think about this.  Should -EKEYREVOKED be the same as -ENOKEY in 
> this 
> case?  I guess the end result is pretty much the same from IMA view point, 
> but 
> there may be a requirement to list all revoked keys...

When checking the blacklist, getting -EKEYREVOKED is definitely
different than -ENOKEY.

Mimi

--
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

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 15:10 +0300, Petko Manolov wrote:
> On 15-10-20 08:00:29, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> > > On 15-10-19 14:21:42, Mimi Zohar wrote:
> > > > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > > > When in development it is useful to read back the IMA policy.  This 
> > > > > patch
> > > > > provides the functionality.  However, this is a potential security 
> > > > > hole so
> > > > > it should not be used in production-grade kernels.
> > > >  
> > > > Like the other IMA securityfs files, only root would be able to read it.
> > > > Once we start allowing additional rules to be appended to the policy,
> > > > being able to view the resulting policy is important.  Is there a reason
> > > > for limiting this option to development?
> > > 
> > > I have not considered allowing non-root users to read the policy - i was 
> > > merely 
> > > cleaning up the Zbigniew's patch.  I guess it might be useful to be able 
> > > to read 
> > > the policy when in development mode.
> > 
> > I guess I wasn't clear.  I don't have a problem with the patch itself, just 
> > with the patch description.  What is this "security hole" that the option 
> > should ONLY be configured for development?  Only privileged users can view 
> > the 
> > policy.  I don't see the problem with configuring it in general.  Please 
> > remove the comment.
> 
> By "security hole" i mean being able to read it at all.  Root or non-root.  
> Knowing what the IMA policy is may give the attacker an idea how to 
> circumvent 
> it.  I used stronger words in order to attract the user's attention and 
> consider 
> carefully what the implications are when enabling this option.
> 
> However, i do not insist on keeping this comment.  I will remove it or 
> re-word 
> it if you think it is nonsensical in it's present form.
> 
> BTW, i still think it is a good idea that only the root user have access to 
> the 
> IMA policy.  Unless i hear otherwise i am planning to keep the current 
> functionality.

Exactly!  Because only privileged users (eg. root) have access to
securityfs files, I don't see the security concern.

> > Since responding, I've enabled this feature.  Very nice!
> 
> Have you tried it?

Yes, being able to see the existing policy is nice.

BTW, I haven't compared this patch with the original one yet.  Unless
there were so many changes so that it isn't the same patch anymore, the
patch author should be Zbigniew Jasiński <z.jasin...@samsung.com>.  Any
changes you made would be listed in the change log.

Mimi

--
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

[PULL REQUEST] IMA changes for 4.4

[Mimi Zohar]

Hi James,

This pull request is for a single bug fix from Dimtry to properly load
only signed certificates onto the trusted IMA keyring from the kernel.
(This patch has been in the linux-next tree).

thanks,

Mimi

The following changes since commit
049e6dde7e57f0054fdc49102e7ef4830c698b46:

  Linux 4.3-rc4 (2015-10-04 16:57:17 +0100)

are available in the git repository at:


git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next

for you to fetch changes up to 72e1eed8abb11c79749266d433c817ce36732893:

  integrity: prevent loading untrusted certificates on the IMA trusted
keyring (2015-10-09 15:31:18 -0400)


Dmitry Kasatkin (1):
  integrity: prevent loading untrusted certificates on the IMA
trusted keyring

 security/integrity/digsig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
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

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> When in development it is useful to read back the IMA policy.  This patch
> provides the functionality.  However, this is a potential security hole so
> it should not be used in production-grade kernels.
 
Like the other IMA securityfs files, only root would be able to read it.
Once we start allowing additional rules to be appended to the policy,
being able to view the resulting policy is important.  Is there a reason
for limiting this option to development?

(Still reviewing the code ...)

Mimi

> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  |  13 +++
>  security/integrity/ima/ima.h|  13 ++-
>  security/integrity/ima/ima_fs.c |  29 +-
>  security/integrity/ima/ima_policy.c | 190 
> 
>  4 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 235b3c2..040778f 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
> 
> If unsure, say N.
> 
> +config IMA_READ_POLICY
> + bool "Enable reading back the current IMA policy"
> + depends on IMA
> + default n
> + help
> +When developing and debugging an IMA enabled system it is often
> +useful to be able to read the IMA policy.  This patch allows
> +for doing so.  However, being able to read the IMA policy is
> +considered insecure and is strongly discouraged for
> +production-grade kernels.
> +
> +If unsure, say N.
> +
>  config IMA_APPRAISE
>   bool "Appraise integrity measurements"
>   depends on IMA
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e2a60c3..ebc3554 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -166,6 +166,10 @@ void ima_update_policy(void);
>  void ima_update_policy_flag(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> +void ima_policy_stop(struct seq_file *m, void *v);
> +int ima_policy_show(struct seq_file *m, void *v);
> 
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE 0x01
> @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
>   return 0;
>  }
>  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> -#endif
> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +#define  POLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> +#else
> +#define  POLICY_FILE_FLAGS   S_IWUSR
> +#endif /* CONFIG_IMA_WRITE_POLICY */
> +
> +#endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a3cf5c0..85bd129 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -311,14 +311,29 @@ enum ima_fs_flags {
> 
>  static unsigned long ima_fs_flags;
> 
> +#ifdef   CONFIG_IMA_READ_POLICY
> +static const struct seq_operations ima_policy_seqops = {
> + .start = ima_policy_start,
> + .next = ima_policy_next,
> + .stop = ima_policy_stop,
> + .show = ima_policy_show,
> +};
> +#endif
> +
>  /*
>   * ima_open_policy: sequentialize access to the policy file
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> - /* No point in being allowed to open it if you aren't going to write */
> - if (!(filp->f_flags & O_WRONLY))
> + if (!(filp->f_flags & O_WRONLY)) {
> +#ifndef  CONFIG_IMA_READ_POLICY
>   return -EACCES;
> +#else
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return seq_open(filp, _policy_seqops);
> +#endif
> + }
>   if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>   return -EBUSY;
>   return 0;
> @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>   const char *cause = valid_policy ? "completed" : "failed";
> + int policy_write = (file->f_flags & O_WRONLY);
> +
> + if (!policy_write)
> + return 0;
> 
>   pr_info("IMA: policy update %s\n", cause);
>   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>   if (!valid_policy) {
>   ima_delete_rules();
>   valid_policy = 1;
> - clear_bit(IMA_FS_BUSY, _fs_flags);
>   return 0;
>   }
> +
>   ima_update_policy();
>  #ifndef  CONFIG_IMA_WRITE_POLICY
>   securityfs_remove(ima_policy);
> @@ -358,6 +377,7 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
> 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Mon, 2015-10-19 at 14:21 -0400, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:

> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> > 
> >  #include "ima.h"
> > 
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> >  static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > const char __user *buf,
> >  {
> > char *data = NULL;
> > ssize_t result;
> > +   int res;
> > +
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> > 
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_write_mutex);
> > +
> > return result;
> >  }
> > 
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > +   clear_bit(IMA_FS_BUSY, _fs_flags);
> > +#endif
> > return 0;
> >  }
> > 

The IMA_FS_BUSY flag needs to be cleared, like here, above for !
valid_policy.

Mimi

--
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

Re: [PATCH] Introduces generic __list_splice_init_rcu();

[Mimi Zohar]

On Tue, 2015-10-06 at 11:37 -0700, Paul E. McKenney wrote:
> On Sun, Sep 27, 2015 at 06:10:28PM +0300, Petko Manolov wrote:
> > __list_splice_init_rcu() can be used to splice lists forming both stack and
> > queue structures, depending on its arguments.  It is based on the initial
> > list_splice_init_rcu() with a few minor modifications to help abstracting 
> > it.
> > 
> > Signed-off-by: Petko Manolov 
> 
> At first glance, this looks sane.
> 
> But who is using the new list_splice_tail_init_rcu() function?

Hi, Paul.  Up to now a single policy was loaded,  normally in the
initramfs, and never changed.  Petko is adding support to extend the
policy rules.  The additional policies would be appended to the existing
list, only after verifying the new set of rules are good.

list.h contains list_splice() and list_splice_tail(), but the RCU
equivalent functions don't exist.

Mimi

--
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

Re: [PATCH v3 2/2] Adds ima_root_ca keyring;

[Mimi Zohar]

On Thu, 2015-09-10 at 14:17 +0300, Petko Manolov wrote:
> The .system keyring is populated at kernel build time and read-only while the
> system is running.  There is no way to dynamically add other user's CA so
> .ima_root_ca was introduced as read-write keyring that stores these
> certificates.  CA hierarchy is achieved by allowing import of key material 
> that
> has been signed by CA already present in the .system keyring.
> 
> The new .ima_blacklist is a keyring that holds all revoked IMA keys.  It is
> consulted first, then the .ima keyring.

A couple of minor comments inline below...

> 
> Signed-off-by: Petko Manolov 
> ---
>  crypto/asymmetric_keys/x509_public_key.c |  2 ++
>  include/keys/system_keyring.h| 13 
>  security/integrity/digsig_asymmetric.c   | 13 
>  security/integrity/ima/Kconfig   | 11 +++
>  security/integrity/ima/Makefile  |  1 +
>  security/integrity/ima/ima_root_ca.c | 56 
> 
>  security/integrity/integrity.h   | 13 
>  7 files changed, 109 insertions(+)
>  create mode 100644 security/integrity/ima/ima_root_ca.c
> 
> diff --git a/crypto/asymmetric_keys/x509_public_key.c 
> b/crypto/asymmetric_keys/x509_public_key.c
> index 6d88dd1..e39ca38 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -319,6 +319,8 @@ static int x509_key_preparse(struct key_preparsed_payload 
> *prep)
>   goto error_free_cert;
>   } else if (!prep->trusted) {
>   ret = x509_validate_trust(cert, get_system_trusted_keyring());
> + if (ret)
> + ret = x509_validate_trust(cert, 
> get_ima_root_ca_keyring());
>   if (!ret)
>   prep->trusted = 1;
>   }
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index b20cd88..774de6c 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -35,4 +35,17 @@ extern int system_verify_data(const void *data, unsigned 
> long len,
> enum key_being_used_for usage);
>  #endif
> 
> +#ifdef CONFIG_IMA_ROOT_CA_KEYRING
> +extern struct key *ima_root_ca_keyring;
> +static inline struct key *get_ima_root_ca_keyring(void)
> +{
> + return ima_root_ca_keyring;
> +}
> +#else
> +static inline struct key *get_ima_root_ca_keyring(void)
> +{
> + return NULL;
> +}
> +#endif /* CONFIG_IMA_ROOT_CA_KEYRING */
> +
>  #endif /* _KEYS_SYSTEM_KEYRING_H */
> diff --git a/security/integrity/digsig_asymmetric.c 
> b/security/integrity/digsig_asymmetric.c
> index 4fec181..52377d9 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -32,9 +32,22 @@ static struct key *request_asymmetric_key(struct key 
> *keyring, uint32_t keyid)
> 
>   pr_debug("key search: \"%s\"\n", name);
> 
> + key = get_ima_blacklist_keyring();
> + if (key) {
> + key_ref_t kref;
> +
> + kref = keyring_search(make_key_ref(key, 1),
> +  _type_asymmetric, name);
> + if (!IS_ERR(kref)) {
> + pr_err("Key '%s' is in ima_blacklist_keyring\n", name);
> + return ERR_PTR(-EKEYREJECTED);
> + }
> + }
> +
>   if (keyring) {
>   /* search in specific keyring */
>   key_ref_t kref;
> +
>   kref = keyring_search(make_key_ref(keyring, 1),
> _type_asymmetric, name);
>   if (IS_ERR(kref))
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index ebe7a907..69426ce 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -146,6 +146,17 @@ config IMA_TRUSTED_KEYRING
>  This option requires that all keys added to the .ima
>  keyring be signed by a key on the system trusted keyring.
> 
> +config IMA_ROOT_CA_KEYRING
> + bool "Create IMA Root CA keyring"
> + depends on IMA_TRUSTED_KEYRING
> + default y
> + help
> +This option creates IMA Root CA keyring.  This is intermediate
> +keyring which sits between the .system and .ima keyrings, effectively
> +creating a simple CA hierarchy.  All keys in it must be signed either
> +by a key in the .system keyring or one which is already in
> +.ima_root_ca_keyring.
> +
>  config IMA_LOAD_X509
>   bool "Load X509 certificate onto the '.ima' trusted keyring"
>   depends on IMA_TRUSTED_KEYRING
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index d79263d..b2f9aa0 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_IMA) += ima.o
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
>ima_policy.o ima_template.o 

Re: [rfc] [patch] persistent IMA policy file

[Mimi Zohar]

On Sun, 2015-09-27 at 18:23 +0300, Petko Manolov wrote:
> On 15-09-23 23:06:54, Mimi Zohar wrote:
> > On Tue, 2015-09-22 at 18:19 +0300, Petko Manolov wrote:
> > > 
> > > Well, this is a sore point.  I don't have sufficient knowledge about how 
> > > audit_rule_xxx callbacks work and the only safe workaround i could think 
> > > of 
> > > is to move this call out of the ima_match_policy()'s big RCU read lock.
> > 
> > The LSM rule numbers change when the LSM policy (eg. SELinux) is reloaded.  
> > All of the old LSM policy rules will be included in the new policy, so 
> > there 
> > shouldn't be a problem with simply replacing the old LSM rule number with 
> > the 
> > new one.
> 
> OK.
> 
> > > I suggest that we run another list_for_each_entry(entry, ima_rules, list) 
> > > loop and update the LSM rules there, where taking a mutex is legal.  What 
> > > would you say?
> > 
> > The mutex prevented the concurrent udpate.  As there shouldn't be any side 
> > affects with updating the field multiple times, I would leave it alone.
> 
> This means the patch stays as it is?

Yes.

> > > Yes, i did.  The problem with list_splice_tail_init() is not RCU safe and 
> > > it 
> > > does pointer assignment in the wrong way for us.  If i used it i should 
> > > have 
> > > put spinlocks around the call, which i thought i can avoid.
> > > 
> > > include/linux/rculist.h has only one splice routine, 
> > > list_splice_init_rcu(), 
> > > but it creates stack structure, not queue.
> > > 
> > > The pointers assignment is done in such order so any in-flight readers 
> > > will 
> > > either see the old policy or the combined one, not a disjointed version 
> > > of 
> > > it.  This is guaranteed by the way the readers walk the list, IOW - 
> > > forward.
> > > 
> > > This line is the key: rcu_assign_pointer(list_next_rcu(policy->prev), 
> > > first);
> > 
> > Fine, eventually this code should be moved to rculist.h.
> 
> I just sent a patch to Paul McKenney doing just that.  However, i suggest 
> that 
> we don't wait for him applying the said patch as it may take some time.

Agreed.

> Do you want me to do anything else or the two patches i sent earlier (one 
> adding 
> additional keyring and another for IMA policy updates) are OK for mainlining?
> 
> As far as i can tell we're late for 4.3, but let's aim for 4.4 window.

Definitely way too late for 4.3.  In general, for patches being
upstreamed via the linux-security subsystem, patches are first
upstreamed via their respective trees.  For integrity, the patches first
go into the linux-integrity next branch.  (linux-next automatically
picks up these patches.)   Pull requests for the different
linux-security subsystems (eg. SELinux, smack, apparmor, capabilities,
YAMA, integrity, etc)  are sent around rc4 to James.  During the open
window, James sends a pull request to Linus.

Mimi

> 
> thanks,
> Petko


--
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

Re: [PATCH 1/1] integrity: prevent loading untrusted certificates to IMA trusted keyring

[Mimi Zohar]

On Thu, 2015-09-10 at 22:06 +0300, Dmitry Kasatkin wrote:
> If IMA_LOAD_X509 is enabled either directly or indirectly via
> IMA_APPRAISE_SIGNED_INIT, it enables certificate loading to the IMA trusted
> keyring from the kernel. Due to the overlook, KEY_ALLOC_TRUSTED was used in 
> the
> key_create_or_update() to create keys within the kernel, which caused
> overriding certificate verification result and allowed to load self-signed or
> wrongly signed certificates.
> 
> This patch just removes this option.

Thanks!

Mimi

> 
> Signed-off-by: Dmitry Kasatkin 
> Cc:   # 3.19+
> ---
>  security/integrity/digsig.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 36fb6b5..5be9ffb 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -105,7 +105,7 @@ int __init integrity_load_x509(const unsigned int id, 
> const char *path)
>  rc,
>  ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>   KEY_USR_VIEW | KEY_USR_READ),
> -KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_TRUSTED);
> +KEY_ALLOC_NOT_IN_QUOTA);
>   if (IS_ERR(key)) {
>   rc = PTR_ERR(key);
>   pr_err("Problem loading X.509 certificate (%d): %s\n",


--
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

Re: [RFC][Patch 1/1] IBAC Patch

[Mimi Zohar]

On Tue, 2007-06-19 at 17:23 -0500, Serge E. Hallyn wrote:

  +#define get_file_security(file) ((unsigned long)(file-f_security))
  +#define set_file_security(file, val) (file-f_security = (void *)val)
  +
  +#define get_task_security(task) ((unsigned long)(task-security))
  +#define set_task_security(task, val) (task-security = (void *)val)
 
 I understand the above are likely remnants of stacker lsm support, and I
 hate to say this, but not only is having those in there going to be
 frowned upon, it then leads you later on to do things like
 
   if (get_file_security(file)==0)
 
 when using 0 for null upsets people in itself.

Instead of allocating memory for file-f_security, it uses 
file-f_security itself, for storing 0 or 1.  So it isn't
checking to see if it is NULL per se.

  +
  +#define XATTR_MEASURE_SUFFIX measure
  +#define XATTR_MEASURE_SUFFIX_LEN (sizeof (XATTR_MEASURE_SUFFIX) -1)
  +
  +struct ibac_isec_data {
  +   int mmapped;/* no. of times inode mmapped */
  +   int measure;/* inode tagged to be measured */
  +   spinlock_t lock;/* protect inode state */
  +};
  +
  +static int ibac_inode_alloc_security(struct inode *inode)
  +{
  +   struct ibac_isec_data *isec;
  +
  +   isec = kzalloc(sizeof(struct ibac_isec_data), GFP_KERNEL);
  +   if (!isec)
  +   return -ENOMEM;
  +
  +   spin_lock_init(isec-lock);
  +   inode-i_security = isec;
 
 Heh, for file and task security you use the above helpers, but for inode
 you do it like this?  :)  Please replace all x_security assignments and
 checks with this format.

For file and task, the code uses the security field itself to store
information as opposed to allocating storage for it.   In the case of 
inode, it is using it for both the original digsig mmap tracking and 
now to tag the inode that it needs to be measured.  Tagging the inode
to be measured is based on the existence of the security.measure xattr,
which is controlled by a userspace application.  The difference is
that storage is allocated for inode-i_security.

  +/*
  + * For all inodes allocate inode-i_security(isec), before the security
  + * subsystem is enabled.
  + */
  +static void ibac_fixup_inodes(void)
  +{
  +   struct super_block *sb;
  +
  +   spin_lock(sb_lock);
  +   list_for_each_entry(sb, super_blocks, s_list) {
  +   struct inode *inode;
  +
  +   spin_unlock(sb_lock);
  +
  +   spin_lock(inode_lock);
  +   list_for_each_entry(inode, sb-s_inodes, i_sb_list) {
  +   spin_unlock(inode_lock);
  +
  +   spin_lock(inode-i_lock);
  +   if (!inode-i_security)
  +   ibac_inode_alloc_security(inode);
 
 since ibac_inode_alloc_security can return -ENOMEM, maybe you should at
 least check for that condition and warn the user?

Yes, that definitely is a good idea.  Will do.

  +   spin_unlock(inode-i_lock);
  +
  +   spin_lock(inode_lock);
  +   }
  +   spin_unlock(inode_lock);
  +
  +   spin_lock(sb_lock);
  +   }
  +   spin_unlock(sb_lock);
  +}
  +

  +static int ibac_inode_permission(struct inode *inode, int mask,
  +struct nameidata *nd)
  +{
  +   struct ibac_isec_data *isec = inode-i_security;
  +   struct dentry *dentry;
  +   char *path = NULL;
  +   char *fname = NULL;
  +   int rc = 0;
  +   int measure;
  +
  +   dentry = (!nd || !nd-dentry) ? d_find_alias(inode) : nd-dentry;
  +   if (!dentry)
  +   return 0;
  +   if (nd) {   /* preferably use fullname */
  +   path = (char *)__get_free_page(GFP_KERNEL);
  +   if (path)
  +   fname = d_path(nd-dentry, nd-mnt, path, PAGE_SIZE);
  +   }
  +
  +   if (!fname) /* no choice, use short name */
  +   fname = (!dentry-d_name.name) ? (char *)dentry-d_iname :
  +   (char *)dentry-d_name.name;
 
 Please separate the above out into a helper function.

Ok.

 This name is only ever used for debugging, right?  I didn't miss any
 place where the name is used for some security decision?

Correct, the filename is not used for any security decision, but it 
is passed to integrity_measure(), which the IMA integrity provider
associates with the given hash value.  To see the filename hints
'cat /sys/kernel/security/ima/ascii_runtime_measurements'.

  +
  +   /* Measure labeled files */
  +   spin_lock(isec-lock);
  +   measure = isec-measure;
  +   spin_unlock(isec-lock);
  +
  +   if (S_ISREG(inode-i_mode)  (measure == 1)
  +(mask  MAY_READ)) {
  +   rc = verify_metadata_integrity(dentry);
  +   if (rc == 0)
  +   rc = verify_and_measure_integrity(dentry, NULL,
  + fname, mask);
  +   }
  +
  +   /* Deny permission to write, if currently mmapped. */
  +   if (inode  mask  MAY_WRITE) {
  +   spin_lock(isec-lock);
  +   

Re: [RFC] [Patch 1/1] IBAC Patch

[Mimi Zohar]

On Tue, 2007-03-13 at 19:27 -0700, Seth Arnold wrote:
 On Thu, Mar 08, 2007 at 05:58:16PM -0500, Mimi Zohar wrote:
  This is a request for comments for a new Integrity Based Access
  Control(IBAC) LSM module which bases access control decisions
  on the new integrity framework services. 
 
 Thanks Mimi, nice to see an example of how the integrity framework ought
 to be used.
 
  (Hopefully this will help clarify the interaction between an LSM 
  module and LIM module.)
 
 Is this module intended to clarify an interface, or be useful in and of
 itself?

It's a little bit of both. :-) Initially it was written to help me with 
implementing and testing the integrity provider.  But it could definitely stand
on it's own.  As Serge Hallyn commented http://lkml.org/lkml/2007/3/13/220, by 
adding the mmap hook, IBAC could replace the LSM aspect of digsig and a gpg 
based integrity provider, could be written, instead of EVM, which is TPM based.

  Index: linux-2.6.21-rc3-mm2/security/ibac/Makefile
  ===
  --- /dev/null
  +++ linux-2.6.21-rc3-mm2/security/ibac/Makefile
  @@ -0,0 +1,6 @@
  +#
  +# Makefile for building IBAC
  +#
  +
  +obj-$(CONFIG_SECURITY_IBAC) += ibac.o
  +ibac-y := ibac_main.o
  Index: linux-2.6.21-rc3-mm2/security/ibac/ibac_main.c
  ===
  --- /dev/null
  +++ linux-2.6.21-rc3-mm2/security/ibac/ibac_main.c
  @@ -0,0 +1,126 @@
  +/*
  + * Integrity Based Access Control (IBAC)
  + *
  + * Copyright (C) 2007 IBM Corporation
  + * Author: Mimi Zohar [EMAIL PROTECTED]
  + *
  + *  This program is free software; you can redistribute it and/or 
  modify
  + *  it under the terms of the GNU General Public License as published 
  by
  + *  the Free Software Foundation, version 2 of the License.
  + */
  +
  +#include linux/module.h
  +#include linux/moduleparam.h
  +#include linux/kernel.h
  +#include linux/security.h
  +#include linux/integrity.h
  +
  +#ifdef CONFIG_SECURITY_IBAC_BOOTPARAM
  +int ibac_enabled = CONFIG_SECURITY_IBAC_BOOTPARAM_VALUE;
  +
  +static int __init ibac_enabled_setup(char *str)
  +{
  +   ibac_enabled = simple_strtol(str, NULL, 0);
  +   return 1;
  +}
  +
  +__setup(ibac=, ibac_enabled_setup);
  +#else
  +int ibac_enabled = 0;
  +#endif
 
 If the command line option isn't enabled, how will ibac_enabled ever be
 set to '1'? Have I overlooked or forgotten some helper routine elsewhere?

I guess I was a bit over zealous in preventing IBAC from running 
unintentionally.  Will fix in the next IBAC patch release.

  +static unsigned int integrity_enforce = 0;
  +static int __init integrity_enforce_setup(char *str)
  +{
  +   integrity_enforce = simple_strtol(str, NULL, 0);
  +   return 1;
  +}
  +
  +__setup(ibac_enforce=, integrity_enforce_setup);
  +
  +#define XATTR_NAME security.evm.hash
 
 Is this name unique to this IBAC module? Or should it be kept in sync
 with the integrity framework?

In order to verify the metadata integrity, an xattr needs to be
specified on the integrity_verify_metadata() call.  As IBAC does not
define an xattr of its own, I used this one, which at the time worked. 
But as EVM is only one integrity provider, this probably is not a good 
idea. To resolve this problem, I guess the integrity_verify_metadata()
API call should respond without requiring an actual xattr.

  +static inline int is_kernel_thread(struct task_struct *tsk)
  +{
  +   return (!tsk-mm) ? 1 : 0;
  +}
  +
  +static int ibac_bprm_check_security(struct linux_binprm *bprm)
  +{
  +   struct dentry *dentry = bprm-file-f_dentry;
  +   int xattr_len;
  +   char *xattr_value = NULL;
  +   int rc, status;
  +
  +   rc = integrity_verify_metadata(dentry, XATTR_NAME,
  +  xattr_value, xattr_len, status);
  +   if (rc  0  rc == -EOPNOTSUPP) {
  +   kfree(xattr_value);
  +   return 0;
  +   }
  +
  +   if (rc  0) {
  +   printk(KERN_INFO verify_metadata %s failed 
  +  (rc: %d - status: %d)\n, bprm-filename, rc, status);
  +   if (!integrity_enforce)
  +   rc = 0;
  +   goto out;
  +   }
  +   if (status != INTEGRITY_PASS) { /* FAIL | NO_LABEL */
  +   if (!is_kernel_thread(current)) {
 
 Please remind me why kernel threads are exempt?

You really don't want to prevent kernel threads from working. Nasty things
happen.

  +   printk(KERN_INFO verify_metadata %s 
  +  (Integrity status: FAIL)\n, bprm-filename);
 
 Integrity status may be FAIL or NO_LABEL at this point -- would it be
 more useful to report the whole truth?

FAIL here indicates that the integrity of the metadata was bad, while
NOLABEL indicates, in the case of EVM, that there was no HMAC.  I'll
update the error message to differentiate between the two.

  +   if (integrity_enforce) {
  +   rc = -EACCES