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

Adds support for TLS v1.3 Encrypted Client Hello (ECH) and HPKE (Hybrid Public Key Encryption) #5623

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

dgarske
Copy link
Contributor

@dgarske dgarske commented Sep 22, 2022

Description

Adds support for:

Author: John Bland @jpbland1

Testing

./configure --enable-hpke --enable-smallstack && make check
./configure --enable-hpke && make check

From @jpbland1 :

Code used to test ech client side. it connects to a cloudflare server, sends a grease ech to get the retry configs from the server and then uses those retry configs in the next connection to actually connect with ech. the http that's returned will show sni=encrypted indicating that ech hid the server name.
https://gist.github.com/jpbland1/c3e9018b4088e102be852f596b363a2a

Test used to verify that ech is working. It connects to a cloudflare server that supports ech, sends a grease ech first to retrieve the retry configs and then uses those configs in the second connection to send the real ech. when ech is working you will see sni=encrypted in the resulting http response that gets printed
https://gist.github.com/jpbland1/c3e9018b4088e102be852f596b363a2a

Checklist

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

@dgarske dgarske self-assigned this Sep 22, 2022
@dgarske dgarske requested a review from SparkiDev September 23, 2022 01:37
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Show resolved Hide resolved
wolfssl/wolfcrypt/hpke.h Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
@jpbland1
Copy link
Contributor

I've implemented the changes Sean suggested, please take another look

Copy link
Contributor Author

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also approved, but let's not merge until you work on the encrypted client_hello. I suspect you'll find the API's might need tweaked a bit.

@dgarske dgarske marked this pull request as ready for review September 30, 2022 19:11
@jpbland1
Copy link
Contributor

jpbland1 commented Oct 1, 2022

Yes, I'm reading the ech rfc again and it looks like we will need to use the hpke context multiple times if the client hello has to be retried, not just one shot mode like I thought initially

@jpbland1
Copy link
Contributor

jpbland1 commented Nov 3, 2022

here's the code used to test ech client side. it connects to a cloudflare server, sends a grease ech to get the retry configs from the server and then uses those retry configs in the next connection to actually connect with ech. the http that's returned will show sni=encrypted indicating that ech hid the server name.
https://gist.github.com/jpbland1/c3e9018b4088e102be852f596b363a2a

@jpbland1
Copy link
Contributor

jpbland1 commented Nov 3, 2022

here's the test I use to verify that ech is working. it connects to a cloudflare server that supports ech, sends a grease ech first to retrieve the retry configs and then uses those configs in the second connection to send the real ech. when ech is working you will see sni=encrypted in the resulting http response that gets printed
https://gist.github.com/jpbland1/c3e9018b4088e102be852f596b363a2a

@dgarske
Copy link
Contributor Author

dgarske commented Nov 3, 2022

@jpbland1 this PR has merge conflicts. Please resolve. Thanks.

@jpbland1
Copy link
Contributor

jpbland1 commented Nov 4, 2022

I resolved the merge conflict and fixed an undefined variable error that cropped up from not setting ret = 0 before checking it

@dgarske
Copy link
Contributor Author

dgarske commented Nov 5, 2022

‘’’ 1;38;5;243m--disable-asn --disable-rsa --disable-ecc --enable-psk�[0m
Testing DEFAULT: --disable-asn --disable-rsa --disable-ecc --enable-psk
�[1;32mConfigure RESULT = 0�[0m

make[2]: warning: -j9 forced in submake: resetting jobserver mode.
src/tls13.c:173:22: error: ‘GetMsgHash’ declared ‘static’ but never defined [-Werror=unused-function]
173 | static WC_INLINE int GetMsgHash(WOLFSSL* ssl, byte* hash);
| ^~~~~~~~~~
cc1: all warnings being treated as errors’’’

@jpbland1
Copy link
Contributor

jpbland1 commented Nov 5, 2022

ah, I made that signature because I need to use that static function before it is defined bu forgot to add the conditional compilation macros

@dgarske dgarske requested a review from SparkiDev November 7, 2022 17:27
configure.ac Outdated Show resolved Hide resolved
src/tls.c Outdated Show resolved Hide resolved
src/tls13.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
src/tls13.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/test/test.c Outdated Show resolved Hide resolved
@dgarske dgarske removed their assignment Nov 7, 2022
@dgarske dgarske requested a review from SparkiDev November 18, 2022 17:10
@dgarske dgarske assigned dgarske and unassigned jpbland1 Nov 18, 2022
@dgarske dgarske assigned dgarske and unassigned dgarske Nov 21, 2022
@dgarske dgarske assigned JacobBarthelmeh and unassigned dgarske Nov 21, 2022
Copy link
Contributor Author

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 bit, trailing spaces or tabs:
./src/tls.c:11125:
weird control chars, hard tabs, CRs, trailing whitespace:
./src/tls.c:11125:
warning, overlong lines added:
/src/internal.c:28936                     ssl->arrays->client_identity[MAX_PSK_ID_LEN] = '\0'; /* null term */
/src/ssl.c:347         ret = wc_HpkeGenerateKeyPair(hpke, &ctx->echConfigs->receiverPrivkey, rng);
/src/tls.c:11342             ret = wc_HpkeInit(hpke, ech->kemId, ech->cipherSuite.kdfId, ech->cipherSuite.aeadId, NULL);
/src/tls.c:11353                 ret = wc_HpkeSerializePublicKey(hpke, ephemeralKey, writeBuf_p, &ech->encLen);
/src/tls.c:11363                 ret = wc_RNG_GenerateBlock(rng, writeBuf_p, 160 + ((writeBuf_p - writeBuf) % 32));
/src/tls.c:11375             ret = wc_HpkeSerializePublicKey(ech->hpke, ech->ephemeralKey, writeBuf_p, &ech->encLen);
/src/tls.c:11538 static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, byte msgType)
/src/tls.c:12986 /* because the size of ech depends on the size of other extensions we need to do it special and last */
/src/tls13.c:894     return Tls13DeriveKey(ssl, secret, -1, key, finishedLabel, FINISHED_LABEL_SZ,
/src/tls13.c:4105         XMEMSET(ech->innerClientHello + ech->innerClientHelloLen - ech->hpke->Nt -
/src/tls13.c:4108         /* copy the client hello to the ech innerClientHello, exclude record and handshake headers */
/wolfcrypt/src/hpke.c:345 int wc_HpkeDeserializePublicKey(Hpke* hpke, void** key, const byte* in, word16 inSz)
/wolfcrypt/src/hpke.c:600     /* 1 for mode and WC_MAX_DIGEST_SIZE times 2 for psk_id_hash and info_hash */
/wolfcrypt/src/hpke.c:886 static int wc_HpkeDecap(Hpke* hpke, void* receiverKey, const byte* pubKey, word16 pubKeySz,
/wolfcrypt/src/hpke.c:1063     if (hpke == NULL || receiverKey == NULL || pubKey == NULL || pubKeySz == 0 ||
/wolfcrypt/src/hpke.c:1078     ret = wc_HpkeSetupBaseReceiver(hpke, context, receiverKey, pubKey, pubKeySz, info, infoSz);
/wolfcrypt/src/misc.c:837 #if !defined(WOLFCRYPT_ONLY) && (!defined(NO_SESSION_CACHE) || defined(HAVE_SESSION_TICKET))
/wolfssl/internal.h:3457         CallbackRsaSign   RsaSignCb;      /* User RsaSign Callback handler (priv key) */
/wolfssl/internal.h:3458         CallbackRsaVerify RsaVerifyCb;    /* User RsaVerify Callback handler (pub key) */
/wolfssl/internal.h:3459         CallbackRsaVerify RsaSignCheckCb; /* User VerifyRsaSign Callback handler (priv key) */
/wolfssl/internal.h:3461             CallbackRsaPssSign   RsaPssSignCb;       /* User RsaSign (priv key) */
/wolfssl/internal.h:3462             CallbackRsaPssVerify RsaPssVerifyCb;     /* User RsaVerify (pub key) */
/wolfssl/internal.h:3463             CallbackRsaPssVerify RsaPssSignCheckCb; /* User VerifyRsaSign (priv key) */
/wolfssl/internal.h:3468     CallbackGenPreMaster        GenPreMasterCb;     /* Use generate pre-master handler */
/wolfssl/internal.h:3469     CallbackGenMasterSecret     GenMasterCb;        /* Use generate master secret handler */
/wolfssl/internal.h:3470     CallbackGenSessionKey       GenSessionKeyCb;    /* Use generate session key handler */
/wolfssl/internal.h:3471     CallbackEncryptKeys         EncryptKeysCb;/* Use setting encrypt keys handler */
/wolfssl/internal.h:3472     CallbackTlsFinished         TlsFinishedCb;      /* Use Tls finished handler */
/wolfssl/internal.h:5844 WOLFSSL_LOCAL int InitHandshakeHashesAndCopy(WOLFSSL* ssl, HS_Hashes* source, HS_Hashes** destination);
/wolfssl/ssl.h:3340 WOLFSSL_API void  wolfSSL_CTX_SetDhAgreeCb(WOLFSSL_CTX* ctx, CallbackDhAgree cb);
/wolfssl/wolfcrypt/hpke.h:110 WOLFSSL_API int wc_HpkeSerializePublicKey(Hpke* hpke, void* key, byte* out, word16* outSz);
/wolfssl/wolfcrypt/hpke.h:111 WOLFSSL_API int wc_HpkeDeserializePublicKey(Hpke* hpke, void** key, const byte* in,
/wolfssl/wolfcrypt/hpke.h:114 WOLFSSL_API int wc_HpkeSealBase(Hpke* hpke, void* ephemeralKey, void* receiverKey,
/wolfssl/wolfcrypt/hpke.h:117 WOLFSSL_API int wc_HpkeOpenBase(Hpke* hpke, void* receiverKey, const byte* pubKey,

src/internal.c Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
ECC_SECP521R1);
break;
case DHKEM_X25519_HKDF_SHA256:
*keypair = XMALLOC(sizeof(curve25519_key), hpke->heap,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./configure --enable-hpke --enable-small-stack && make
...
wolfcrypt/src/hpke.c:282:39: error: use of undeclared identifier 'curve25519_key'
            *keypair = XMALLOC(sizeof(curve25519_key), hpke->heap,
                                      ^
wolfcrypt/src/hpke.c:287:21: error: implicit declaration of function 'wc_curve25519_init_ex' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
              ret = wc_curve25519_init_ex((curve25519_key*)*keypair,
                    ^
  CC       src/libwolfssl_la-internal.lo
wolfcrypt/src/hpke.c:287:59: error: expected expression
              ret = wc_curve25519_init_ex((curve25519_key*)*keypair,
                                                          ^
wolfcrypt/src/hpke.c:287:44: error: use of undeclared identifier 'curve25519_key'
              ret = wc_curve25519_init_ex((curve25519_key*)*keypair,
                                           ^
wolfcrypt/src/hpke.c:291:23: error: implicit declaration of function 'wc_curve25519_make_key' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                ret = wc_curve25519_make_key(rng, 32,
                      ^
wolfcrypt/src/hpke.c:291:23: note: did you mean 'wc_curve25519_init_ex'?
wolfcrypt/src/hpke.c:287:21: note: 'wc_curve25519_init_ex' declared here
              ret = wc_curve25519_init_ex((curve25519_key*)*keypair,
                    ^
wolfcrypt/src/hpke.c:292:37: error: expected expression
                    (curve25519_key*)*keypair);
                                    ^
wolfcrypt/src/hpke.c:292:22: error: use of undeclared identifier 'curve25519_key'
                    (curve25519_key*)*keypair);
                     ^
wolfcrypt/src/hpke.c:331:19: error: implicit declaration of function 'wc_curve25519_export_public_ex' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            ret = wc_curve25519_export_public_ex((curve25519_key*)key, out,
                  ^
wolfcrypt/src/hpke.c:331:66: error: expected expression
            ret = wc_curve25519_export_public_ex((curve25519_key*)key, out,
                                                                 ^
wolfcrypt/src/hpke.c:331:51: error: use of undeclared identifier 'curve25519_key'
            ret = wc_curve25519_export_public_ex((curve25519_key*)key, out,
                                                  ^
wolfcrypt/src/hpke.c:332:28: error: use of undeclared identifier 'EC25519_LITTLE_ENDIAN'
                &tmpOutSz, EC25519_LITTLE_ENDIAN);
                           ^
  CC       src/libwolfssl_la-wolfio.lo
wolfcrypt/src/hpke.c:372:35: error: use of undeclared identifier 'curve25519_key'
            *key = XMALLOC(sizeof(curve25519_key), hpke->heap,
                                  ^
wolfcrypt/src/hpke.c:377:21: error: implicit declaration of function 'wc_curve25519_init_ex' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
              ret = wc_curve25519_init_ex((curve25519_key*)*key, hpke->heap,
                    ^
wolfcrypt/src/hpke.c:377:59: error: expected expression
              ret = wc_curve25519_init_ex((curve25519_key*)*key, hpke->heap,
                                                          ^
wolfcrypt/src/hpke.c:377:44: error: use of undeclared identifier 'curve25519_key'
              ret = wc_curve25519_init_ex((curve25519_key*)*key, hpke->heap,
                                           ^
wolfcrypt/src/hpke.c:381:23: error: implicit declaration of function 'wc_curve25519_import_public_ex' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                ret = wc_curve25519_import_public_ex(in, inSz,
                      ^
wolfcrypt/src/hpke.c:382:37: error: expected expression
                    (curve25519_key*)*key, EC25519_LITTLE_ENDIAN);
                                    ^
wolfcrypt/src/hpke.c:382:22: error: use of undeclared identifier 'curve25519_key'
                    (curve25519_key*)*key, EC25519_LITTLE_ENDIAN);
                     ^
wolfcrypt/src/hpke.c:382:44: error: use of undeclared identifier 'EC25519_LITTLE_ENDIAN'
                    (curve25519_key*)*key, EC25519_LITTLE_ENDIAN);
                                           ^

@dgarske dgarske self-assigned this Nov 28, 2022
Copy link
Contributor

@JacobBarthelmeh JacobBarthelmeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning of uninitialized variable:

./configure --enable-all && make

  CC       src/src_libwolfssl_la-tls13.lo
  CC       src/src_libwolfssl_la-ocsp.lo
src/tls.c: In function ‘TLSX_ExtractEch’:
src/tls.c:10680:13: error: ‘infoLen’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         ret = wc_HpkeOpenBase(ech->hpke, echConfig->receiverPrivkey, ech->enc,
         ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ech->encLen, info, infoLen, aad, aadLen, ech->outerClientPayload,
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ech->innerClientHelloLen,
             ~~~~~~~~~~~~~~~~~~~~~~~~~
             ech->innerClientHello + HANDSHAKE_HEADER_SZ);
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:6327: recipe for target 'src/src_libwolfssl_la-tls.lo' failed

Using GCC gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)

Please also go through and add a comment to new functions. No need for extensive comments but having a comment at the top of new functions showing what the expected return value is and potentially a sentence on what it does helps with maintaining and calling them.

src/ssl.c Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
src/ssl.c Show resolved Hide resolved
XMEMSET( out, 0, w );

for (i = 0; i < w && n > 0; i++) {
out[w-(i + 1)] = (byte)n;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a big issue as of now, but curious if big endian systems have been taken into account with these transformations?

hpke->curve_id = ECC_SECP384R1;
hpke->Nsecret = WC_SHA384_DIGEST_SIZE;
hpke->Nh = WC_SHA384_DIGEST_SIZE;
hpke->Ndh = wc_ecc_get_curve_size_from_id( hpke->curve_id );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_ecc_get_curve_size_from_id can return a negative value on failure, we should check that Ndh here and later on in this function is not getting set as an error value

wolfcrypt/src/hpke.c Outdated Show resolved Hide resolved
wolfcrypt/src/hpke.c Show resolved Hide resolved
wolfcrypt/src/hpke.c Show resolved Hide resolved
src/tls.c Outdated Show resolved Hide resolved
@JacobBarthelmeh JacobBarthelmeh removed their assignment Dec 30, 2022
@dgarske dgarske assigned JacobBarthelmeh and unassigned jpbland1 Jan 3, 2023
Copy link
Contributor Author

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I'm going to push fix for these.

src/tls.c Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
}

if (ret == 0)
ret = wc_HpkeInit(hpke, DHKEM_X25519_HKDF_SHA256, HKDF_SHA256,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpbland1 why is this hard coded for X25519? We also support ECC, so wondering how that case gets used? Is there a way to know the desired asymmetric algo at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpbland1 please address this comment. Otherwise the PR looks good and ready to merge by @JacobBarthelmeh .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had it setup the default config that cloudflare was using, I can change it to take the algorithms the user wants but I think my rationale was wanting to make it simple

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should also note that you can actually make an indefinite number of configs but I still think we should keep things simple and just have 1 config and add support for multiple if someone asks for it

…-esni) and HPKE (Hybrid Public Key Encryption) RFC9180.
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

Successfully merging this pull request may close these issues.

4 participants