Skip to contentSkip to conversation

certs: Move RSA self-test data to separate file

PATCH Superseded Joachim Vandersmissen 2 participants 5 messages Apr 20, 2024 – May 1, 2024

Subsystems (2)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/2 certs: Move RSA self-test data to separate file

Herbert, please let me know if this is what you had in mind. Thanks.

---8<---

In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. We only need to compile this file in if CRYPTO_RSA is configured. The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.

---
 crypto/asymmetric_keys/Makefile       |   1 +
 crypto/asymmetric_keys/selftest.c     | 218 ++++----------------------
 crypto/asymmetric_keys/selftest.h     |  16 ++
 crypto/asymmetric_keys/selftest_rsa.c | 172 ++++++++++++++++++++
 4 files changed, 218 insertions(+), 189 deletions(-)
 create mode 100644 crypto/asymmetric_keys/selftest.h
 create mode 100644 crypto/asymmetric_keys/selftest_rsa.c
crypto/asymmetric_keys/Makefile +1 -0
index 1a273d6df3eb..4db6968132e9 100644
@@ -24,6 +24,7 @@ x509_key_parser-y := \
 	x509_public_key.o
 obj-$(CONFIG_FIPS_SIGNATURE_SELFTEST) += x509_selftest.o
 x509_selftest-y += selftest.o
+x509_selftest-$(CONFIG_CRYPTO_RSA) += selftest_rsa.o
 
 $(obj)/x509_cert_parser.o: \
 	$(obj)/x509.asn1.h \
crypto/asymmetric_keys/selftest.c +29 -189

251 lines not on this page · raw message

crypto/asymmetric_keys/selftest.h new file +16 -0
new file mode 100644
index 000000000000..3203de306619
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Helper function for self-testing PKCS#7 signature verification.
+ *
+ * Copyright (C) 2024 Joachim Vandersmissen <[email protected]>
+ */
+
+void fips_signature_selftest(const char *name,
+			     const u8 *keys, size_t keys_len,
+			     const u8 *data, size_t data_len,
+			     const u8 *sig, size_t sig_len);
+
+#ifdef CONFIG_CRYPTO_RSA
+void __init fips_signature_selftest_rsa(void);
+#else
+static inline void __init fips_signature_selftest_rsa(void) { }
+#endif
crypto/asymmetric_keys/selftest_rsa.c new file +172 -0

172 lines not on this page · raw message

Signature · 1 line
2.44.0
#1 lore raw

PATCH v2 2/2 certs: Add ECDSA signature verification self-test

Commit c27b2d2012e1 ("crypto: testmgr - allow ecdsa-nist-p256 and -p384 in FIPS mode") enabled support for ECDSA in crypto/testmgr.c. The PKCS#7 signature verification API builds upon the KCAPI primitives to perform its high-level operations. Therefore, this change in testmgr.c also allows ECDSA to be used by the PKCS#7 signature verification API (in FIPS mode).

However, from a FIPS perspective, the PKCS#7 signature verification API is a distinct "service" from the KCAPI primitives. This is because the PKCS#7 API performs a "full" signature verification, which consists of both hashing the data to be verified, and the public key operation. On the other hand, the KCAPI primitive does not perform this hashing step - it accepts pre-hashed data from the caller and only performs the public key operation.

For this reason, the ECDSA self-tests in crypto/testmgr.c are not sufficient to cover ECDSA signature verification offered by the PKCS#7 API. This is reflected by the self-test already present in this file for RSA PKCS#1 v1.5 signature verification.

The solution is simply to add a second self-test here for ECDSA. P-256 with SHA-256 hashing was chosen as those parameters should remain FIPS-approved for the foreseeable future, while keeping the performance impact to a minimum. The ECDSA certificate and PKCS#7 signed data was generated using OpenSSL. The input data is identical to the input data for the existing RSA self-test.

---
 crypto/asymmetric_keys/Makefile         |  1 +
 crypto/asymmetric_keys/selftest.c       |  1 +
 crypto/asymmetric_keys/selftest.h       |  6 ++
 crypto/asymmetric_keys/selftest_ecdsa.c | 89 +++++++++++++++++++++++++
 4 files changed, 97 insertions(+)
 create mode 100644 crypto/asymmetric_keys/selftest_ecdsa.c
crypto/asymmetric_keys/Makefile +1 -0
index 4db6968132e9..55728d7aa979 100644
@@ -25,6 +25,7 @@ x509_key_parser-y := \
 obj-$(CONFIG_FIPS_SIGNATURE_SELFTEST) += x509_selftest.o
 x509_selftest-y += selftest.o
 x509_selftest-$(CONFIG_CRYPTO_RSA) += selftest_rsa.o
+x509_selftest-$(CONFIG_CRYPTO_ECDSA) += selftest_ecdsa.o
 
 $(obj)/x509_cert_parser.o: \
 	$(obj)/x509.asn1.h \
crypto/asymmetric_keys/selftest.c +1 -0
index ec289d2d065c..98dc5cdfdebe 100644
@@ -61,6 +61,7 @@ void fips_signature_selftest(const char *name,
 static int __init fips_signature_selftest_init(void)
 {
 	fips_signature_selftest_rsa();
+	fips_signature_selftest_ecdsa();
 	return 0;
 }
 
crypto/asymmetric_keys/selftest.h +6 -0
index 3203de306619..279c8f22ca3d 100644
@@ -14,3 +14,9 @@ void __init fips_signature_selftest_rsa(void);
 #else
 static inline void __init fips_signature_selftest_rsa(void) { }
 #endif
+
+#ifdef CONFIG_CRYPTO_ECDSA
+void __init fips_signature_selftest_ecdsa(void);
+#else
+static inline void __init fips_signature_selftest_ecdsa(void) { }
+#endif
crypto/asymmetric_keys/selftest_ecdsa.c new file +89 -0

89 lines not on this page · raw message

Signature · 1 line
2.44.0
maintainer
#2 lore raw
In reply to Joachim Vandersmissen
Quoted 1 line · Joachim Vandersmissen

Herbert, please let me know if this is what you had in mind. Thanks.

Thanks, it's pretty much what I had in mind.

crypto/asymmetric_keys/Makefile x509_key_parser-y := \ · line 27

Quoted 9 lines
crypto/asymmetric_keys/Makefile +1 -0
index 1a273d6df3eb..4db6968132e9 100644
@@ -24,6 +24,7 @@ x509_key_parser-y := \
 	x509_public_key.o
 obj-$(CONFIG_FIPS_SIGNATURE_SELFTEST) += x509_selftest.o
 x509_selftest-y += selftest.o
+x509_selftest-$(CONFIG_CRYPTO_RSA) += selftest_rsa.o

This doesn't work if RSA is a module. So you need to play a bit more of a game with Kconfig to get it to work. Perhaps define an extra Kconfig option for it:

config FIPS_SIGNATURE_SELFTEST_RSA

	def_bool (FIPS_SIGNATURE_SELFTEST=m && CRYPTO_RSA!=n) || CRYPTO_RSA=y

and then

x509_selftest-$(CONFIG_FIPS_SIGNATURE_SELFTEST_RSA) += selftest_rsa.o

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#3 lore raw

Hi Herbert,

Quoted 17 lines · Herbert Xu
Quoted 1 line · Joachim Vandersmissen

Herbert, please let me know if this is what you had in mind. Thanks.

Thanks, it's pretty much what I had in mind.

crypto/asymmetric_keys/Makefile x509_key_parser-y := \ · line 27

Quoted 9 lines
crypto/asymmetric_keys/Makefile +1 -0
index 1a273d6df3eb..4db6968132e9 100644
@@ -24,6 +24,7 @@ x509_key_parser-y := \
  	x509_public_key.o
  obj-$(CONFIG_FIPS_SIGNATURE_SELFTEST) += x509_selftest.o
  x509_selftest-y += selftest.o
+x509_selftest-$(CONFIG_CRYPTO_RSA) += selftest_rsa.o

This doesn't work if RSA is a module. So you need to play a bit more of a game with Kconfig to get it to work. Perhaps define an extra Kconfig option for it:

Quoted 6 lines

config FIPS_SIGNATURE_SELFTEST_RSA

	def_bool (FIPS_SIGNATURE_SELFTEST=m && CRYPTO_RSA!=n) || CRYPTO_RSA=y

and then

x509_selftest-$(CONFIG_FIPS_SIGNATURE_SELFTEST_RSA) += selftest_rsa.o

After thinking about it for a while, I understand what you mean now. The current behavior of the patch seems to be that, if FIPS_SIGNATURE_SELFTEST=y but CRYPTO_RSA=m, the RSA signature self-test will not be executed. I believe your suggestion would explicitly encode that behavior in the Kconfig?

The most correct solution in that case would probably be executing the PKCS#7 self-test when the RSA module is loaded, but I don't think that's feasible in the current architecture.

Another option would be to simply add CRYPTO_RSA and CRYPTO_ECDSA as explicit dependencies to FIPS_SIGNATURE_SELFTEST, as Eric Biggers proposed. Perhaps nowadays everyone includes ECDSA already.

I'm currently leaning towards adding FIPS_SIGNATURE_SELFTEST_RSA (and similarly FIPS_SIGNATURE_SELFTEST_ECDSA) as user-facing configuration options that depend on CRYPTO_RSA (and CRYPTO_ECDSA) and FIPS_SIGNATURE_SELFTEST. Then, it is up to the user to select the correct self-tests they need. It would still allow the user to create the same configuration "error" where FIPS_SIGNATURE_SELFTEST=y and FIPS_SIGNATURE_SELFTEST_RSA=m, but I think that users which care about FIPS_SIGNATURE_SELFTEST are doing it in the first place for FIPS compliance reasons. In that case, a FIPS laboratory should review the configuration to verify that the correct self-tests are executed at the correct time.

Quoted 2 lines

Thanks,

maintainer
#4 lore raw
Quoted 11 lines · Joachim Vandersmissen

I'm currently leaning towards adding FIPS_SIGNATURE_SELFTEST_RSA (and similarly FIPS_SIGNATURE_SELFTEST_ECDSA) as user-facing configuration options that depend on CRYPTO_RSA (and CRYPTO_ECDSA) and FIPS_SIGNATURE_SELFTEST. Then, it is up to the user to select the correct self-tests they need. It would still allow the user to create the same configuration "error" where FIPS_SIGNATURE_SELFTEST=y and FIPS_SIGNATURE_SELFTEST_RSA=m, but I think that users which care about FIPS_SIGNATURE_SELFTEST are doing it in the first place for FIPS compliance reasons. In that case, a FIPS laboratory should review the configuration to verify that the correct self-tests are executed at the correct time.

If the combo results in a crash then I think we have to fix it.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt