selinux: security_bounded_transition fails

[Hannu Savolainen]

Hi,

I'm having a problem with a multithreaded application. It does lengthy  
initialization in advance under relatively privileged context and then switches 
to a less privileged one after the moment when the actual request arrives. 
After that it will create a chrooted container and join all threads to a new 
SELinux context.

However the transition fails with audit message "op=security_bounded_transition 
result=denied oldcontext=old_context newcontext=new_context".

Is there any policy rule that could be used to fix this or is this just not 
supported?

Best regards,

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

[Luis R. Rodriguez]

On Thu, Dec 17, 2015 at 07:32:10AM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 14:45 +0800, Dave Young wrote:
> > On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> > > 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.)

I'm with Dave though, I realize that's work but I can help do it with you.

 Luis
--
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 3/7] ima: load policy using path

[Luis R. Rodriguez]

The subject should probably be: ima: add support to load policy from path

Cc'ing Roberts who originally wanted SELinux file policy signing capabilities.
Also Greg, who is reviewing the sysdata file changes I had proposed which would
provide a generic file loading facility for modules, and later a generic file
signing checker. Cc'ing Linus for any feedback on the possible issues he might
foresee if we all go gung-ho on "kernel file loading", as this is where it
seems some folks might be going. I *think* the user hint might help close the
semantic gap observed on using a file loader on firmware_class, but perhaps he
or other might have some other corner cases in mind we should also consider.

Note that the crux between using a generic kernel file loader and the common
"sysdata" file loader will be that sysdata file users (wireless would be one
getting rid of CRDA) and a core kernel file loader (IMA could be one, likewise
perhaps SELinux if it follows suit in design as in this patch) is that the core
kernel file loader would allow arbitrary file paths, and the sysdata users
would have stuff in /lib/firmware/ paths (and therefore also have things in the
linux-firmware tree).

On Tue, Dec 08, 2015 at 01:01:20PM -0500, Mimi Zohar wrote:
> From: Dmitry Kasatkin 
> 
> Currently the IMA policy is loaded by writing the policy rules to
> '/ima/policy'.

> That way the policy cannot be measured or appraised. This patch extends the
> policy loading interface with the possibility to load the policy using a
> pathname. The policy can be loaded like:
> 
> echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy

I don't think this is as clear, you can just say something like:

-
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 polify 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 for the file
signature if it had one. This adds support to load IMA policies
with a given path as follows:

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

> 
> Signed-off-by: Dmitry Kasatkin 
> Signed-off-by: Mimi Zohar 
> ---
>  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;
> +}
> +

Please no, instead of adding yet-another kernel file-loading facility which is
likely error prone we should consolidate *all kernel file-loading facilities*
into a *common generic shared one*. So please work to make that happen since you
need yet-another user for it. I've done some initial homework already on a few
prominent common users. I say "initial" homework as my search was rather crude 
on
'git grep' and not done using semantic parsers to see if I "search for file
loaders" through semantic means. This later search may be optional, but it
would help 

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

[Luis R. Rodriguez]

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

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

[Luis R. Rodriguez]

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?

> @@ -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_FIRMWARE0x10
> +#define IMA_APPRAISE_POLICY  0x20
>  
>  #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.

  Luis
--
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 3/5] X.509: Support leap seconds

[David Howells]

The format of ASN.1 GeneralizedTime seems to be specified by ISO 8601
[X.680 46.3] and this apparently supports leap seconds (ie. the seconds
field is 60).  It's not entirely clear that ASN.1 expects it, but we can
relax the seconds check slightly for GeneralizedTime.

This, however, results in us passing a time with sec as 60 to mktime64()
which, unpatched, doesn't really handle such things.  What it will do is
equate the 60th second of a minute to the 0th second of the next minute.

We can't really do otherwise without giving the kernel much greater
knowledge of where all the leap seconds are.  Unfortunately, this would
require change the mapping of the kernel's current-time-in-seconds.

UTCTime, however, only supports a seconds value in the range 00-59.

Without this patch, certain X.509 certificates will be rejected,
potentially making a kernel unbootable.

Reported-by: Rudolf Polzer 
Signed-off-by: David Howells 
cc: David Woodhouse 
cc: John Stultz 
cc: Arnd Bergmann 
cc: sta...@vger.kernel.org
---

 crypto/asymmetric_keys/x509_cert_parser.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c 
b/crypto/asymmetric_keys/x509_cert_parser.c
index 13c4e5a5fe8c..9be2caebc57b 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -497,7 +497,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
static const unsigned char month_lengths[] = { 31, 28, 31, 30, 31, 30,
   31, 31, 30, 31, 30, 31 };
const unsigned char *p = value;
-   unsigned year, mon, day, hour, min, sec, mon_len;
+   unsigned year, mon, day, hour, min, sec, mon_len, max_sec;
 
 #define dec2bin(X) ({ unsigned char x = (X) - '0'; if (x > 9) goto 
invalid_time; x; })
 #define DD2bin(P) ({ unsigned x = dec2bin(P[0]) * 10 + dec2bin(P[1]); P += 2; 
x; })
@@ -511,6 +511,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
year += 1900;
else
year += 2000;
+   max_sec = 59;
} else if (tag == ASN1_GENTIM) {
/* GenTime: MMDDHHMMSSZ */
if (vlen != 15)
@@ -518,6 +519,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
year = DD2bin(p) * 100 + DD2bin(p);
if (year >= 1950 && year <= 2049)
goto invalid_time;
+   max_sec = 60; /* ISO 8601 permits leap seconds [X.680 46.3] */
} else {
goto unsupported_time;
}
@@ -550,7 +552,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
if (day < 1 || day > mon_len ||
hour > 23 ||
min > 59 ||
-   sec > 59)
+   sec > max_sec)
goto invalid_time;
 
*_t = mktime64(year, mon, day, hour, min, sec);

--
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 1/5] X.509: Fix leap year handling again

[David Howells]

There are still a couple of minor issues in the X.509 leap year handling:

 (1) To avoid doing a modulus-by-400 in addition to a modulus-by-100 when
 determining whether the year is a leap year or not, I divided the year
 by 100 after doing the modulus-by-100, thereby letting the compiler do
 one instruction for both, and then did a modulus-by-4.

 Unfortunately, I then passed the now-modified year value to mktime64()
 to construct a time value.

 Since this isn't a fast path and since mktime64() does a bunch of
 divisions, just condense down to "% 400".  It's also easier to read.

 (2) The default month length for any February where the year doesn't
 divide by four exactly is obtained from the month_length[] array where
 the value is 29, not 28.

 This is fixed by altering the table.

Reported-by: Rudolf Polzer 
Signed-off-by: David Howells 
Acked-By: David Woodhouse 
cc: sta...@vger.kernel.org
---

 crypto/asymmetric_keys/x509_cert_parser.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c 
b/crypto/asymmetric_keys/x509_cert_parser.c
index 021d39c0ba75..13c4e5a5fe8c 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -494,7 +494,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
 unsigned char tag,
 const unsigned char *value, size_t vlen)
 {
-   static const unsigned char month_lengths[] = { 31, 29, 31, 30, 31, 30,
+   static const unsigned char month_lengths[] = { 31, 28, 31, 30, 31, 30,
   31, 31, 30, 31, 30, 31 };
const unsigned char *p = value;
unsigned year, mon, day, hour, min, sec, mon_len;
@@ -540,9 +540,9 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
if (year % 4 == 0) {
mon_len = 29;
if (year % 100 == 0) {
-   year /= 100;
-   if (year % 4 != 0)
-   mon_len = 28;
+   mon_len = 28;
+   if (year % 400 == 0)
+   mon_len = 29;
}
}
}

--
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] Handle leap seconds in mktime64()

[David Howells]

Handle leap seconds in mktime64() - where the seconds parameter is the
value 60 - by treating it the same as 59.

This facility will be used by the X.509 parser.  Doing it in mktime64()
makes the policy common to the whole kernel and easier to find.

Whilst we're at it, remove the const markers from all the parameters since
they don't really achieve anything and we do need to alter the sec
parameter.

Signed-off-by: David Howells 
cc: John Stultz 
cc: Arnd Bergmann 
cc: sta...@vger.kernel.org
---

 include/linux/time.h |   13 ++---
 kernel/time/time.c   |   14 +++---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index beebe3a02d43..35384f0c0aa2 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -39,17 +39,16 @@ static inline int timeval_compare(const struct timeval 
*lhs, const struct timeva
return lhs->tv_usec - rhs->tv_usec;
 }
 
-extern time64_t mktime64(const unsigned int year, const unsigned int mon,
-   const unsigned int day, const unsigned int hour,
-   const unsigned int min, const unsigned int sec);
+extern time64_t mktime64(unsigned int year, unsigned int mon,
+unsigned int day, unsigned int hour,
+unsigned int min, unsigned int sec);
 
 /**
  * Deprecated. Use mktime64().
  */
-static inline unsigned long mktime(const unsigned int year,
-   const unsigned int mon, const unsigned int day,
-   const unsigned int hour, const unsigned int min,
-   const unsigned int sec)
+static inline unsigned long mktime(unsigned int year, unsigned int mon,
+  unsigned int day, unsigned int hour,
+  unsigned int min, unsigned int sec)
 {
return mktime64(year, mon, day, hour, min, sec);
 }
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 86751c68e08d..1858b10602f5 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -322,10 +322,14 @@ EXPORT_SYMBOL(timespec_trunc);
  * -year/100+year/400 terms, and add 10.]
  *
  * This algorithm was first published by Gauss (I think).
+ *
+ * A leap second can be indicated by calling this function with sec as
+ * 60 (allowable under ISO 8601).  The leap second is treated the same
+ * as the preceding second since they don't exist in UNIX time.
  */
-time64_t mktime64(const unsigned int year0, const unsigned int mon0,
-   const unsigned int day, const unsigned int hour,
-   const unsigned int min, const unsigned int sec)
+time64_t mktime64(unsigned int year0, unsigned int mon0,
+ unsigned int day, unsigned int hour,
+ unsigned int min, unsigned int sec)
 {
unsigned int mon = mon0, year = year0;
 
@@ -335,6 +339,10 @@ time64_t mktime64(const unsigned int year0, const unsigned 
int mon0,
year -= 1;
}
 
+   /* Handle leap seconds */
+   if (sec == 60)
+   sec = 59;
+
return time64_t)
  (year/4 - year/100 + year/400 + 367*mon/12 + day) +
  year*365 - 719499

--
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] X.509: Fix determination of self-signedness

[David Howells]

Fix determination of whether an X.509 certificate is self-signed or not.

It is currently assumed that a cert is self-signed if has no
authorityKeyIdentifier or the authorityKeyIdentifier matches the
subjectKeyIdentifier.  However, it is possible to encounter a certificate
that has neither AKID not SKID but is not self-signed.

This symptoms of this show up as an attempt to load a certificate failing
with -ERANGE or -EBADMSG, produced from the RSA module when the result of
calculating "m = s^e mod n" is checked.

To fix this, don't check to see if a certificate is self-signed if the
Issuer and Subject names differ.

Signed-off-by: David Howells 
cc: David Woodhouse 
---

 crypto/asymmetric_keys/x509_public_key.c |   11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c 
b/crypto/asymmetric_keys/x509_public_key.c
index 2a44b3752471..6236e7996f19 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -313,9 +313,14 @@ static int x509_key_preparse(struct key_preparsed_payload 
*prep)
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)) {
+   if ((!cert->akid_skid && !cert->akid_id)) {
+   if (cert->raw_issuer_size == cert->raw_subject_size &&
+   memcmp(cert->raw_issuer, cert->raw_subject,
+  cert->raw_subject_size) == 0)
+   goto self_signed;
+   } else if (asymmetric_key_id_same(cert->skid, cert->akid_skid) ||
+  asymmetric_key_id_same(cert->id, cert->akid_id)) {
+self_signed:
ret = x509_check_signature(cert->pub, cert); /* self-signed */
if (ret < 0)
goto error_free_cert;

--
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 4/5] Handle both ISO 8601 encodings of midnight in mktime64()

[David Howells]

ISO 8601 format dates permit two different encodings of midnight - 00:00:00
and 24:00:00 - the first is midnight today and the second is midnight
tomorrow and is exactly equivalent to the first with tomorrow's date.

Note that the implementation of mktime64() doesn't actually need to be
changed to handle this - the multiplication by 3600 of the hour will take
care of it automatically.  However, we should document that this handling
is done in mktime64() and is thus in a common place in the kernel.

This handling is required for X.509 certificate parsing which can be given
ISO 8601 dates.

Signed-off-by: David Howells 
cc: John Stultz 
cc: Arnd Bergmann 
cc: sta...@vger.kernel.org
---

 kernel/time/time.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index 1858b10602f5..56e7ada38471 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -326,6 +326,9 @@ EXPORT_SYMBOL(timespec_trunc);
  * A leap second can be indicated by calling this function with sec as
  * 60 (allowable under ISO 8601).  The leap second is treated the same
  * as the preceding second since they don't exist in UNIX time.
+ *
+ * An encoding of midnight at the end of the day as 24:00:00 - ie. midnight
+ * tomorrow - (allowable under ISO 8601) is supported.
  */
 time64_t mktime64(unsigned int year0, unsigned int mon0,
  unsigned int day, unsigned int hour,
@@ -346,7 +349,7 @@ time64_t mktime64(unsigned int year0, unsigned int mon0,
return time64_t)
  (year/4 - year/100 + year/400 + 367*mon/12 + day) +
  year*365 - 719499
-   )*24 + hour /* now have hours */
+   )*24 + hour /* now have hours - midnight tomorrow handled here */
  )*60 + min /* now have minutes */
)*60 + sec; /* finally seconds */
 }

--
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 5/5] X.509: Handle midnight alternative notation in GeneralizedTime

[David Howells]

The ASN.1 GeneralizedTime object carries an ISO8601 format date and time.
The time is permitted to show midnight as 00:00 or 24:00 (the latter being
equivalent of 00:00 of the following day).

The permitted value is checked in x509_decode_time() but the actual
handling is left to mktime64().

Without this patch, certain X.509 certificates will be rejected and could
lead to an unbootable kernel.

Reported-by: Rudolf Polzer 
Signed-off-by: David Howells 
cc: David Woodhouse 
cc: John Stultz 
cc: Arnd Bergmann 
cc: sta...@vger.kernel.org
---

 crypto/asymmetric_keys/x509_cert_parser.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c 
b/crypto/asymmetric_keys/x509_cert_parser.c
index 9be2caebc57b..b9de251c419c 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -497,7 +497,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
static const unsigned char month_lengths[] = { 31, 28, 31, 30, 31, 30,
   31, 31, 30, 31, 30, 31 };
const unsigned char *p = value;
-   unsigned year, mon, day, hour, min, sec, mon_len, max_sec;
+   unsigned year, mon, day, hour, min, sec, mon_len, max_sec, max_hour;
 
 #define dec2bin(X) ({ unsigned char x = (X) - '0'; if (x > 9) goto 
invalid_time; x; })
 #define DD2bin(P) ({ unsigned x = dec2bin(P[0]) * 10 + dec2bin(P[1]); P += 2; 
x; })
@@ -512,6 +512,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
else
year += 2000;
max_sec = 59;
+   max_hour = 23;
} else if (tag == ASN1_GENTIM) {
/* GenTime: MMDDHHMMSSZ */
if (vlen != 15)
@@ -520,6 +521,7 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
if (year >= 1950 && year <= 2049)
goto invalid_time;
max_sec = 60; /* ISO 8601 permits leap seconds [X.680 46.3] */
+   max_hour = 24;
} else {
goto unsupported_time;
}
@@ -550,11 +552,17 @@ int x509_decode_time(time64_t *_t,  size_t hdrlen,
}
 
if (day < 1 || day > mon_len ||
-   hour > 23 ||
+   hour > max_hour ||
min > 59 ||
sec > max_sec)
goto invalid_time;
 
+   /* GeneralizedTime, encoded as ISO 8601, also permits 24:00 today as an
+* alternative for 00:00 tomorrow.
+*/
+   if (hour == 24 && (min != 0 || sec != 0))
+   goto invalid_time;
+
*_t = mktime64(year, mon, day, hour, min, sec);
return 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 0/5] X.509: Fix time handling

[David Howells]

Here's a set of patches that fix X.509 time handling in three ways:

 (1) Fix leap year handling.

 (2) Add leap second handling (where you get a time of 23:59:60).

 (3) Add end-of-day midnight encoding (where you get a time of 24:00:00).

