[PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Artem Savkov]

Since BIG_KEYS can't be compiled as module it requires one of the "stdrng"
providers to be compiled into kernel. Otherwise big_key_crypto_init() fails
on crypto_alloc_rng step and next dereference of big_key_skcipher (e.g. in
big_key_preparse()) results in a NULL pointer dereference.

Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index f826e87..d942c7c 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -41,7 +41,7 @@ config BIG_KEYS
 	bool "Large payload keys"
 	depends on KEYS
 	depends on TMPFS
-	select CRYPTO
+	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
 	select CRYPTO_AES
 	select CRYPTO_ECB
 	select CRYPTO_RNG
-- 
2.7.4

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[David Howells]

Artem Savkov <asavkov@redhat.com> wrote:

> -	select CRYPTO
> +	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)

Should those be "==" not "="?

David

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Artem Savkov]

On Tue, Sep 06, 2016 at 01:58:49PM +0100, David Howells wrote:
> Artem Savkov <asavkov@redhat.com> wrote:
> 
> > -	select CRYPTO
> > +	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
> 
> Should those be "==" not "="?

Accodring to Documentation/kbuild/kconfig-language.txt (line 173) it is
"=" and I can only see "=" being used in existing Kconfigs.

-- 
Regards,
  Artem

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[David Howells]

Artem Savkov <asavkov@redhat.com> wrote:

> > > -	select CRYPTO
> > > +	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
> > 
> > Should those be "==" not "="?
> 
> Accodring to Documentation/kbuild/kconfig-language.txt (line 173) it is
> "=" and I can only see "=" being used in existing Kconfigs.

Okay.  The other thing is that I have been given a conflicting patch (see
below).  Is your fix preferable?

David
---
commit 69ed34b303f87a1a53470dd37149ac1573d79da2
Author: Kirill Marinushkin <k.marinushkin@gmail.com>
Date: Mon, 8 Aug 2016 23:19:32 +0200

KEYS: fix big_key dependency

Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
cc: David Howells <dhowells@redhat.com>
cc: Peter Hlavaty <zer0mem@yahoo.com>
cc: Greg KH <gregkh@linuxfoundation.org>
cc: stable@vger.kernel.org
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index f826e87..8213221 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -44,7 +44,7 @@ config BIG_KEYS
 	select CRYPTO
 	select CRYPTO_AES
 	select CRYPTO_ECB
-	select CRYPTO_RNG
+	select CRYPTO_ANSI_CPRNG
 	help
 	  This option provides support for holding large keys within the kernel
 	  (for example Kerberos ticket caches).  The data may be stored out to

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Stephan Mueller]

Am Dienstag, 6. September 2016, 14:11:56 CEST schrieb David Howells:

Hi David,

> Artem Savkov <asavkov@redhat.com> wrote:
> > > > -	select CRYPTO
> > > > +	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
> > > 
> > > Should those be "==" not "="?
> > 
> > Accodring to Documentation/kbuild/kconfig-language.txt (line 173) it is
> > "=" and I can only see "=" being used in existing Kconfigs.
> 
> Okay.  The other thing is that I have been given a conflicting patch (see
> below).  Is your fix preferable?

The listed patch only selects the ANSI X9.31 DRNG and thus conflicts with FIPS 
mode and the current default stdrng which is the DRBG.

Ciao
Stephan

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Artem Savkov]

On Tue, Sep 06, 2016 at 02:11:56PM +0100, David Howells wrote:
> Artem Savkov <asavkov@redhat.com> wrote:
> 
> > > > -	select CRYPTO
> > > > +	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
> > > 
> > > Should those be "==" not "="?
> > 
> > Accodring to Documentation/kbuild/kconfig-language.txt (line 173) it is
> > "=" and I can only see "=" being used in existing Kconfigs.
> 
> Okay.  The other thing is that I have been given a conflicting patch (see
> below).  Is your fix preferable?
> 
> David
> ---
> commit 69ed34b303f87a1a53470dd37149ac1573d79da2
> Author: Kirill Marinushkin <k.marinushkin@gmail.com>
> Date: Mon, 8 Aug 2016 23:19:32 +0200
> 
> KEYS: fix big_key dependency
> 
> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
> cc: David Howells <dhowells@redhat.com>
> cc: Peter Hlavaty <zer0mem@yahoo.com>
> cc: Greg KH <gregkh@linuxfoundation.org>
> cc: stable@vger.kernel.org
> ---
>  security/keys/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index f826e87..8213221 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -44,7 +44,7 @@ config BIG_KEYS
>  	select CRYPTO
>  	select CRYPTO_AES
>  	select CRYPTO_ECB
> -	select CRYPTO_RNG
> +	select CRYPTO_ANSI_CPRNG
>  	help
>  	  This option provides support for holding large keys within the kernel
>  	  (for example Kerberos ticket caches).  The data may be stored out to

I would argue that locking a user into a specific stdrng implementation
is not something that should be done when there are options available.

