certs: Move RSA self-test data to separate file

PATCH Superseded Joachim Vandersmissen 3 participants 10 messages May 11, 2024 – May 13, 2024

Subsystems (2)

Crypto API Asymmetric keys

lore

Share & tools

lore

Joachim Vandersmissen

author

2024-05-11 06:24 UTC #0 lore raw

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

v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.

---8<---

In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.
The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.

Signed-off-by Joachim Vandersmissen <[email protected]>

---
 crypto/asymmetric_keys/Kconfig        |   7 +
 crypto/asymmetric_keys/Makefile       |   1 +
 crypto/asymmetric_keys/selftest.c     | 218 ++++----------------------
 crypto/asymmetric_keys/selftest.h     |  16 ++
 crypto/asymmetric_keys/selftest_rsa.c | 172 ++++++++++++++++++++
 5 files changed, 225 insertions(+), 189 deletions(-)
 create mode 100644 crypto/asymmetric_keys/selftest.h
 create mode 100644 crypto/asymmetric_keys/selftest_rsa.c

crypto/asymmetric_keys/Kconfig +7 -0

index 59ec726b7c77..33bbfd0d8367 100644
@@ -86,4 +86,11 @@ config FIPS_SIGNATURE_SELFTEST
 	depends on PKCS7_MESSAGE_PARSER=X509_CERTIFICATE_PARSER
 	depends on X509_CERTIFICATE_PARSER
 
+config FIPS_SIGNATURE_SELFTEST_RSA
+	bool
+	default y
+	depends on FIPS_SIGNATURE_SELFTEST
+	depends on CRYPTO_SHA256=y || CRYPTO_SHA256=FIPS_SIGNATURE_SELFTEST
+	depends on CRYPTO_RSA=y || CRYPTO_RSA=FIPS_SIGNATURE_SELFTEST
+
 endif # ASYMMETRIC_KEY_TYPE

crypto/asymmetric_keys/Makefile +1 -0

index 1a273d6df3eb..ac1402e27324 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_FIPS_SIGNATURE_SELFTEST_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..842ac3cf86b4
@@ -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_FIPS_SIGNATURE_SELFTEST_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.45.0

Joachim Vandersmissen

author

2024-05-11 06:24 UTC #1 lore raw

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

v4: FIPS_SIGNATURE_SELFTEST_ECDSA is no longer user-configurable and will be set when the dependencies are fulfilled.

---8<---

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.

Signed-off-by Joachim Vandersmissen <[email protected]>

---
 crypto/asymmetric_keys/Kconfig          |  7 ++
 crypto/asymmetric_keys/Makefile         |  1 +
 crypto/asymmetric_keys/selftest.c       |  1 +
 crypto/asymmetric_keys/selftest.h       |  6 ++
 crypto/asymmetric_keys/selftest_ecdsa.c | 89 +++++++++++++++++++++++++
 5 files changed, 104 insertions(+)
 create mode 100644 crypto/asymmetric_keys/selftest_ecdsa.c

crypto/asymmetric_keys/Kconfig +7 -0

index 33bbfd0d8367..06e64e29587e 100644
@@ -93,4 +93,11 @@ config FIPS_SIGNATURE_SELFTEST_RSA
 	depends on CRYPTO_SHA256=y || CRYPTO_SHA256=FIPS_SIGNATURE_SELFTEST
 	depends on CRYPTO_RSA=y || CRYPTO_RSA=FIPS_SIGNATURE_SELFTEST
 
+config FIPS_SIGNATURE_SELFTEST_ECDSA
+	bool
+	default y
+	depends on FIPS_SIGNATURE_SELFTEST
+	depends on CRYPTO_SHA256=y || CRYPTO_SHA256=FIPS_SIGNATURE_SELFTEST
+	depends on CRYPTO_ECDSA=y || CRYPTO_ECDSA=FIPS_SIGNATURE_SELFTEST
+
 endif # ASYMMETRIC_KEY_TYPE

crypto/asymmetric_keys/Makefile +1 -0

index ac1402e27324..bc65d3b98dcb 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_FIPS_SIGNATURE_SELFTEST_RSA) += selftest_rsa.o
+x509_selftest-$(CONFIG_FIPS_SIGNATURE_SELFTEST_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 842ac3cf86b4..4139f05906cb 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_FIPS_SIGNATURE_SELFTEST_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.45.0

Herbert Xu

maintainer Acked-by on v4 2/2

2024-05-12 07:16 UTC #2 lore raw

Re: [PATCH v4 2/2] certs: Add ECDSA signature verification self-test

Quoted 33 lines · Joachim Vandersmissen

v4: FIPS_SIGNATURE_SELFTEST_ECDSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---
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.
Signed-off-by Joachim Vandersmissen <[email protected]>

Quoted 8 lines

---
 crypto/asymmetric_keys/Kconfig          |  7 ++
 crypto/asymmetric_keys/Makefile         |  1 +
 crypto/asymmetric_keys/selftest.c       |  1 +
 crypto/asymmetric_keys/selftest.h       |  6 ++
 crypto/asymmetric_keys/selftest_ecdsa.c | 89 +++++++++++++++++++++++++
 5 files changed, 104 insertions(+)
 create mode 100644 crypto/asymmetric_keys/selftest_ecdsa.c

Acked-by Herbert Xu <[email protected]>

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

Herbert Xu

maintainer Acked-by on v4 1/2

2024-05-12 07:16 UTC #3 lore raw

In reply to Joachim Vandersmissen

Quoted 14 lines · Joachim Vandersmissen

v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---
In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.
The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.
Signed-off-by Joachim Vandersmissen <[email protected]>

Quoted 9 lines

---
 crypto/asymmetric_keys/Kconfig        |   7 +
 crypto/asymmetric_keys/Makefile       |   1 +
 crypto/asymmetric_keys/selftest.c     | 218 ++++----------------------
 crypto/asymmetric_keys/selftest.h     |  16 ++
 crypto/asymmetric_keys/selftest_rsa.c | 172 ++++++++++++++++++++
 5 files changed, 225 insertions(+), 189 deletions(-)
 create mode 100644 crypto/asymmetric_keys/selftest.h
 create mode 100644 crypto/asymmetric_keys/selftest_rsa.c

Acked-by Herbert Xu <[email protected]>

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

Jarkko Sakkinen

2024-05-12 23:11 UTC #4 lore raw

In reply to Joachim Vandersmissen

Quoted 4 lines · Joachim Vandersmissen

v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---

This is in wrong place. If the patch is applied it will be included to the kernel git log. Please put your log before diffstat.

Quoted 5 lines

In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.

1. Please just call the thing by its name instead of building tension with "the new configuration option".
2. Lacks the motivation of adding a new configuration option.

Quoted 2 lines

The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.

This is confusing, please remove.

So why just send this and not this plus the selftest? Feels incomplete to me.

BR, Jarkko

Joachim Vandersmissen

author

2024-05-13 01:11 UTC #5 lore raw

Quoted 7 lines · Jarkko Sakkinen

Quoted 4 lines · Joachim Vandersmissen
v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---
This is in wrong place. If the patch is applied it will be included to the kernel git log. Please put your log before diffstat.

I will keep it in mind for the next round.

Quoted 9 lines

Quoted 6 lines
In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.
1. Please just call the thing by its name instead of building tension with "the new configuration option".
2. Lacks the motivation of adding a new configuration option.

The configuration option is there to ensure that the RSA (or ECDSA) self-tests only get compiled in when RSA (or ECDSA) is actually enabled. Otherwise, the self-test will panic on boot. I can make this more explicit in the commit message.

Quoted 4 lines

Quoted 3 lines
The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.
This is confusing, please remove.

Fair enough, I'll remove it from the commit message.

Quoted 3 lines

So why just send this and not this plus the selftest? Feels incomplete to me.

Do you mean the ECDSA self-test? I didn't include that one here because I didn't want to make the commit too big.

Quoted 3 lines

BR, Jarkko

Jarkko Sakkinen

2024-05-13 01:32 UTC #6 lore raw

Quoted 23 lines · Joachim Vandersmissen

Quoted 7 lines · Jarkko Sakkinen
Quoted 4 lines · Joachim Vandersmissen
v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---
This is in wrong place. If the patch is applied it will be included to the kernel git log. Please put your log before diffstat.
I will keep it in mind for the next round.
Quoted 9 lines
Quoted 6 lines
In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.
1. Please just call the thing by its name instead of building tension with "the new configuration option".
2. Lacks the motivation of adding a new configuration option.
The configuration option is there to ensure that the RSA (or ECDSA) self-tests only get compiled in when RSA (or ECDSA) is actually enabled. Otherwise, the self-test will panic on boot. I can make this more explicit in the commit message.
Quoted 1 line

Quoted 4 lines

Quoted 3 lines
Quoted 2 lines
The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.
This is confusing, please remove.
Fair enough, I'll remove it from the commit message.

Yeah, I mean it is good to enough to have a code change no need to document it here :-)

