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

Add pre-TLS13 libcrypto PRF implementation #4020

Merged
merged 8 commits into from
Jun 6, 2023
Merged

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented May 24, 2023

Description of changes:

s2n-tls currently uses a custom PRF implementation to generate secret data for the TLS connection. This PR adds the AWSLC/BoringSSL libcrypto implementation, and uses this when s2n-tls is operating in FIPS mode.

Call-outs:

  • OpenSSL also provides a TLS PRF implementation via the EVP_PKEY_CTX APIs, separate from the AWSLC/BoringSSL API. However, the TLS PRF was added in OpenSSL 1.1.0. Since the only FIPS-compatible OpenSSL version is 1.0.2, I didn't implement the OpenSSL version.
  • The libcrypto TLS 1.3 PRF implementation (HKDF) will be added in a future PR.

Testing:

  • Added new unit tests to ensure the correct PRF implementation is being used.
  • The existing unit tests in s2n_tls_prf_test ensure the libcrypto implementation is correct. The s2n_prf function is provided 3 seeds in the case of calculating a hybrid PQ master secret, and this is tested in s2n_tls_hybrid_prf_test.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Squashed commit of the following:

commit 1678bff
Author: Sam Clark <[email protected]>
Date:   Wed May 24 13:21:23 2023 -0400

    cleanup

commit ebea361
Author: Sam Clark <[email protected]>
Date:   Wed May 24 13:12:19 2023 -0400

    clang-format

commit e12b4ac
Author: Sam Clark <[email protected]>
Date:   Wed May 24 13:09:12 2023 -0400

    move s2n_libcrypto_supports_tls_prf to s2n_prf

commit e9ae9ae
Author: Sam Clark <[email protected]>
Date:   Wed May 24 13:01:43 2023 -0400

    cleanup dynamic seed_b allocation

commit 2e8ecc0
Author: Sam Clark <[email protected]>
Date:   Tue May 23 16:32:12 2023 -0400

    remove openssl implementation

commit 21b77d2
Author: Sam Clark <[email protected]>
Date:   Tue May 23 11:45:20 2023 -0400

    gate libcrypto prf with > OpenSSL 1.0.2

commit ab20222
Author: Sam Clark <[email protected]>
Date:   Tue May 23 11:04:10 2023 -0400

    discard const

commit 287ecc1
Author: Sam Clark <[email protected]>
Date:   Wed May 17 19:01:56 2023 -0400

    Add pre-TLS13 libcrypto PRF implementation

commit 9711902
Author: Sam Clark <[email protected]>
Date:   Wed May 17 16:35:41 2023 -0400

    wip
@goatgoose goatgoose marked this pull request as ready for review May 24, 2023 18:53
@goatgoose goatgoose requested review from lrstewart and maddeleine May 24, 2023 19:00
tls/s2n_prf.h Outdated Show resolved Hide resolved
tls/s2n_prf.c Outdated Show resolved Hide resolved
tls/s2n_prf.c Outdated
Comment on lines 496 to 498
/* BoringSSL and AWSLC define the CRYPTO_tls1_prf API in a private header. This function is
* forward-declared to make it accessible.
*/
Copy link
Contributor

@lrstewart lrstewart May 26, 2023

Choose a reason for hiding this comment

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

Do you have any concerns about visibility / this method not being stable? For example, this would be a very bad idea to do with any s2n-tls method not officially in the api.

If this is safe, we need to explain why in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to explain this. I also removed the feature probe. After testing the probe by rebuilding AWSLC with modified API definitions, the try_compile was not sufficient to catch the changes and disable the API. If the modification just hid the symbol, the try_compile worked, but if an argument was added or something the try_compile failed to catch it. Since BoringSSL could theoretically change this API definition, I gated it behind just AWSLC rather than a feature probe.

Copy link
Contributor

@lrstewart lrstewart Jun 5, 2023

Choose a reason for hiding this comment

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

And just to confirm, since we're not using a try_compile, the method is available and exported in all versions of awslc? Even the oldest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - CRYPTO_tls1_prf was made visible by aws/aws-lc@37c6eb4 in 2018. The first AWSLC release was v0.1-alpha, released in 2020. The API is exported in this version here: https://github.com/aws/aws-lc/blob/v0.1-alpha/third_party/boringssl/crypto/fipsmodule/tls/internal.h#L27-L33

tls/s2n_prf.c Outdated Show resolved Hide resolved
tests/unit/s2n_tls_prf_test.c Outdated Show resolved Hide resolved
tls/s2n_prf.c Show resolved Hide resolved
tls/s2n_prf.c Outdated Show resolved Hide resolved
tls/s2n_prf.c Outdated Show resolved Hide resolved
@goatgoose goatgoose force-pushed the fips-prf branch 3 times, most recently from a136896 to 46237a4 Compare June 2, 2023 14:43
@goatgoose goatgoose enabled auto-merge (squash) June 6, 2023 14:45
@goatgoose goatgoose merged commit 56a897c into aws:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants