Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
omasse-linaro opened this issue Jan 30, 2024 · 5 comments
Closed

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

omasse-linaro opened this issue Jan 30, 2024 · 5 comments

Comments

@omasse-linaro
Copy link
Contributor

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 following test vector from google/wycheproof
AES GCM test id 79

which could be added to xtest ae_cases with:

{ TEE_ALG_AES_GCM, TEE_MODE_ENCRYPT, 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,
/* IV  */ (const uint8_t []){0x1a, 0x55, 0x2e, 0x67, 0xcd, 0xc4, 0xdc, 0x1a, 0x33, 0xb8, 0x24, 0x87, 0x4e, 0xbf, 0x0b, 0xed, }, 16.0,
0,
/* AAD */ NULL, 0,
0,
/* 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.0,
/* CT  */ (const uint8_t []){0x94, 0x8c, 0xa3, 0x7a, 0x8e, 0x66, 0x49, 0xe8, 0x8a, 0xef, 0xfb, 0x1c, 0x59, 0x8f, 0x36, 0x07, 0x00, 0x77, 0x02, 0x41, 0x7e, 0xa0, 0xe0, 0xbc, 0x3c, 0x60, 0xad, 0x5a, 0x94, 0x98, 0x86, 0xde, 0x96, 0x8c, 0xf5, 0x3e, 0xa6, 0x46, 0x2a, 0xed, }, 40.0,
/* Tag */ (const uint8_t []){0x99, 0xb3, 0x81, 0xbf, 0xa2, 0xaf, 0x97, 0x51, 0xc3, 0x9d, 0x1b, 0x6e, 0x86, 0xd1, 0xbe, 0x6a, }, 16.0,
79},

o regression_4005.1 AE case 0 algo 0x40000810 line 79
regression_4000.c:2766: out_tag has an unexpected content:
Got
  B2:F7:6A:3D 12:8D:74:92 32:44:20:77 BC:CD:53:E2  ..j=..t.2D w..S.
Expected
  99:B3:81:BF A2:AF:97:51 C3:9D:1B:6E 86:D1:BE:6A  .......Q...n...j
regression_4000.c:2772: out has an unexpected content:
Got
  FD:E4:FB:AE 4A:09:E0:20 EF:F7:22:96 9F:83:83:2B  ....J.. .."....+
  84:D4:C9:C0 8B:4F:48:28 61:E3:A9:C6 C3:5B:C4:D9  .....OH(a....[..
  1D:F9:27:37 45:13:BF:D4                          ..'7E...
Expected
  94:8C:A3:7A 8E:66:49:E8 8A:EF:FB:1C 59:8F:36:07  ...z.fI.....Y.6.
  00:77:02:41 7E:A0:E0:BC 3C:60:AD:5A 94:98:86:DE  .w.A~...<`.Z....
  96:8C:F5:3E A6:46:2A:ED                          ...>.F*.

Test vector pass successfully when using libtomcrypt aes gcm implementation.

@omasse-linaro
Copy link
Contributor Author

xtest PR available here : OP-TEE/optee_test#726

@jenswi-linaro
Copy link
Contributor

Thanks for reporting and the xtest PR, I'm looking at the problem.

@jenswi-linaro
Copy link
Contributor

I've found the issue and am working on a fix now.

jenswi-linaro added a commit to jenswi-linaro/optee_os that referenced this issue Jan 31, 2024
We have several AES-GCM implementations in crypto libraries and
internal. The internal implementation comes in two flavours, with Arm
crypto extensions (CFG_CRYPTO_WITH_CE=y) and a pure software
implementation.

Each block to be encrypted is xored with an encrypted counter block of
equal size (16 bytes). For each block the counter is increased.

Prior to this patch the entire counter block was increased as a 128-bit
integer, but that's not how AES-GCM is defined. In AES-GCM only the last
32 bits of the counter block are increased, leaving the rest untouched.
The difference is only noticeable when the 32 bits has reached
0xffffffff and wraps to 0x00000000 on next increment. With a 128-bit
integer this would propagate into other parts of the block.

Fix this by only incrementing the last 32-bit word in the counter block,
both in the pure software implementation and when using Arm crypto
extensions.

Link: OP-TEE#6659
Fixes: 1fca7e2 ("core: crypto: add new AES-GCM implementation")
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
Copy link
Contributor

Fix available here #6661

jenswi-linaro added a commit to jenswi-linaro/optee_os that referenced this issue Jan 31, 2024
We have several AES-GCM implementations in crypto libraries and
internal. The internal implementation comes in two flavours, with Arm
crypto extensions (CFG_CRYPTO_WITH_CE=y) and a pure software
implementation.

Each block to be encrypted is xored with an encrypted counter block of
equal size (16 bytes). For each block the counter is increased.

Prior to this patch the entire counter block was increased as a 128-bit
integer, but that's not how AES-GCM is defined. In AES-GCM only the last
32 bits of the counter block are increased, leaving the rest untouched.
The difference is only noticeable when the 32 bits has reached
0xffffffff and wraps to 0x00000000 on next increment. With a 128-bit
integer this would propagate into other parts of the block.

Fix this by only incrementing the last 32-bit word in the counter block,
both in the pure software implementation and when using Arm crypto
extensions.

Link: OP-TEE#6659
Fixes: 1fca7e2 ("core: crypto: add new AES-GCM implementation")
Signed-off-by: Jens Wiklander <[email protected]>
@omasse-linaro
Copy link
Contributor Author

omasse-linaro commented Jan 31, 2024

thanks, all counter wrap test cases are successful now.

jenswi-linaro added a commit to jenswi-linaro/optee_os that referenced this issue Feb 1, 2024
We have several AES-GCM implementations in crypto libraries and
internal. The internal implementation comes in two flavours, with Arm
crypto extensions (CFG_CRYPTO_WITH_CE=y) and a pure software
implementation.

Each block to be encrypted is xored with an encrypted counter block of
equal size (16 bytes). For each block the counter is increased.

Prior to this patch the entire counter block was increased as a 128-bit
integer, but that's not how AES-GCM is defined. In AES-GCM only the
least significant 32 bits of the counter block are increased, leaving
the rest untouched.  The difference is only noticeable when the 32 bits
has reached 0xffffffff and wraps to 0x00000000 on next increment. With a
128-bit integer this would propagate into other parts of the block.

Fix this by only incrementing the last 32-bit word in the counter block,
both in the pure software implementation and when using Arm crypto
extensions.

Link: OP-TEE#6659
Fixes: 1fca7e2 ("core: crypto: add new AES-GCM implementation")
Signed-off-by: Jens Wiklander <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
jforissier pushed a commit that referenced this issue Feb 1, 2024
We have several AES-GCM implementations in crypto libraries and
internal. The internal implementation comes in two flavours, with Arm
crypto extensions (CFG_CRYPTO_WITH_CE=y) and a pure software
implementation.

Each block to be encrypted is xored with an encrypted counter block of
equal size (16 bytes). For each block the counter is increased.

Prior to this patch the entire counter block was increased as a 128-bit
integer, but that's not how AES-GCM is defined. In AES-GCM only the
least significant 32 bits of the counter block are increased, leaving
the rest untouched.  The difference is only noticeable when the 32 bits
has reached 0xffffffff and wraps to 0x00000000 on next increment. With a
128-bit integer this would propagate into other parts of the block.

Fix this by only incrementing the last 32-bit word in the counter block,
both in the pure software implementation and when using Arm crypto
extensions.

Link: #6659
Fixes: 1fca7e2 ("core: crypto: add new AES-GCM implementation")
Signed-off-by: Jens Wiklander <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants