diff mbox series

[testsuite,1/1] tests/keys: fix the KEYCTL_DH_COMPUTE parameter values

Message ID 20201014073721.392783-2-omosnace@redhat.com (mailing list archive)
State Accepted
Delegated to: Ondrej Mosnáček
Headers show
Series [testsuite,1/1] tests/keys: fix the KEYCTL_DH_COMPUTE parameter values | expand

Commit Message

Ondrej Mosnacek Oct. 14, 2020, 7:37 a.m. UTC 
The KEYCTL_DH_COMPUTE key payloads are interpreted as MPI-formatted
numbers. Even though the generic DH algorithm implementation doesn't
look at the actual values when setting the parameters (and probably only
validates them later when doing the actual operation), this isn't
necessarily true for other implementations. Specifically, the Intel QAT
driver checks if the prime size is one of 1536, 2048, 3072, or 4096
bits, causing the KEYCTL_DH_COMPUTE operation to fail with -EINVAL.

While it is questionable if the QAT implementation should reject unusual
prime sizes (it should use a fallback to generic instead), let's just
make sure we pass valid numbers to avoid similar validation errors.

I verified on a machine with an Intel QAT device that this patch makes
the testsuite pass there.

Fixes: 2d7aad8a1f8c ("selinux-testsuite: Add keys tests")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 tests/keys/keyctl.c          |  8 ++++----
 tests/keys/keyring_service.c |  8 ++++----
 tests/keys/keys_common.h     | 28 +++++++++++++++++++++++++---
 3 files changed, 33 insertions(+), 11 deletions(-)

Comments

Ondrej Mosnacek Oct. 23, 2020, 2:52 p.m. UTC | #1 
On Wed, Oct 14, 2020 at 9:37 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The KEYCTL_DH_COMPUTE key payloads are interpreted as MPI-formatted
> numbers. Even though the generic DH algorithm implementation doesn't
> look at the actual values when setting the parameters (and probably only
> validates them later when doing the actual operation), this isn't
> necessarily true for other implementations. Specifically, the Intel QAT
> driver checks if the prime size is one of 1536, 2048, 3072, or 4096
> bits, causing the KEYCTL_DH_COMPUTE operation to fail with -EINVAL.
>
> While it is questionable if the QAT implementation should reject unusual
> prime sizes (it should use a fallback to generic instead), let's just
> make sure we pass valid numbers to avoid similar validation errors.
>
> I verified on a machine with an Intel QAT device that this patch makes
> the testsuite pass there.
>
> Fixes: 2d7aad8a1f8c ("selinux-testsuite: Add keys tests")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

I have now applied this patch:
https://github.com/SELinuxProject/selinux-testsuite/commit/19b43c07d87278023b8637eba1be83eb73f22606
diff mbox series

Patch

diff --git a/tests/keys/keyctl.c b/tests/keys/keyctl.c
index 5aff8d8..e13c655 100644
--- a/tests/keys/keyctl.c
+++ b/tests/keys/keyctl.c
@@ -72,15 +72,15 @@  int main(int argc, char *argv[])
 	 * required by the keyctl(KEYCTL_DH_COMPUTE, ..) function.
 	 * These require key { create write } permissions.
 	 */
-	private = add_key("user", "private", payload, strlen(payload),
-			  KEY_SPEC_PROCESS_KEYRING);
+	private = add_key("user", "private", payload_private,
+			  sizeof(payload_private), KEY_SPEC_PROCESS_KEYRING);
 	if (private < 0) {
 		fprintf(stderr, "Failed add_key(private): %s\n",
 			strerror(errno));
 		exit(5);
 	}
 
