Re: [GIT PULL] KEYS: Fixes and crypto fixes

[Herbert Xu]

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 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [GIT PULL] KEYS: Fixes and crypto fixes

[James Morris]

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

Re: [GIT PULL] KEYS: Fixes and crypto fixes

[Eric Biggers]

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

Re: [GIT PULL] KEYS: Fixes and crypto fixes

[James Morris]

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

[GIT PULL] KEYS: Fixes and crypto fixes

[David Howells]

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