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

[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