-	prime = add_key("user", "prime", payload, strlen(payload),
+	prime = add_key("user", "prime", payload_prime, sizeof(payload_prime),
 			KEY_SPEC_PROCESS_KEYRING);
 	if (prime < 0) {
 		fprintf(stderr, "Failed add_key(prime): %s\n",
@@ -88,7 +88,7 @@  int main(int argc, char *argv[])
 		exit(5);
 	}
 
-	base = add_key("user", "base", payload, strlen(payload),
+	base = add_key("user", "base", payload_base, sizeof(payload_base),
 		       KEY_SPEC_PROCESS_KEYRING);
 	if (base < 0) {
 		fprintf(stderr, "Failed add_key(base): %s\n",
diff --git a/tests/keys/keyring_service.c b/tests/keys/keyring_service.c
index e5dcab0..9913653 100644
--- a/tests/keys/keyring_service.c
+++ b/tests/keys/keyring_service.c
@@ -91,15 +91,15 @@  int main(int argc, char **argv)
 		printf("\tKEYCTL_JOIN_SESSION_KEYRING newkey ID: 0x%x\n",
 		       newring);
 
-	private = add_key("user", "private", payload, strlen(payload),
-			  KEY_SPEC_SESSION_KEYRING);
+	private = add_key("user", "private", payload_private,
+			  sizeof(payload_private), KEY_SPEC_SESSION_KEYRING);
 	if (private < 0) {
 		fprintf(stderr, "Failed add_key(private): %s\n",
 			strerror(errno));
 		exit(6);
 	}
 
-	prime = add_key("user", "prime", payload, strlen(payload),
+	prime = add_key("user", "prime", payload_prime, sizeof(payload_prime),
 			KEY_SPEC_SESSION_KEYRING);
 	if (prime < 0) {
 		fprintf(stderr, "Failed add_key(prime): %s\n",
@@ -107,7 +107,7 @@  int main(int argc, char **argv)
 		exit(6);
 	}
 
-	base = add_key("user", "base", payload, strlen(payload),
+	base = add_key("user", "base", payload_base, sizeof(payload_base),
 		       KEY_SPEC_SESSION_KEYRING);
 	if (base < 0) {
 		fprintf(stderr, "Failed add_key(base): %s\n",
diff --git a/tests/keys/keys_common.h b/tests/keys/keys_common.h
index df15280..55cc4eb 100644
--- a/tests/keys/keys_common.h
+++ b/tests/keys/keys_common.h
@@ -11,6 +11,28 @@ 
 #include <selinux/selinux.h>
 #include <selinux/context.h>
 
-/* This is used as the payload for each add_key() */
-static const char payload[] =
-	" -----BEGIN PUBLIC KEY-----\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDN4FHsPjlJf03r9KfNt1Ma9/D6\nQDEiR/cfhZrNUPgHRresult+E4dj52VJSonPFJ6HaLlUi5pZq2t1LqPNrMfFKCNn12m+\nWw4aduBJM7u1RUPSNxrlfDAJZkdtNALOO/ds3U93hZrxOYNetzbnjILDu5JT1nbI\n4aC60SkdlCw1TxmvXwIDAQAB\n-----END PUBLIC KEY-----\n";
+/* dummy values - the prime was genearted using `openssl dhparam -C` */
+static const unsigned char payload_prime[] = {
+	0x96, 0xC1, 0xFA, 0xA4, 0xD4, 0xAB, 0xD6, 0x6D, 0x6F, 0x99,
+	0x3A, 0xCC, 0xD4, 0x74, 0x71, 0xE9, 0x5B, 0x77, 0xA4, 0x5E,
+	0x68, 0x81, 0x04, 0x83, 0xE4, 0x92, 0x3C, 0xDD, 0x4B, 0xCA,
+	0x16, 0x83, 0xB4, 0x5B, 0x1A, 0x0A, 0x80, 0xA3, 0xFA, 0xCD,
+	0x24, 0xA5, 0xFF, 0x46, 0x49, 0x86, 0x7B, 0x42, 0xDC, 0x39,
+	0x20, 0x2A, 0x6A, 0x86, 0x17, 0x12, 0x95, 0x3F, 0x13, 0xAD,
+	0x5E, 0x98, 0x67, 0xAE, 0xA2, 0xA5, 0x78, 0x9D, 0x6D, 0x7D,
+	0x78, 0xF6, 0xB2, 0x85, 0x7A, 0xA4, 0xB2, 0xC1, 0x96, 0x03,
+	0x09, 0x15, 0x66, 0xBE, 0xFD, 0xA6, 0x63, 0xEA, 0xF6, 0xC4,
+	0x61, 0x1A, 0x78, 0x9E, 0x9C, 0x82, 0x53, 0x8A, 0xCF, 0x07,
+	0x90, 0x89, 0xE5, 0x28, 0x4A, 0x53, 0x77, 0x92, 0x72, 0xCB,
+	0xBD, 0x17, 0x51, 0xE9, 0xC6, 0x34, 0xC4, 0xC9, 0x9A, 0x6A,
+	0xFE, 0x55, 0x58, 0xD6, 0x7D, 0x3F, 0x67, 0xCD, 0xAF, 0x5C,
+	0xCB, 0x46, 0x9B, 0xD1, 0x25, 0x43, 0x80, 0xE0, 0xA6, 0x80,
+	0x1A, 0x15, 0xE6, 0xC6, 0x24, 0xB5, 0x8F, 0xC1, 0xA5, 0xAF,
+	0x23, 0xCD, 0xA9, 0x21, 0x1E, 0x1E, 0xA1, 0x6A, 0xC9, 0xA7,
+	0x17, 0xE9, 0xF5, 0x00, 0x94, 0x84, 0x7B, 0xF2, 0xD8, 0x28,
+	0xE2, 0x8A, 0xC5, 0x58, 0x34, 0xE8, 0xCE, 0xFD, 0x72, 0xA4,
+	0xC7, 0xEB, 0x93, 0x87, 0xC7, 0x54, 0x3D, 0x23, 0x75, 0x77,
+	0x50, 0x73
+};
+static const unsigned char payload_base[] = { 0x02 };
+static const unsigned char payload_private[] = { 42 };