Fixes for TLS with crypto callbacks by dgarske · Pull Request #7070 · wolfSSL/wolfssl

dgarske · 2023-12-14T18:55:45Z

Description

Fixes for TLS v1.3 with crypto callbacks not offloading KDF / HMAC and DeriveKeyMsg computations.
Fix TLS v1.2 case where SHA-1 could be used uninitialized.
Exclude the SHA1 struct from HS_Hashes when not needed (fixes mix-match of the SHA-1 with NO_OLD_TLS and WOLFSSL_ALLOW_TLS_SHA1)
Fix typo with HMAC determination of update/final in crypto callback.
Fix random.c health test to use devId.
Added support for using devId with one-shot hash functions.
Added more information in the DEBUG_CRYPTOCB.

ZD 17134

Testing

./configure --enable-cryptocb --enable-debug CFLAGS="-DDEBUG_CRYPTOCB"
./configure --enable-cryptocb CFLAGS="-DWOLFSSL_ALLOW_TLS_SHA1"

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

…etDevIdAtIndex(0);`

… KDF HMAC and ECH.

… Fix random.c health test to use devId. Fix FIPS unused "ssl".

…e SHA1 struct from HS_Hashes when not needed. Fixes mix-match of the SHA-1 with `NO_OLD_TLS` and `WOLFSSL_ALLOW_TLS_SHA1`.

dgarske · 2023-12-20T21:35:53Z

Retest this please

…1 on either old TLS or `WOLFSSL_ALLOW_TLS_SHA1`.

douzzer · 2023-12-22T05:07:07Z

+        printf("Crypto CB: %s %s (%d) (%p ctx)\n", GetAlgoTypeStr(info->algo_type),
+            GetCipherTypeStr(info->cipher.type), info->cipher.type, info->cipher.ctx);


line length

douzzer · 2023-12-22T05:08:00Z

    else if (info->algo_type == WC_ALGO_TYPE_HASH) {
-        printf("Crypto CB: %s %s (%d)\n", GetAlgoTypeStr(info->algo_type),
-            GetHashTypeStr(info->hash.type), info->hash.type);
+        printf("Crypto CB: %s %s (%d) (%p ctx) %s\n", GetAlgoTypeStr(info->algo_type),


line length

douzzer · 2023-12-22T05:08:15Z

+        printf("Crypto CB: %s %s (%d) (%p ctx) %s\n", GetAlgoTypeStr(info->algo_type),
+            GetHashTypeStr(info->hmac.macType), info->hmac.macType, info->hmac.hmac,


line length

douzzer · 2023-12-22T05:45:41Z

-            digestType);
+    if (ret == 0) {
+        PRIVATE_KEY_UNLOCK();
+        ret = Tls13HKDFExpandKeyLabel(ssl,


note this is one of the calls to Tls13HKDFExpandKeyLabel() that happens even in FIPS builds. without the fix at line 241 I see this on fips+all builds:

src/tls13.c: In function ‘EchCheckAcceptance’: d71e214bb6 (<david@wolfssl.com> 2023-12-18 17:16:54 -0800 4825) ret = Tls13HKDFExpandKeyLabel(ssl, src/tls13.c:4825:15: error: implicit declaration of function ‘Tls13HKDFExpandKeyLabel’; did you mean ‘Tls13HKDFExpandLabel’? [-Werror=implicit-function-declaration] 4825 | ret = Tls13HKDFExpandKeyLabel(ssl, | ^~~~~~~~~~~~~~~~~~~~~~~ | Tls13HKDFExpandLabel

…pto callback line lengths.

douzzer

The gcc sanitizers, due to multilevel inlining, have managed to figure out that there's something wrong with Tls13DeriveKey() in this PR. It's clean on master so it seems to be new to this PR:

[sp-all-asm-sanitizer] [1 of 1] [59155051bb]
    autogen.sh 59155051bb...   real 0m16.249s  user 0m14.299s  sys 0m1.088s
    configure...   real 0m18.709s  user 0m10.000s  sys 0m10.108s
    build...In function ‘Tls13DeriveKey’,
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 1095)     return Tls13DeriveKey(ssl, secret, -1, key, finishedLabel,
    inlined from ‘DeriveFinishedSecret’ at src/tls13.c:1095:12:
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 502)     ret = Tls13HKDFExpandKeyLabel(ssl, output, outputLen, secret, hashSz,
src/tls13.c:502:11: error: ‘hash’ may be used uninitialized [-Werror=maybe-uninitialized]
  502 |     ret = Tls13HKDFExpandKeyLabel(ssl, output, outputLen, secret, hashSz,
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  503 |                                   protocol, protocolLen, label, labelLen,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  504 |                                   hash, hashOutSz, digestAlg, side);
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/tls13.c: In function ‘DeriveFinishedSecret’:
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 248) static int Tls13HKDFExpandKeyLabel(WOLFSSL* ssl, byte* okm, word32 okmLen,
src/tls13.c:248:12: note: by argument 8 of type ‘const byte *’ {aka ‘const unsigned char *’} to ‘Tls13HKDFExpandKeyLabel.constprop’ declared here
  248 | static int Tls13HKDFExpandKeyLabel(WOLFSSL* ssl, byte* okm, word32 okmLen,
      |            ^~~~~~~~~~~~~~~~~~~~~~~
481f4765eb (<david@wolfssl.com> 2018-01-11 09:52:49 -0800 431)     byte        hash[WC_MAX_DIGEST_SIZE];
src/tls13.c:431:17: note: ‘hash’ declared here
  431 |     byte        hash[WC_MAX_DIGEST_SIZE];
      |                 ^~~~
In function ‘Tls13DeriveKey’,
    inlined from ‘DeriveTrafficSecret’ at src/tls13.c:1116:12,
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 1538)                 ret = DeriveTrafficSecret(ssl, ssl->clientSecret,
    inlined from ‘DeriveTls13Keys’ at src/tls13.c:1538:23:
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 502)     ret = Tls13HKDFExpandKeyLabel(ssl, output, outputLen, secret, hashSz,
src/tls13.c:502:11: error: ‘hash’ may be used uninitialized [-Werror=maybe-uninitialized]
  502 |     ret = Tls13HKDFExpandKeyLabel(ssl, output, outputLen, secret, hashSz,
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  503 |                                   protocol, protocolLen, label, labelLen,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  504 |                                   hash, hashOutSz, digestAlg, side);
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/tls13.c: In function ‘DeriveTls13Keys’:
6190666108 (<anthony@wolfssl.com> 2022-09-21 03:21:33 -0400 248) static int Tls13HKDFExpandKeyLabel(WOLFSSL* ssl, byte* okm, word32 okmLen,
src/tls13.c:248:12: note: by argument 8 of type ‘const byte *’ {aka ‘const unsigned char *’} to ‘Tls13HKDFExpandKeyLabel.constprop’ declared here
  248 | static int Tls13HKDFExpandKeyLabel(WOLFSSL* ssl, byte* okm, word32 okmLen,
      |            ^~~~~~~~~~~~~~~~~~~~~~~
481f4765eb (<david@wolfssl.com> 2018-01-11 09:52:49 -0800 431)     byte        hash[WC_MAX_DIGEST_SIZE];
src/tls13.c:431:17: note: ‘hash’ declared here
  431 |     byte        hash[WC_MAX_DIGEST_SIZE];
      |                 ^~~~

If I just XMEMSET(hash, 0, sizeof hash) at the start of the function, the sanitizing build and make check are clean. But that shouldn't be necessary -- I'm concerned one of the hash functions isn't writing out the hash value correctly.

…nitialized "hash".

douzzer

LGTM. Passing all the extra tests now. The documentation update looks great!

dgarske · 2023-12-27T23:33:09Z

Retest this please

dgarske self-assigned this Dec 14, 2023

dgarske changed the title ~~Add more information to the crypto callback DEBUG_CRYPTOCB~~ Fixes for TLS v1.3 and crypto callbacks Dec 18, 2023

dgarske force-pushed the cryptocb_moreinfo branch from 61200d7 to 4a868bf Compare December 18, 2023 21:48

dgarske mentioned this pull request Dec 18, 2023

Improve TLS crypto callback examples wolfSSL/wolfssl-examples#413

Merged

dgarske added 4 commits December 18, 2023 17:11

Add more information in the DEBUG_CRYPTOCB.

205403e

Fix typo with HMAC determination of update/final.

d5e8331

Add support for using devId with one-shot hash functions.

8b20371

Remove the SHA1-/SHA2-256 auto devId selection `devId = wc_CryptoCb_G…

