Re: [PATCH 3/5] X.509: Support leap seconds

[Arnd Bergmann]

On Friday 18 December 2015 00:02:09 David Howells wrote:
> 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

Acked-by: Arnd Bergmann 
--
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/5] Handle leap seconds in mktime64()

[Arnd Bergmann]

On Friday 18 December 2015 00:02:02 David Howells wrote:
> 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
> 

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

[Arnd Bergmann]

On Friday 18 December 2015 00:02:17 David Howells wrote:
> 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
> 

Acked-by: Arnd Bergmann 
--
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: Fix determination of self-signedness

[Josh Boyer]

On Thu, Dec 17, 2015 at 7:03 PM, David Howells  wrote:
> 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 

Should this also be Cc'd to stable?

josh

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

Re: [PATCH] X.509: Fix determination of self-signedness

[David Howells]

Josh Boyer  wrote:

> Should this also be Cc'd to stable?

Argh.  Probably.

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: [GIT PULL] Keys fixes

[Linus Torvalds]

On Thu, Dec 17, 2015 at 8:10 PM, James Morris  wrote:
>
> David Howells (7):
>   Handle leap seconds in mktime64()

This one is completely wrong.

Leap seconds are inserted *at* the minute, not at the secodn before the minute.

So this code:

+   /* Handle leap seconds */
+   if (sec == 60)
+   sec = 59;

is just complete crap. Making the whole commit bogus and wrong.

The code did the right thing wrt leap seconds before, without having
any magical and incorrect special case. That commit makes it instead
have two seconds of xx:xx:59.

The fact that people add extra code to make things extra wrong is
annoying. The patch is marked as being cc'd to John Stultz, but I
assume it was never acked, because I doubt he would ack something like
this.

To make things worse, this whole series seems to have existed for less
than one day, and then it was sent to me as a pull request, however
buggy and non-acked it was.

To make things EVEN *more* broken, this crap was marked for stable.

So there is no way in hell I am pulling this.

People, it's past rc5. Don't send me shit like this.

 LInus
--
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: [GIT PULL] Keys fixes

[Linus Torvalds]

On Fri, Dec 18, 2015 at 11:56 AM, John Stultz  wrote:
>
> So leap-seconds are inserted at the minute, but the kernel represents
> it as repeating the last second of the day.

Maybe there is some reason why you want to do that, but at least from
a mktime64() standpoint, it's completely pointless.

Now, one difference is that mktime64() only handles "at the second"
events, so fro mktime, the "repeat the second" is basically an atomic
thing, and there is no real visibility into what happens *during* the
second.

For gettimeofday(), you actually have to handle the whole "can't go
backwards, but you have nanoseconds etc" complication. Very different.

   Linus
--
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: [GIT PULL] Keys fixes

[John Stultz]

On Fri, Dec 18, 2015 at 11:46 AM, Linus Torvalds
 wrote:
> On Thu, Dec 17, 2015 at 8:10 PM, James Morris  wrote:
>>
>> David Howells (7):
>>   Handle leap seconds in mktime64()
>
> This one is completely wrong.
>
> Leap seconds are inserted *at* the minute, not at the secodn before the 
> minute.
>
> So this code:
>
> +   /* Handle leap seconds */
> +   if (sec == 60)
> +   sec = 59;
>
> is just complete crap. Making the whole commit bogus and wrong.
>
> The code did the right thing wrt leap seconds before, without having
> any magical and incorrect special case. That commit makes it instead
> have two seconds of xx:xx:59.

So leap-seconds are inserted at the minute, but the kernel represents
it as repeating the last second of the day.
ie:
xx:xx:58
xx:xx:59
xx:xx:59 (+ TIME_OOP)
xx:xx:00

So I did suggest that he follow the kernel's behavior in this way, but
I've not yet had a chance to review the patch.

thanks
-john
--
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: [GIT PULL] Keys fixes

[Linus Torvalds]

On Fri, Dec 18, 2015 at 11:46 AM, Linus Torvalds
 wrote:
>
> The fact that people add extra code to make things extra wrong is
> annoying.

Side note: the key handling extra checks seem pretty pointless too.

There's no reason to have those "some time formats allow 60 seconds,
some don't".

So instead of that whole "max_seconds" complication depending on
exactly which time format you happened to use, just allow xx:xx:60
unconditionally.Same goes for 24:00:00.

Just let it go. The *reason* for the bugs that those commit fix was
that the code was overly anal to begin with. The fix is not to make it
EVEN MORE ANAL. No, the fix is to just say "ok, we allow the extra
second at the end".

So just allow "24" in the hours field, and allow "60" in the minutes field.

And you know what? If somebody decides that they want to have a key
that says it was done at some nonsensical time like 24:30:60, just let
it go. Just accept it. It's not your problem.

I'd like to point out how time64() has always allowed those things,
and nobody has *ever* complained. In fact, exactly because it allowed
those things, it gets the leap second right *without* the broken patch
you had.

There's a fine line between "be careful" and "be anal". One of those
is good. The other one is not.

And those patches generally crossed that line.

 Linus
--
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: [GIT PULL] Keys fixes

[James Morris]

On Fri, 18 Dec 2015, Linus Torvalds wrote:

> So there is no way in hell I am pulling this.
> 

Sorry for the confusion.

Please pull the first patch from here:

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

David Howells (1):
  KEYS: Fix race between read and revoke

---

 security/keys/keyctl.c |   18 +-
 1 files changed, 9 insertions(+), 9 deletions(-)
commit b4a1b4f5047e4f54e194681125c74c0aa64d637d
Author: David Howells 
Date:   Fri Dec 18 01:34:26 2015 +

KEYS: Fix race between read and revoke

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
Signed-off-by: James Morris 

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fb111ea..1c3872a 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

Re: [GIT PULL] Keys fixes

[Linus Torvalds]

On Fri, Dec 18, 2015 at 2:37 PM, David Howells  wrote:
>
> Except that it has been argued that they have to be there or someone can use
> dates that contribute to the signature to fake a signed content.  Admittedly
> being able to have a seconds=60 value in somewhere that should stop at 59
> doesn't allow a lot of contribution...

Oh Christ, you've been talking to the crazies again.

Here's a good rule of thumb: the more vocal the security person is,
the more likely he is wrong and crazy.

Allowing '24' in the hour field (and '60' in the second) field adds
something like on twentieth of a bit of extra information for an
attacker to play with. Not one whole bit. One _twentieth_ of a bit.
They already had access to the range 0-23 and 0-59, giving them access
to a slightly larger range doesn't really give them anything
fundamentally more interesting.

If your key uniqueness depend on that kind of "much less than one bit
of information" security, your key is garbage.

In other words, it's not an argument you should care about.

Btw, if you want a *real* bit of information that you can actually use
to make informed judgement that i worth something, then use *that*
bit: the kind of people who spout idiotic theoretical nonsense like
that, are people you should ignore. What else did they tell you?
Because that was probably crap too.

There are good security people out there, but there's a lot of
crackpors out there too. You need to recognize the crackpots.

Another good rule of thumb: if you can make the code simpler and more
obvious, do it. Because *that* is going to make it a hell of a lot
more secure than trying to be clever about when you can allow 24 or 60
in the hours/seconds field.

 Linus
--
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: Exposing secid to secctx mapping to user-space

[Paul Moore]

On Tue, Dec 15, 2015 at 3:58 PM, Daniel Cashman  wrote:
> On 12/15/2015 07:00 AM, Stephen Smalley wrote:
>> 1. I don't think it is the size of the context that is the concern but
>> rather the fact that it is a variable-length string, whereas current
>> binder commands use fixed-size arguments and encode the size in the
>> command value (common for ioctls).  Supporting passing a variable-length
>> string would be a change to the protocol and would complicate the code.
>>  On the performance side, it means generating a context string from the
>> secid and then copying it to userspace on each IPC (versus just getting
>> the secid and putting that in the existing binder_transaction_data that
>> is already copied to userspace).
>
> This is precisely the motivation for the original enquiry. Issue has
> been brought up about changing the protocol, and concern has also been
> strongly expressed about the overhead introduced by the string
> operations, although this has not been measured.  User-space would still
> need to do something intelligent with the secid, which would involve its
> own lookup and caching, but the idea is that this wouldn't be done with
> the binder lock held.
>
>> 2. Don't know; deferring to Daniel to run whatever binder IPC benchmarks
>> might exist with and without the current patch that copies the context
>> string.
>
> Yes, this needs to be done.  This issue was brought up as part of
> discussion regarding a proposed change to the binder driver to add the
> context string to each transaction.  An outcome of that discussion was,
> "before we go too far into this, let's see the reaction upstream to
> exposing the secid."  Based on the reaction here (upstream), I think
> it's my responsibility to push forward the string-based change and get
> the appropriate perf numbers so that a meaningful comparison can be made.

The existing, variable length string based approach is going to be
your easiest path forward with respect to the kernel, although it may
turn out to be a non-starter from a binder point of view.  I just want
to reiterate that I'm not against the idea of exposing the secid
tokens, but not necessarily in their current form; we will probably
want to revisit the idea of a persistent secid and consider the impact
to any future stacking work.

-- 
paul moore
www.paul-moore.com
--
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: Exposing secid to secctx mapping to user-space

[Paul Moore]

On Tue, Dec 15, 2015 at 2:09 PM, Joe Nall  wrote:
>> On Dec 15, 2015, at 12:03 PM, Stephen Smalley  wrote:
>> Are you patching the kernel to support > 4K contexts?
>> Otherwise, I'd expect you run up against the proc and selinuxfs API 
>> limitations (page size) and/or the filesystem xattr storage limitations 
>> (block size).
>
> No. The example was a contrived example of what is possible within the 
> format. We use a couple of 2500 byte labels in formal test these days to make 
> sure that we don't have an OS regression. I just get tired of code like this 
> in openswan:
>
> #ifdef HAVE_LABELED_IPSEC
> /* security label length should not exceed 256 in most cases,
>  * (discussed with kernel and selinux people).
>  */
> #define MAX_SECCTX_LEN257 /* including '\0'*/

So let's just get rid of labeled IPsec ... show of hands? ;)

-- 
paul moore
www.paul-moore.com
--
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: [GIT PULL] Keys fixes

[David Howells]

Linus Torvalds  wrote:

> > David Howells (7):
> >   Handle leap seconds in mktime64()
> 
> This one is completely wrong.
> 
> Leap seconds are inserted *at* the minute, not at the secodn before the 
> minute.
> 
> So this code:
> 
> +   /* Handle leap seconds */
> +   if (sec == 60)
> +   sec = 59;
> 
> is just complete crap. Making the whole commit bogus and wrong.

I did ask on ksummit-discuss beforehand.  The advice was to treat hh:mm:60 as
hh:mm:59 rather than hh:mm+1:00.  Unless we actually support leap seconds as
distinct time_t values, it has to be one or the other.

> The code did the right thing wrt leap seconds before, without having
> any magical and incorrect special case. That commit makes it instead
> have two seconds of xx:xx:59.

... as opposed to two seconds of xx:xx+1:00.  You can argue it either way -
and arguably both are equally wrong since neither maps correctly to reality.

> The fact that people add extra code to make things extra wrong is
> annoying. The patch is marked as being cc'd to John Stultz, but I
> assume it was never acked, because I doubt he would ack something like
> this.
>
> To make things worse, this whole series seems to have existed for less
> than one day, and then it was sent to me as a pull request, however
> buggy and non-acked it was.

I only asked James to pass the CVE-labelled commit on to you and didn't
include it in a patch series.  The rest I posted hoping for reviews.

> To make things EVEN *more* broken, this crap was marked for stable.

It will theoretically need to end up there anyway, since it is technically
possible for the bugs to prevent a kernel from booting - just not very likely.
--
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: [GIT PULL] Keys fixes

[David Howells]

Linus Torvalds  wrote:

> Side note: the key handling extra checks seem pretty pointless too.

Except that it has been argued that they have to be there or someone can use
dates that contribute to the signature to fake a signed content.  Admittedly
being able to have a seconds=60 value in somewhere that should stop at 59
doesn't allow a lot of contribution...

> There's no reason to have those "some time formats allow 60 seconds,
> some don't".

Feel free to explain that to the people who drafted the ASN.1 standards.
Maybe they'll listen to you...

> And you know what? If somebody decides that they want to have a key
> that says it was done at some nonsensical time like 24:30:60, just let
> it go. Just accept it. It's not your problem.

I've been told that it's a security hole.

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