David
---
David Howells (5):
  X.509: Fix leap year handling again
  Handle leap seconds in mktime64()
  X.509: Support leap seconds
  Handle both ISO 8601 encodings of midnight in mktime64()
  X.509: Handle midnight alternative notation in GeneralizedTime


 crypto/asymmetric_keys/x509_cert_parser.c |   24 +---
 include/linux/time.h  |   13 ++---
 kernel/time/time.c|   19 +++
 3 files changed, 38 insertions(+), 18 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

[PATCH]: Smack: type confusion in smak sendmsg() handler

[Roman Kubiak]

Please note that this problem was not found by me but by Mateusz Fruba
and he takes full credit for all the below details, the patch has been
submitted by me due to corporate rules, all questions/issues etc. can be
submitted here and I will forward them to Mateusz if needed.

--- cut here for patch
Smack security handler for sendmsg() syscall
is vulnerable to type confusion issue what
can allow to privilege escalation into root
or cause denial of service.

A malicious attacker can create socket of one
type for example AF_UNIX and pass is into
sendmsg() function ensuring that this is
AF_INET socket.

Remedy
Do not trust user supplied data.
Proposed fix below.

Signed-off-by: Roman Kubiak 
Signed-off-by: Mateusz Fruba 
---
 security/smack/smack_lsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ff81026..9258a52 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3758,7 +3758,7 @@ static int smack_socket_sendmsg(struct socket *sock, 
struct msghdr *msg,
if (sip == NULL)
return 0;
 
-   switch (sip->sin_family) {
+   switch (sock->sk->sk_family) {
case AF_INET:
rc = smack_netlabel_send(sock->sk, sip);
break;
-- 
1.9.1
--- cut here for patch

--- cut here for detailed bug information
Summary:
Smack security handler for sendmsg() syscall is vulnerable to type confusion 
issue what can allow to privilege escalation into root or cause denial of 
service.

Detail:
Mainline kernel implementation of sendmsg() syscall in smack is vulnerable to 
type confusion bug.
Vulnerable code can be found in:
Linux/security/smack/smack_lsm.c
static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
int size)
{
struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;
struct sockaddr_in6 *sap = (struct sockaddr_in6 *) msg->msg_name;
int rc = 0;
 
/*
 * Perfectly reasonable for this to be NULL
 */
if (sip == NULL)
return 0;
 
switch (sip->sin_family) {
case AF_INET:
rc = smack_netlabel_send(sock->sk, sip);
break;
case AF_INET6:
rc = smk_ipv6_port_check(sock->sk, sap, SMK_SENDING);
break;
}
return rc;
}
As you can see in code above type of socket is checked using user supplied data.
An malicious attacker can create socket of one type for example AF_UNIX and 
pass is into sendmsg() function ensuring that this is AF_INET socket.
If we go dipper 
smack_socket_sendmsg(...)>smack_netlabel_send(...)>smack_netlabel(...)->cipso_v4_sock_delattr(struct
 sock *sk) then we can find following code:
Linux/net/ipv4/cipso_ipv4.c
void cipso_v4_sock_delattr(struct sock *sk)
{
int hdr_delta;
struct ip_options_rcu *opt;
struct inet_sock *sk_inet;
 
sk_inet = inet_sk(sk);
opt = rcu_dereference_protected(sk_inet->inet_opt, 1);
if (opt == NULL || opt->opt.cipso == 0)
return;
 
hdr_delta = cipso_v4_delopt(_inet->inet_opt);
if (sk_inet->is_icsk && hdr_delta > 0) {
struct inet_connection_sock *sk_conn = inet_csk(sk);
sk_conn->icsk_ext_hdr_len -= hdr_delta;
sk_conn->icsk_sync_mss(sk, sk_conn->icsk_pmtu_cookie);
}
}
In code above our malicious AF_NETLINK socket is casted into AF_INET and next 
"inet_opt" field is dereferenced from it. In case of AF_NETLINK there is no 
inet_opt field so an another value will be returned.
Test for x64 kernel 3.18.24:
As verified inet_opt field is under 0x2d0 in struct inet_sock, if we check 
other type of sockets:
AF_INET
(gdb) p/x &((struct inet_sock *)0)->inet_opt
$4 = 0x2d0
AF_NETLINK
(gdb) p/x &((struct netlink_sock *)0)->groups
$6 = 0x2d0
AF_UNIX
(gdb) p/x ((struct unix_sock *)sk)->readlock
$7 = 0x2d0
So the offset is dependent from kernel version, socket type (multiple socket 
available depending on kernel config), processor architecture.
Summing up in some cases value which will be resolved by inet_opt can be 
controlled by attacker what can be used to gain code execution in kernel.

POC:
Issue require applied smack into kernel.
I have tested and confirmed this issue on:
Mainline kernel 3.18.24 (x64)
Z300H BOK8 build - kernel 3.10.65 (arm)
SM-R730S AOKA build - kernel 3.4.0 (arm)
Below you can find poc code which use smack type confusion to crash kernel.
#include 
#include 
#include 
#include 
#include 
#include 
#include 
 
#include 
#include 
 
 
int
main(int argc, char *argv[])
{
int ret = 0;
 
int sockfd = socket(AF_NETLINK, SOCK_RAW, 0);
printf( "socket = %d\n", sockfd);
 
struct sockaddr_nl nl_addr;
nl_addr.nl_family = AF_NETLINK;
nl_addr.nl_pid = getpid();
nl_addr.nl_groups = 1337;
ret = bind(sockfd, 

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
> > #
> > 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
> > 
> Thanks
> Dave

Thank you for your suggestion.

Mimi

--
To unsubscribe from this list: send 

[GIT PULL] Keys fixes

[James Morris]

Please pull these fixes for the keys subsystem, including a fix for 
CVE-2015-7550.


The following changes since commit 73796d8bf27372e26c2b79881947304c14c2d353:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2015-12-17 
14:05:22 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git 
for-linus

David Howells (7):
  KEYS: Fix race between read and revoke
  X.509: Fix determination of self-signedness
  X.509: Fix leap year handling again
  Handle leap seconds in mktime64()
  X.509: Support leap seconds
  Handle both ISO 8601 encodings of midnight in mktime64()
  X.509: Handle midnight alternative notation in GeneralizedTime

 crypto/asymmetric_keys/x509_cert_parser.c |   24 +---
 crypto/asymmetric_keys/x509_public_key.c  |   11 ---
 include/linux/time.h  |   13 ++---
 kernel/time/time.c|   19 +++
 security/keys/keyctl.c|   18 +-
 5 files changed, 55 insertions(+), 30 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 1/4] firmware: generalize "firmware" as "system data" helpers

[Luis R. Rodriguez]

On Thu, Oct 8, 2015 at 1:16 PM, Josh Boyer  wrote:
> On Tue, Oct 6, 2015 at 5:08 AM, Greg KH  wrote:
>> Just responding to one thing at the moment:
>>
>> On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote:
>>>   * we should phase out the usermode helper from firmware_class long term
>>
>> You can "phase out", but you can not delete it as it's a user/kernel api
>> that we have to support for forever, sorry.
>
> Assuming that dell-rbu is the only in-tree legitimate user of the
> userhelper code, I'm curious if the code itself could simply move into
> that driver.  It might help prevent the spread of reliance on an API
> we don't want to see grow in usage.  We'd probably need to evaluate if
> the two new users could migrate off that.

Greg pointed out Daniel might have some uses for this. More on this later.

>> Also, for some devices / use cases, the usermode helper is the solution
>> (think async loading of firmware when the host wants to do it.)
>
> Are any of those use cases in the kernel today, aside from dell-rbu?
> Would Luis' async mode to system data suffice?

We'll have to see based on Daniel's feedback (Daniel, please respond
to the other thread I'll Cc you on).

 Luis
--
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: Problems loading firmware using built-in drivers with kernels that use initramfs.

[Luis R. Rodriguez]

On Sun, Aug 30, 2015 at 11:11 AM, Linus Torvalds
 wrote:
> On Sun, Aug 30, 2015 at 1:25 AM, Arend van Spriel  wrote:
>> On 08/29/2015 12:38 PM, Ming Lei wrote:
>>
>> Does this mean a built-in driver can not get firmware from initramfs or
>> built in the kernel early. Seems a bit too aggressive.
>
> Yeah, that seems wrong. Loading firmware from initramfs is required
> for some things, like disk drivers. Of course, depending on how it's
> done, it's all after the SYSTEM_BOOTING phase, but ..
>
> What we *might* do is to not allow it for the user-mode helper
> fallback,

FWIW, that's what we did, request_firmware_direct() now skips the
silly usermode helper. I'll note that Greg pointed out to me that
Daniel (was this right?) might have some use cases for the usermode
helper in the future on graphics though. Daniel is that right? Can you
clarify the use case, I'd just like to document this and keep it in
mind for future design changes on firmware_class. Also, it occurs to
me that if you have a need for it, perhaps others might and if we can
avoid *others* from coming up with their own solution that'd be best,
specifically as this is related to file loading -- more on this later
generalized possible use cases in another thread I'll Cc you folks on.

> but I think it's more likely that we'll just deprecate the
> usermode helper fw loader entirely, so adding new error cases for it
> seems pointless.

I was shooting hard to deprecate the usermodehelper, Greg has noted
that we can only phase it out though, so that is what I will be
shooting for: a sysdata API, what will have firmware signing support
later will *not* make use of the usermode helper. The old APIs will
still use it.

[0] http://lkml.kernel.org/r/20151006090821.gb9...@kroah.com

 Luis
--
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] KEYS: Fix race between read and revoke

[David Howells]

This fixes CVE-2015-7550.

There's a race between keyctl_read() and keyctl_revoke().  If the revoke
happens between keyctl_read() checking the validity of a key and the key's
semaphore being taken, then the key type read method will see a revoked key.

This causes a problem for the user-defined key type because it assumes in
its read method that there will always be a payload in a non-revoked key
and doesn't check for a NULL pointer.

Fix this by making keyctl_read() check the validity of a key after taking
semaphore instead of before.

I think the bug was introduced with the original keyrings code.

This was discovered by a multithreaded test program generated by syzkaller
(http://github.com/google/syzkaller).  Here's a cleaned up version:

#include 
#include 
#include 
void *thr0(void *arg)
{
key_serial_t key = (unsigned long)arg;
keyctl_revoke(key);
return 0;
}
void *thr1(void *arg)
{
key_serial_t key = (unsigned long)arg;
char buffer[16];
keyctl_read(key, buffer, 16);
return 0;
}
int main()
{
key_serial_t key = add_key("user", "%", "foo", 3, 
KEY_SPEC_USER_KEYRING);
pthread_t th[5];
pthread_create([0], 0, thr0, (void *)(unsigned long)key);
pthread_create([1], 0, thr1, (void *)(unsigned long)key);
pthread_create([2], 0, thr0, (void *)(unsigned long)key);
pthread_create([3], 0, thr1, (void *)(unsigned long)key);
pthread_join(th[0], 0);
pthread_join(th[1], 0);
pthread_join(th[2], 0);
pthread_join(th[3], 0);
return 0;
}

Build as:

cc -o keyctl-race keyctl-race.c -lkeyutils -lpthread

Run as:

while keyctl-race; do :; done

as it may need several iterations to crash the kernel.  The crash can be
summarised as:

BUG: unable to handle kernel NULL pointer dereference at 
0010
IP: [] user_read+0x56/0xa3
...
Call Trace:
 [] keyctl_read_key+0xb6/0xd7
 [] SyS_keyctl+0x83/0xe0
 [] entry_SYSCALL_64_fastpath+0x12/0x6f

Reported-by: Dmitry Vyukov 
Signed-off-by: David Howells 
Tested-by: Dmitry Vyukov 
Cc: sta...@vger.kernel.org
---

 security/keys/keyctl.c |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fb111eafcb89..1c3872aeed14 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -751,16 +751,16 @@ long keyctl_read_key(key_serial_t keyid, char __user 
*buffer, size_t buflen)
 
/* the key is probably readable - now try to read it */
 can_read_key:
-   ret = key_validate(key);
-   if (ret == 0) {
-   ret = -EOPNOTSUPP;
-   if (key->type->read) {
-   /* read the data with the semaphore held (since we
-* might sleep) */
-   down_read(>sem);
+   ret = -EOPNOTSUPP;
+   if (key->type->read) {
+   /* Read the data with the semaphore held (since we might sleep)
+* to protect against the key being updated or revoked.
+*/
+   down_read(>sem);
+   ret = key_validate(key);
+   if (ret == 0)
ret = key->type->read(key, buffer, buflen);
-   up_read(>sem);
-   }
+   up_read(>sem);
}
 
 error2:

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