Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741029 > unrolled thread

[GIT PULL] KEYS: Fixes and crypto fixes

Back to article view | Back to linux.kernel

Contents

  [GIT PULL] KEYS: Fixes and crypto fixes David Howells <dhowells@redhat.com> - 2017-09-27 23:20 +0200
    Re: [GIT PULL] KEYS: Fixes and crypto fixes James Morris <jmorris@namei.org> - 2017-09-28 01:20 +0200
      Re: [GIT PULL] KEYS: Fixes and crypto fixes Eric Biggers <ebiggers3@gmail.com> - 2017-09-28 02:20 +0200
        Re: [GIT PULL] KEYS: Fixes and crypto fixes James Morris <jmorris@namei.org> - 2017-09-28 04:10 +0200
          Re: [GIT PULL] KEYS: Fixes and crypto fixes Herbert Xu <herbert@gondor.apana.org.au> - 2017-09-28 12:40 +0200

#1741029 — [GIT PULL] KEYS: Fixes and crypto fixes

Hi James,

Can you pull these and pass them on to Linus.  There are two sets of
patches here:

 (1) A bunch of core keyrings bug fixes from Eric Biggers.

 (2) Fixing big_key to use safe crypto from Jason A. Donenfeld.

There are more patches to come from Eric, but I haven't reviewed at them
yet, so I haven't included them here.  Thanks to Eric for reviewing the
keyrings code.