90748b5

…etDevIdAtIndex(0);`

dgarske force-pushed the cryptocb_moreinfo branch 3 times, most recently from ed65d74 to 1b3afac Compare December 19, 2023 01:35

Fixes for TLS v1.3 with crypto callbacks not offloading DeriveKeyMsg,…

2001d1c

… KDF HMAC and ECH.

dgarske force-pushed the cryptocb_moreinfo branch from 1b3afac to 2001d1c Compare December 19, 2023 16:16

dgarske added 2 commits December 19, 2023 11:20

Fix one shot hash routines to attempt offloading to crypto callbacks.…

fb5eab8

… Fix random.c health test to use devId. Fix FIPS unused "ssl".

Fix TLS v1.2 case where SHA-1 could be used uninitialized. Exclude th…

41d4f4a

…e SHA1 struct from HS_Hashes when not needed. Fixes mix-match of the SHA-1 with `NO_OLD_TLS` and `WOLFSSL_ALLOW_TLS_SHA1`.

dgarske changed the title ~~Fixes for TLS v1.3 and crypto callbacks~~ Fixes for TLS with crypto callbacks Dec 19, 2023

dgarske requested a review from wolfSSL-Bot December 20, 2023 19:52

dgarske assigned wolfSSL-Bot Dec 20, 2023

dgarske removed their assignment Dec 20, 2023

Further cleanup for Hashes.sha when not required. Gate all TLS SHA-…

0d212d8

…1 on either old TLS or `WOLFSSL_ALLOW_TLS_SHA1`.

dgarske assigned dgarske and unassigned dgarske Dec 21, 2023

douzzer requested changes Dec 22, 2023

View reviewed changes

douzzer assigned dgarske and unassigned wolfSSL-Bot Dec 22, 2023

Peer review fixes. Fix issues with Tls13HKDFExpandKeyLabel. Fix cry…

d9ac8b5

…pto callback line lengths.

dgarske requested a review from douzzer December 22, 2023 22:17

dgarske assigned wolfSSL-Bot Dec 22, 2023

douzzer requested changes Dec 23, 2023

View reviewed changes

dgarske added 4 commits December 27, 2023 09:52

Improve the TLS v1.3 expand key label warning for possible use of uni…

b86dfff

…nitialized "hash".

Fix line lengths.

0d05709

Make new HDFK _ex functions public.

cca6cc0

Add documentation for HKDF functions. Improve param comments for devId.

1c4d728

dgarske requested a review from douzzer December 27, 2023 21:57

douzzer approved these changes Dec 27, 2023

View reviewed changes

dgarske removed their assignment Dec 27, 2023

douzzer merged commit 457188f into wolfSSL:master Dec 27, 2023

dgarske mentioned this pull request Jan 11, 2024

Fix for crypto callbacks to allow invalid devId for non _ex single shot hash functions #7127

Merged

4 tasks

This was referenced May 15, 2024

Allow SHA1 with NO_OLD_TLS #4793

Closed

SHA1 client certificate validation fails if NO_OLD_TLS is specified #4791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for TLS with crypto callbacks#7070

Fixes for TLS with crypto callbacks#7070
douzzer merged 13 commits intowolfSSL:masterfrom
dgarske:cryptocb_moreinfo

dgarske commented Dec 14, 2023 •

edited

Loading

Uh oh!

dgarske commented Dec 20, 2023

Uh oh!

douzzer Dec 22, 2023

Uh oh!

douzzer Dec 22, 2023

Uh oh!

douzzer Dec 22, 2023

Uh oh!

douzzer Dec 22, 2023

Uh oh!

douzzer left a comment

Uh oh!

douzzer left a comment

Uh oh!

dgarske commented Dec 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		printf("Crypto CB: %s %s (%d) (%p ctx)\n", GetAlgoTypeStr(info->algo_type),
		GetCipherTypeStr(info->cipher.type), info->cipher.type, info->cipher.ctx);

		printf("Crypto CB: %s %s (%d) (%p ctx) %s\n", GetAlgoTypeStr(info->algo_type),
		GetHashTypeStr(info->hmac.macType), info->hmac.macType, info->hmac.hmac,

Conversation

dgarske commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist

Uh oh!

dgarske commented Dec 20, 2023

Uh oh!

douzzer Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

douzzer Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

douzzer Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

douzzer Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

dgarske commented Dec 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dgarske commented Dec 14, 2023 •

edited

Loading