Quoted 6 lines

Quoted 3 lines
So why just send this and not this plus the selftest? Feels incomplete to me.
Do you mean the ECDSA self-test? I didn't include that one here because I didn't want to make the commit too big.

So, I'd suggest to make a patch set with the second patch containing the tests.

BR, Jarkko

Joachim Vandersmissen

author

2024-05-13 01:43 UTC #7 lore raw

Quoted 29 lines · Jarkko Sakkinen

Quoted 17 lines · Joachim Vandersmissen
Quoted 7 lines · Jarkko Sakkinen
Quoted 4 lines · Joachim Vandersmissen
v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will be set when the dependencies are fulfilled.
---8<---
This is in wrong place. If the patch is applied it will be included to the kernel git log. Please put your log before diffstat.
I will keep it in mind for the next round.
Quoted 8 lines
Quoted 5 lines
In preparation of adding new ECDSA self-tests, the existing data is moved to a separate file. A new configuration option is added to control the compilation of the separate file. This configuration option also enforces dependencies that were missing from the existing CONFIG_FIPS_SIGNATURE_SELFTEST option.
1. Please just call the thing by its name instead of building tension with "the new configuration option".
2. Lacks the motivation of adding a new configuration option.
Quoted 8 lines
The configuration option is there to ensure that the RSA (or ECDSA) self-tests only get compiled in when RSA (or ECDSA) is actually enabled. Otherwise, the self-test will panic on boot. I can make this more explicit in the commit message.
Quoted 3 lines
Quoted 2 lines
The old fips_signature_selftest is no longer an init function, but now a helper function called from fips_signature_selftest_rsa.
This is confusing, please remove.
Fair enough, I'll remove it from the commit message.
Yeah, I mean it is good to enough to have a code change no need to document it here :-)

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
So why just send this and not this plus the selftest? Feels incomplete to me.
Do you mean the ECDSA self-test? I didn't include that one here because I didn't want to make the commit too big.
So, I'd suggest to make a patch set with the second patch containing the tests.

I think this is part of a patch set, is it not? There should be a 2/2 patch ("Add ECDSA signature verification self-test"), you should be on CC for that one too.

Quoted 3 lines

BR, Jarkko

Jarkko Sakkinen

2024-05-13 03:27 UTC #8 lore raw

Quoted 3 lines · Joachim Vandersmissen

I think this is part of a patch set, is it not? There should be a 2/2 patch ("Add ECDSA signature verification self-test"), you should be on CC for that one too.

OK found it! Yep, pretty much similar remarks (like putting change log to diffstat).

Also it is best to have full change log instead of the entry just for the latest version. So at least the current v4 entry and then v5...

BR, Jarkko

Jarkko Sakkinen

2024-05-13 03:32 UTC #9 lore raw

Quoted 10 lines · Jarkko Sakkinen

Quoted 3 lines · Joachim Vandersmissen
I think this is part of a patch set, is it not? There should be a 2/2 patch ("Add ECDSA signature verification self-test"), you should be on CC for that one too.
OK found it! Yep, pretty much similar remarks (like putting change log to diffstat).
Also it is best to have full change log instead of the entry just for the latest version. So at least the current v4 entry and then v5...

I'll try to get this still picked to 6.10 i.e. if we can put this during first half of this week, then it should make it.

I'm also working asymmetric keys patch set [1] but that is likely to be postponed to 6.11.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=tpm2_key

BR, Jarkko