-- 
Regards,
  Artem

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Kirill Marinushkin]

> The other thing is that I have been given a conflicting patch (see
> below).  Is your fix preferable?

The patch you attached previously is v1; I submitted v2 after review by
Stephan Mueller. I additionally attach it here below.

IMO, the preferable fix depends on your future plan.
If you plan to continue using both ANSI X9.31 DRNG and DRBG - I agree with the
patch suggested by Artem Savkov.
If you plan to reduce using ANSI X9.31 DRNG and use DRBG more widely - I
suggest my patch.

Best Regards,
Kirill

---
From	Kirill Marinushkin <k.marinushkin@gmail.com>
Subject	[PATCH v2] KEYS: fix big_key dependency
Date	Tue, 16 Aug 2016 21:51:12 +0200

This patch fixes the following bug:
[oss-security] - panic at big_key_preparse #4.7-r6/rc7 & master

Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index f826e87..5bc5114 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -44,7 +44,7 @@ config BIG_KEYS
        select CRYPTO
        select CRYPTO_AES
        select CRYPTO_ECB
-       select CRYPTO_RNG
+       select CRYPTO_RNG_DEFAULT

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[David Howells]

Kirill Marinushkin <k.marinushkin@gmail.com> wrote:

> IMO, the preferable fix depends on your future plan.
> If you plan to continue using both ANSI X9.31 DRNG and DRBG - I agree with the
> patch suggested by Artem Savkov.
> If you plan to reduce using ANSI X9.31 DRNG and use DRBG more widely - I
> suggest my patch.

No such plans, TBH.

David

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Artem Savkov]

On Tue, Sep 06, 2016 at 07:16:13PM +0100, David Howells wrote:
> Kirill Marinushkin <k.marinushkin@gmail.com> wrote:
> 
> > IMO, the preferable fix depends on your future plan.
> > If you plan to continue using both ANSI X9.31 DRNG and DRBG - I agree with the
> > patch suggested by Artem Savkov.
> > If you plan to reduce using ANSI X9.31 DRNG and use DRBG more widely - I
> > suggest my patch.
> 
> No such plans, TBH.

I agre with Kirill here, so if we are not trying to reduce ANSI X9.31
DRNG usage can we move on with the suggested patch, or are there any
issues with it that need addressing?

-- 
Regards,
  Artem

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[David Howells]

Artem Savkov <asavkov@redhat.com> wrote:

> > > IMO, the preferable fix depends on your future plan.
> > > If you plan to continue using both ANSI X9.31 DRNG and DRBG - I agree with the
> > > patch suggested by Artem Savkov.
> > > If you plan to reduce using ANSI X9.31 DRNG and use DRBG more widely - I
> > > suggest my patch.
> > 
> > No such plans, TBH.
> 
> I agre with Kirill here, so if we are not trying to reduce ANSI X9.31
> DRNG usage can we move on with the suggested patch, or are there any
> issues with it that need addressing?

Which suggested patch?  One of Kirill's (there are at least two) or yours?

Note that we *also* need the "KEYS: Sort out big_key initialisation" patch -
just changing the Kconfig is not sufficient a fix in and of itself.

David

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[Artem Savkov]

On Mon, Oct 24, 2016 at 03:50:54PM +0100, David Howells wrote:
> Artem Savkov <asavkov@redhat.com> wrote:
> 
> > > > IMO, the preferable fix depends on your future plan.
> > > > If you plan to continue using both ANSI X9.31 DRNG and DRBG - I agree with the
> > > > patch suggested by Artem Savkov.
> > > > If you plan to reduce using ANSI X9.31 DRNG and use DRBG more widely - I
> > > > suggest my patch.
> > > 
> > > No such plans, TBH.
> > 
> > I agre with Kirill here, so if we are not trying to reduce ANSI X9.31
> > DRNG usage can we move on with the suggested patch, or are there any
> > issues with it that need addressing?
> 
> Which suggested patch?  One of Kirill's (there are at least two) or yours?

I suggest mine, since it is more flexible.

> Note that we *also* need the "KEYS: Sort out big_key initialisation" patch -
> just changing the Kconfig is not sufficient a fix in and of itself.

Right, I see it also changes the Kconfig, so we might be better off with
v2 of "KEYS: Sort out big_key initialisation" with "depends on
(CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)" in Kconfig.

-- 
Regards,
  Artem

Re: [PATCH] security/keys: make BIG_KEYS dependent on stdrng.

[David Howells]

Artem Savkov <asavkov@redhat.com> wrote:

> > Which suggested patch?  One of Kirill's (there are at least two) or yours?
> 
> I suggest mine, since it is more flexible.

Fine by me.

> > Note that we *also* need the "KEYS: Sort out big_key initialisation" patch -
> > just changing the Kconfig is not sufficient a fix in and of itself.
> 
> Right, I see it also changes the Kconfig

No, it doesn't.  It only changes big_key.c

David