Skip to contentSkip to conversation

security/keys: make BIG_KEYS dependent on stdrng.

PATCH In review Artem Savkov 4 participants 12 messages Sep 5, 2016 – Oct 25, 2016

Subsystems (2)
Security (LSM) Keys/keyrings
lore
Share & tools
lore

Artem Savkov

author
#0 lore raw

PATCH 1/1 security/keys: make BIG_KEYS dependent on stdrng.

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 <[email protected]>
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
security/keys/Kconfig +1 -1
index f826e87..d942c7c 100644
@@ -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
Signature · 1 line
2.7.4
maintainer
#1 lore raw

security/keys/Kconfig config BIG_KEYS · line 44

Quoted 2 lines · Artem Savkov
security/keys/Kconfig +1 -1
-	select CRYPTO
+	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)

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

David

author
#2 lore raw
Quoted 6 lines · David Howells

security/keys/Kconfig config BIG_KEYS · line 44

Quoted 2 lines · Artem Savkov
security/keys/Kconfig +1 -1
-	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.

Signature · 2 lines
Regards,
  Artem
maintainer
#3 lore raw
Quoted 7 lines · Artem Savkov
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-	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 <[email protected]>
Date: Mon, 8 Aug 2016 23:19:32 +0200

KEYS: fix big_key dependency

Signed-off-by Kirill Marinushkin <[email protected]>
cc David Howells <[email protected]>
cc Peter Hlavaty <[email protected]>
cc Greg KH <[email protected]>
cc [email protected]
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
security/keys/Kconfig +1 -1
index f826e87..8213221 100644
@@ -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
#4 lore raw

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

Hi David,

Quoted 11 lines
Quoted 7 lines · Artem Savkov
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-	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

author
#5 lore raw
In reply to David Howells
Quoted 35 lines · David Howells
Quoted 7 lines · Artem Savkov
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-	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 <[email protected]>
Date: Mon, 8 Aug 2016 23:19:32 +0200

KEYS: fix big_key dependency

Signed-off-by Kirill Marinushkin <[email protected]>
cc David Howells <[email protected]>
cc Peter Hlavaty <[email protected]>
cc Greg KH <[email protected]>
cc [email protected]
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
security/keys/Kconfig +0 -0
index f826e87..8213221 100644

security/keys/Kconfig config BIG_KEYS · line 50

Quoted 8 lines
security/keys/Kconfig +1 -1
@@ -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.

Signature · 2 lines
Regards,
  Artem
#6 lore raw
In reply to David Howells
Quoted 2 lines

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 <[email protected]>
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 <[email protected]>
---
 security/keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
security/keys/Kconfig +1 -1
index f826e87..5bc5114 100644
@@ -44,7 +44,7 @@ config BIG_KEYS
        select CRYPTO
        select CRYPTO_AES
        select CRYPTO_ECB
-       select CRYPTO_RNG
+       select CRYPTO_RNG_DEFAULT
maintainer
#7 lore raw
Quoted 5 lines · Kirill Marinushkin

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

author
#8 lore raw
Quoted 9 lines · David Howells
Quoted 5 lines · Kirill Marinushkin

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?

Signature · 2 lines
Regards,
  Artem
maintainer
#9 lore raw
Quoted 11 lines · Artem Savkov
Quoted 7 lines
Quoted 5 lines

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

author
#10 lore raw
Quoted 8 lines · David Howells
Quoted 6 lines · Artem Savkov
Quoted 6 lines
Quoted 5 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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.

Quoted 2 lines

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.

Signature · 2 lines
Regards,
  Artem
maintainer
#11 lore raw
Quoted 3 lines · Artem Savkov
Quoted 1 line

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.

Quoted 4 lines
Quoted 2 lines

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