David
---
The following changes since commit ebb2c2437d8008d46796902ff390653822af6cc4:

  Merge tag 'mmc-v4.14-2' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc (2017-09-18 08:44:51 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20170927

for you to fetch changes up to 428490e38b2e352812e0b765d8bceafab0ec441d:

  security/keys: rewrite all of big_key crypto (2017-09-25 23:31:58 +0100)

----------------------------------------------------------------
Keyrings fixes

----------------------------------------------------------------
Eric Biggers (10):
      KEYS: fix cred refcount leak in request_key_auth_new()
      KEYS: don't revoke uninstantiated key in request_key_auth_new()
      KEYS: fix key refcount leak in keyctl_assume_authority()
      KEYS: fix key refcount leak in keyctl_read_key()
      KEYS: fix writing past end of user-supplied buffer in keyring_read()
      KEYS: prevent creating a different user's keyrings
      KEYS: prevent KEYCTL_READ on negative key
      KEYS: reset parent each time before searching key_user_tree
      KEYS: restrict /proc/keys by credentials at open time
      KEYS: use kmemdup() in request_key_auth_new()

Jason A. Donenfeld (2):
      security/keys: properly zero out sensitive key material in big_key
      security/keys: rewrite all of big_key crypto

 include/linux/key.h              |   2 +
 security/keys/Kconfig            |   4 +-
 security/keys/big_key.c          | 139 ++++++++++++++++++---------------------
 security/keys/internal.h         |   2 +-
 security/keys/key.c              |   6 +-
 security/keys/keyctl.c           |  13 ++--
 security/keys/keyring.c          |  37 ++++++-----
 security/keys/proc.c             |   8 +--
 security/keys/process_keys.c     |   6 +-
 security/keys/request_key_auth.c |  74 ++++++++++-----------
 10 files changed, 139 insertions(+), 152 deletions(-)

[toc] | [next] | [standalone]

#1741089

On Wed, 27 Sep 2017, David Howells wrote:

>  (2) Fixing big_key to use safe crypto from Jason A. Donenfeld.
> 

I'm concerned about the lack of crypto review mentioned by Jason -- I 
wonder if we can get this rewrite any more review from crypto folk.

Also, are there any tests for this code?  If not, it would be good to make 
some.


-- 
James Morris
<jmorris@namei.org>

[toc] | [prev] | [next] | [standalone]

#1741109

On Thu, Sep 28, 2017 at 09:14:58AM +1000, James Morris wrote:
> On Wed, 27 Sep 2017, David Howells wrote:
> 
> >  (2) Fixing big_key to use safe crypto from Jason A. Donenfeld.
> > 
> 
> I'm concerned about the lack of crypto review mentioned by Jason -- I 
> wonder if we can get this rewrite any more review from crypto folk.
> 
> Also, are there any tests for this code?  If not, it would be good to make 
> some.
> 

There is a test for the big_key key type in the keyutils test suite.  I also
manually tested Jason's change.  And as far as I can tell there isn't actually a
whole lot to test besides adding a big_key larger than BIG_KEY_FILE_THRESHOLD
bytes, reading it back, and verifying that the data is unchanged --- since that
covers the code that was changed.  An earlier version of the patch produced a
warning with CONFIG_DEBUG_SG=y since it put the aead_request on the stack, but
that's been fixed.

It would be great if someone else would comment on the crypto too, but for what
it's worth I'm satisfied with the crypto changes.  GCM is a much better choice
than ECB as long as we don't repeat (key, IV) pairs --- which we don't.  And in
any case ECB mode makes no sense in this context; you'd need a *very* good
reason to actually choose to encrypt something with ECB mode.  Unfortunately it
tends to be a favorite of people who don't understand encryption modes...

Plus, getting all the randomness at boot time didn't make sense because that's
when entropy is the most scarce.

Eric

[toc] | [prev] | [next] | [standalone]

#1741139

On Wed, 27 Sep 2017, Eric Biggers wrote:

> On Thu, Sep 28, 2017 at 09:14:58AM +1000, James Morris wrote:
> > On Wed, 27 Sep 2017, David Howells wrote:
> > 
> > >  (2) Fixing big_key to use safe crypto from Jason A. Donenfeld.
> > > 
> > 
> > I'm concerned about the lack of crypto review mentioned by Jason -- I 
> > wonder if we can get this rewrite any more review from crypto folk.
> > 
> > Also, are there any tests for this code?  If not, it would be good to make 
> > some.
> > 
> 
> There is a test for the big_key key type in the keyutils test suite.  I also
> manually tested Jason's change.  And as far as I can tell there isn't actually a
> whole lot to test besides adding a big_key larger than BIG_KEY_FILE_THRESHOLD
> bytes, reading it back, and verifying that the data is unchanged --- since that
> covers the code that was changed.  An earlier version of the patch produced a
> warning with CONFIG_DEBUG_SG=y since it put the aead_request on the stack, but
> that's been fixed.
> 

Ok, thanks a lot.

> It would be great if someone else would comment on the crypto too, but for what
> it's worth I'm satisfied with the crypto changes.  GCM is a much better choice
> than ECB as long as we don't repeat (key, IV) pairs --- which we don't.  And in
> any case ECB mode makes no sense in this context; you'd need a *very* good
> reason to actually choose to encrypt something with ECB mode.  Unfortunately it
> tends to be a favorite of people who don't understand encryption modes...

Adding Herbert.


-- 
James Morris
<jmorris@namei.org>

[toc] | [prev] | [next] | [standalone]

#1741402

On Thu, Sep 28, 2017 at 12:08:36PM +1000, James Morris wrote:
> On Wed, 27 Sep 2017, Eric Biggers wrote:
> 
> > On Thu, Sep 28, 2017 at 09:14:58AM +1000, James Morris wrote:
> > > On Wed, 27 Sep 2017, David Howells wrote:
> > > 
> > > >  (2) Fixing big_key to use safe crypto from Jason A. Donenfeld.
> > > > 
> > > 
> > > I'm concerned about the lack of crypto review mentioned by Jason -- I 
> > > wonder if we can get this rewrite any more review from crypto folk.
> > > 
> > > Also, are there any tests for this code?  If not, it would be good to make 
> > > some.
> > > 
> > 
> > There is a test for the big_key key type in the keyutils test suite.  I also
> > manually tested Jason's change.  And as far as I can tell there isn't actually a
> > whole lot to test besides adding a big_key larger than BIG_KEY_FILE_THRESHOLD
> > bytes, reading it back, and verifying that the data is unchanged --- since that
> > covers the code that was changed.  An earlier version of the patch produced a
> > warning with CONFIG_DEBUG_SG=y since it put the aead_request on the stack, but
> > that's been fixed.
> > 
> 
> Ok, thanks a lot.
> 
> > It would be great if someone else would comment on the crypto too, but for what
> > it's worth I'm satisfied with the crypto changes.  GCM is a much better choice
> > than ECB as long as we don't repeat (key, IV) pairs --- which we don't.  And in
> > any case ECB mode makes no sense in this context; you'd need a *very* good
> > reason to actually choose to encrypt something with ECB mode.  Unfortunately it
> > tends to be a favorite of people who don't understand encryption modes...
> 
> Adding Herbert.

I think Jason's patch is definitely an improvement over the status quo.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web