regression 4005: Add GCM counter overflow test vectors #726

omasse-linaro · 2024-01-30T15:46:23Z

By default optee uses the OP-TEE internal AES-GCM implementation in core/crypto.mk
(CFG_CRYPTO_AES_GCM_FROM_CRYPTOLIB ?= n)

This implementation failed with the Counter Wrap test vector from google/wycheproof

This PR is adding all counter wrap test vectors to xtest regression 4005.

jforissier · 2024-01-31T10:39:14Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

@@ -0,0 +1,360 @@
+{ TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES,
+/* Key */ (const uint8_t []){0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, }, 16.0,


Missing space after {

Why 16.0? The type of this field (struct xt_ae_case::key_len) is size_t.

etienne-lms

Could you add a description line below header line in the commit message?

etienne-lms · 2024-01-31T12:46:34Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

@@ -0,0 +1,360 @@
+{ TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES,


Could you add license and copyright notice inline comments with SPDX-License-Identifier tag?

For header files, prefer /* ... */ inline comment format:

/* SPDX-License-Identifier: Apache-2.0 */ /* * Copyright 2024 NXP */

omasse-linaro · 2024-01-31T14:09:15Z

let's check again when OP-TEE/optee_os#6661 is merged

etienne-lms · 2024-02-01T08:56:25Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

@@ -0,0 +1,360 @@
+{ TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES,


For header files, prefer /* ... */ inline comment format:

/* SPDX-License-Identifier: Apache-2.0 */ /* * Copyright 2024 NXP */

etienne-lms · 2024-02-01T09:18:26Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

+/* PT  */ (const uint8_t []){ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, 40,
+/* CT  */ (const uint8_t []){ 0x00, 0x07, 0x8d, 0x10, 0x9d, 0x92, 0x14, 0x3f, 0xcd, 0x5d, 0xf5, 0x67, 0x21, 0xb8, 0x84, 0xfa, 0xc6, 0x4a, 0xc7, 0x76, 0x2c, 0xc0, 0x9e, 0xea, 0x2a, 0x3c, 0x68, 0xe9, 0x2a, 0x17, 0xbd, 0xb5, 0x75, 0xf8, 0x7b, 0xda, 0x18, 0xbe, 0x56, 0x4e, }, 40,
+/* Tag */ (const uint8_t []){ 0x15, 0x2a, 0x65, 0x04, 0x5f, 0xe6, 0x74, 0xf9, 0x76, 0x27, 0x42, 0x7a, 0xf5, 0xbe, 0x22, 0xda, }, 16,
+77},


Could you also add a space char before close braces. I think it would be better to have clear new-line chars between sub case data blocks, like:

{ TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES, <all data> }, { TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES, <all data> }, ...

Sorry for the noise but since you use a nice script to produce these arrays I think it could help new readers and maintenance to have explicit struct xtest_ae_case field names:

{ .algo = TEE_ALG_AES_GCM, .mode = TEE_MODE_DECRYPT, .key_type = TEE_TYPE_AES, .key = (const uint8_t []){ ... }, .key_len = <value>, .nonce = (const uint8_t []){ ... }, .nonce_len = <value>, .aad_incr = <value>, .aad = (const uint8_t []){ ... }, .aad_len = <value>, .in_incr = <value>, .ptx = (const uint8_t []){ ... }, .ptx_len = <value>, .ctx = (const uint8_t []){ ... }, ctx_len = <value>, .tag = (const uint8_t []){ ... }, .tag_len = <value>, },

Feel free to ignore this comment if you think it's not worth it.

it worth it 👍

etienne-lms · 2024-02-01T11:01:05Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

+	.ctx_len = 40,
+	.tag = (const uint8_t []){ 0x15, 0x2a, 0x65, 0x04, 0x5f, 0xe6, 0x74, 0xf9, 0x76, 0x27, 0x42, 0x7a, 0xf5, 0xbe, 0x22, 0xda, },
+	.tag_len = 16,
+	.line = 77


What does this line number refers to?
Looking at the vectors generated from NIST GCM vectors, the line number refers to the line in the deflated .rsp file. Here it corresponds to the vector files you process but these files are not reacheable here.

it corresponds to the test id:
https://github.com/google/wycheproof/blob/master/testvectors_v1/aes_gcm_test.json#L1141

to find easily which test vector pass or not.

I think it's worth mentioning that in the commit message that the line number refers the "tcId" in the reference Json file.

Sorry for the noise but since you use a nice script to produce these arrays [...]

Should that script be committed too? It would help in case one wants to update/extend the data.

Should that script be committed too? It would help in case one wants to update/extend the data.

yes, it could be added to the scripts folder, but it currently only support AES GCM test vectors from google/wycheproof.

Should that script be committed too? It would help in case one wants to update/extend the data.

yes, it could be added to the scripts folder, but it currently only support AES GCM test vectors from google/wycheproof.

That's OK, you can mention it in the script as a comment.

it corresponds to the test id: https://github.com/google/wycheproof/blob/master/testvectors_v1/aes_gcm_test.json#L1141

to find easily which test vector pass or not.

I find this a bit confusing. We're quite used to the line number being the line number in the C code. Perhaps we should have another field (.extra_id? Edit or simply .id) which would be reported in addition to the line number when non-zero for instance?

o regression_4005.43 AE case 42 algo 0x40000810 line 2609 regression_4005.43 OK o regression_4005.44 AE case 43 algo 0x40000810 line 2609 regression_4005.44 OK o regression_4005.45 AE case 44 algo 0x40000810 line 22 extra_id 77 regression_4005.45 OK o regression_4005.46 AE case 45 algo 0x40000810 line 40 extra_id 78 regression_4005.46 OK

something like that 86b73e5 ?

etienne-lms · 2024-02-01T15:28:58Z

host/xtest/aes_gcm_counter_overflow_test_decrypt.h

@@ -19,7 +19,8 @@
 	.ctx_len = 40,
 	.tag = (const uint8_t []){ 0x15, 0x2a, 0x65, 0x04, 0x5f, 0xe6, 0x74, 0xf9, 0x76, 0x27, 0x42, 0x7a, 0xf5, 0xbe, 0x22, 0xda, },
 	.tag_len = 16,
-	.line = 77
+	.line = 6,


Having the line number related to this generated header is nice. Thanks.

jforissier · 2024-02-01T15:36:24Z

scripts/aes_gcm_test.py

+    outf.write('\t' + to_compound_str('.ptx', tv['msg']) + '\n')
+    outf.write('\t' + to_compound_str('.ctx', tv['ct']) + '\n')
+    outf.write('\t' + to_compound_str('.tag', tv['tag']) + '\n')
+    outf.write('\t.line = ' + repr(line_num) + ',\n')


I suppose '\t.line = __LINE__\n' would work equally well (and perhaps be even more trustworthy 😉) and would avoid the extra complexity of dealing with the line_num argument.

smarter indeed 👍
nitpicking, but line number will point on .line where it was pointing on beginning of the test case.

jforissier · 2024-02-01T15:37:57Z

host/xtest/regression_4000.c

-				     (int)n, (unsigned int)ae_cases[n].algo,
-				     (int)ae_cases[n].line);
+		if (ae_cases[n].id)
+			Do_ADBG_BeginSubCase(c, "AE case %d algo 0x%x line %d extra_id %d",


s/extra_id/id/

jforissier

Reviewed-by: Jerome Forissier <[email protected]>

I think everything should be folded into the first commit, not much point in adding the .id afterwards since it is there because of the new vectors.

omasse-linaro · 2024-02-01T16:15:47Z

I think everything should be folded into the first commit, not much point in adding the .id afterwards since it is there because of the new vectors.

yes, let's squash in a single commit

jforissier · 2024-02-01T16:26:20Z

regression 4005: Add GCM counter overflow test vectors

Add all counter wrap test vectors from [goolge/wycheproof]

s/goolge/google/

(https://github.com/google/wycheproof/blob/master/testvectors_v1/aes_gcm_test.json) repo

Preferably move that below to a Link: tag, as it makes the text more compact.
Link: https://github.com/google/wycheproof/blob/master/testvectors_v1/aes_gcm_test.json [google/wycheproof]

to the xtest regression 4005 test suite.

To create a link between xtest and wycheproof test cases,
line number of the first one match the tcId of the second one.

Not true anymore 😉

Signed-off-by: Olivier Masse <[email protected]>

Please also add the Review-by/Acked-by tags as they are posted. Thanks!

omasse-linaro · 2024-02-01T16:37:22Z

commit message updated and tags applied.

etienne-lms · 2024-02-01T18:03:13Z

The python scripts has formatting issues:

$ pycodestyle scripts/aes_gcm_test.py
scripts/aes_gcm_test.py:6:1: E302 expected 2 blank lines, found 1
scripts/aes_gcm_test.py:21:80: E501 line too long (97 > 79 characters)
scripts/aes_gcm_test.py:33:1: E302 expected 2 blank lines, found 1
scripts/aes_gcm_test.py:52:1: E302 expected 2 blank lines, found 1
scripts/aes_gcm_test.py:53:1: E101 indentation contains mixed spaces and tabs
scripts/aes_gcm_test.py:53:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:54:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:55:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:57:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:57:59: E703 statement ends with a semicolon
scripts/aes_gcm_test.py:58:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:58:20: E703 statement ends with a semicolon
scripts/aes_gcm_test.py:59:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:59:39: E703 statement ends with a semicolon
scripts/aes_gcm_test.py:60:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:60:23: E703 statement ends with a semicolon
scripts/aes_gcm_test.py:62:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:63:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:64:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:65:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:67:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:69:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:70:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:71:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:72:1: W191 indentation contains tabs
scripts/aes_gcm_test.py:74:1: E305 expected 2 blank lines after class or function definition, found 1
scripts/aes_gcm_test.py:75:1: W191 indentation contains tabs

Acked-by: Etienne Carriere <[email protected]> once addressed.

Add all counter wrap test vectors from google/wycheproof repo [1] to the xtest regression 4005 test suite. To create a link between xtest and wycheproof test cases, tcId is printed as an extra information in the result log. Link: [1] https://github.com/google/wycheproof/blob/master/testvectors_v1/aes_gcm_test.json Signed-off-by: Olivier Masse <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]>

omasse-linaro · 2024-02-02T08:55:29Z

Comments addressed and tags applied.

omasse-linaro mentioned this pull request Jan 30, 2024

OP-TEE internal AES GCM: Counter Wrap test vector failed OP-TEE/optee_os#6659

Closed

jforissier reviewed Jan 31, 2024

View reviewed changes

etienne-lms reviewed Jan 31, 2024

View reviewed changes

omasse-linaro force-pushed the aes_gcm_overflow branch from fbef404 to 958990b Compare January 31, 2024 13:25

etienne-lms reviewed Feb 1, 2024

View reviewed changes

omasse-linaro force-pushed the aes_gcm_overflow branch from 36d9c3d to 623870b Compare February 1, 2024 09:48

etienne-lms reviewed Feb 1, 2024

View reviewed changes

omasse-linaro force-pushed the aes_gcm_overflow branch from 623870b to 10ac999 Compare February 1, 2024 14:01

etienne-lms reviewed Feb 1, 2024

View reviewed changes

jforissier reviewed Feb 1, 2024

View reviewed changes

omasse-linaro force-pushed the aes_gcm_overflow branch from 5a8b331 to fbc1b3a Compare February 1, 2024 16:15

omasse-linaro force-pushed the aes_gcm_overflow branch from fbc1b3a to be103ee Compare February 1, 2024 16:36

omasse-linaro force-pushed the aes_gcm_overflow branch from be103ee to bf246a4 Compare February 2, 2024 08:21

jforissier merged commit 9d56621 into OP-TEE:master Feb 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regression 4005: Add GCM counter overflow test vectors #726

regression 4005: Add GCM counter overflow test vectors #726

omasse-linaro commented Jan 30, 2024

jforissier Jan 31, 2024

omasse-linaro Jan 31, 2024

etienne-lms left a comment

etienne-lms Jan 31, 2024 •

edited

Loading

omasse-linaro Jan 31, 2024

etienne-lms Feb 1, 2024

omasse-linaro commented Jan 31, 2024

etienne-lms Feb 1, 2024

etienne-lms Feb 1, 2024

omasse-linaro Feb 1, 2024

etienne-lms Feb 1, 2024

omasse-linaro Feb 1, 2024

etienne-lms Feb 1, 2024 •

edited

Loading

jforissier Feb 1, 2024

omasse-linaro Feb 1, 2024 •

edited

Loading

jforissier Feb 1, 2024

omasse-linaro Feb 1, 2024

jforissier Feb 1, 2024 •

edited

Loading

omasse-linaro Feb 1, 2024

etienne-lms Feb 1, 2024

omasse-linaro Feb 1, 2024

jforissier Feb 1, 2024

omasse-linaro Feb 1, 2024 •

edited

Loading

jforissier Feb 1, 2024

jforissier left a comment

omasse-linaro commented Feb 1, 2024

jforissier commented Feb 1, 2024

omasse-linaro commented Feb 1, 2024 •

edited

Loading

etienne-lms commented Feb 1, 2024 •

edited

Loading

omasse-linaro commented Feb 2, 2024

		@@ -0,0 +1,360 @@
		{ TEE_ALG_AES_GCM, TEE_MODE_DECRYPT, TEE_TYPE_AES,
		/* Key */ (const uint8_t []){0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, }, 16.0,

regression 4005: Add GCM counter overflow test vectors #726

regression 4005: Add GCM counter overflow test vectors #726

Conversation

omasse-linaro commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omasse-linaro commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omasse-linaro Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omasse-linaro Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

omasse-linaro commented Feb 1, 2024

jforissier commented Feb 1, 2024

omasse-linaro commented Feb 1, 2024 • edited Loading

etienne-lms commented Feb 1, 2024 • edited Loading

omasse-linaro commented Feb 2, 2024

etienne-lms Jan 31, 2024 •

edited

Loading

etienne-lms Feb 1, 2024 •

edited

Loading

omasse-linaro Feb 1, 2024 •

edited

Loading

jforissier Feb 1, 2024 •

edited

Loading

omasse-linaro Feb 1, 2024 •

edited

Loading

omasse-linaro commented Feb 1, 2024 •

edited

Loading

etienne-lms commented Feb 1, 2024 •

edited

Loading