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

[mbedtls] Upgrade mbedTLS to version 3.6.0 #1832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kailun-qin
Copy link
Contributor

@kailun-qin kailun-qin commented Apr 2, 2024

Description of the changes

This commit introduces the following changes along with the upgrade of mbedTLS to version 3.6.0:

  • updating the sonames of mbedTLS-produced libs;
  • using the uploaded release asset from the release tag instead of the GitHub-generated one to avoid the involvement of Git submodules;
  • updating the subproject name from mbedtls-mbedtls- to mbedtls- in accordance with the above asset change;
  • initializing PSA crypto in ra-tls-mbedtls example and secret-prov libs, required for TLS 1.3 -- enabled by default since this mbedTLS version;
  • patching curl-8.7.1 (mbedTLS backend) to fix a bug of calling mbedtls_ssl_setup() without RNG provided.

This mbedTLS version includes fixes for CVE-2024-28755, CVE-2024-28836 and CVE-2024-28960.

How to test this PR?

CI


This change is Reviewable

Copy link
Contributor Author

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)


-- commits line 8 at r1:
Pls see https://github.com/Mbed-TLS/mbedtls/blob/30978ec6507c0a0376ae041fa333a2f24b0c09b8/README.md#L59 and Mbed-TLS/mbedtls#8993 for details.

Code quote:

  * using the uploaded release asset from the release tag instead of the
    GitHub-generated one to avoid the involvement of Git submodules;

subprojects/mbedtls-3.6.0.wrap line 4 at r1 (raw file):

directory = mbedtls-3.6.0
source_url = https://github.com/Mbed-TLS/mbedtls/releases/download/v3.6.0/mbedtls-3.6.0.tar.bz2
source_fallback_url = https://packages.gramineproject.io/distfiles/mbedtls-3.6.0.tar.bz2

@woju: would you pls help mirror this file? I'll test it afterwards.

Before that, pls take a look if the change of the release asset source url makes sense to you. Thanks!

Code quote:

https://packages.gramineproject.io/distfiles/mbedtls-3.6.0.tar.bz2

@kailun-qin kailun-qin marked this pull request as draft April 2, 2024 10:01
@kailun-qin kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.6.0 branch 2 times, most recently from f848ffe to bf6f1c5 Compare April 3, 2024 03:39
Copy link
Contributor Author

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)


-- commits line 12 at r3:
Pls see https://github.com/Mbed-TLS/mbedtls/releases/tag/v3.6.0 and Mbed-TLS/mbedtls@27eb68d for details.

Code quote:

enabled by default since this mbedTLS version

@kailun-qin kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.6.0 branch from bf6f1c5 to ad3f053 Compare April 3, 2024 05:19
Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)


subprojects/mbedtls-3.6.0.wrap line 4 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

@woju: would you pls help mirror this file? I'll test it afterwards.

Before that, pls take a look if the change of the release asset source url makes sense to you. Thanks!

Done.
Judging by the mbedtls release (https://github.com/Mbed-TLS/mbedtls/releases/tag/v3.6.0), This is the Way.

Did anyone recreate this tarball to see if it's legit? (*cough* xz *cough*).

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)


CI-Examples/ra-tls-mbedtls/src/client.c line 171 at r4 (raw file):

    psa_status_t status = psa_crypto_init();
    if (status != PSA_SUCCESS) {
        mbedtls_fprintf(stderr, "Failed to initialize PSA Crypto implementation: %d\n",

We just use mbedtls_printf("...") in this example code, let's stick with this.


CI-Examples/ra-tls-mbedtls/src/client.c line 174 at r4 (raw file):

                        (int)status);
        ret = 1;
        goto exit;

We are in the initialization phase here, you can just do return 1;


CI-Examples/ra-tls-mbedtls/src/server.c line 109 at r4 (raw file):

    psa_status_t status = psa_crypto_init();
    if (status != PSA_SUCCESS) {
        mbedtls_fprintf(stderr, "Failed to initialize PSA Crypto implementation: %d\n",

ditto


CI-Examples/ra-tls-mbedtls/src/server.c line 112 at r4 (raw file):

                        (int)status);
        ret = 1;
        goto exit;

ditto


subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 44 at r4 (raw file):

#define MBEDTLS_SSL_CLI_C
#define MBEDTLS_SSL_CONTEXT_SERIALIZATION
#define MBEDTLS_SSL_PROTO_TLS1_2

Don't we want to add MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE?

I'm not sure if this is important for the in-PAL mbedtls version though...

@kailun-qin kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.6.0 branch from ad3f053 to 7af9ba0 Compare April 5, 2024 14:09
Copy link
Contributor Author

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 12 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @woju)


CI-Examples/ra-tls-mbedtls/src/client.c line 171 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We just use mbedtls_printf("...") in this example code, let's stick with this.

Done.


CI-Examples/ra-tls-mbedtls/src/client.c line 174 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We are in the initialization phase here, you can just do return 1;

Done.


CI-Examples/ra-tls-mbedtls/src/server.c line 109 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.


CI-Examples/ra-tls-mbedtls/src/server.c line 112 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.


subprojects/mbedtls-3.6.0.wrap line 4 at r1 (raw file):

Did anyone recreate this tarball to see if it's legit? (*cough* xz *cough*).

:)


subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 44 at r4 (raw file):

I'm not sure if this is important for the in-PAL mbedtls version though...

+1, I think we should evaluate the exact impact first

Don't we want to add MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE?

Due to the above reason, I'm not sure whether we should do this in this PR which we consider as a general upgrade.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @woju)

a discussion (no related file):
Why is this PR marked as draft?



subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 44 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

I'm not sure if this is important for the in-PAL mbedtls version though...

+1, I think we should evaluate the exact impact first

Don't we want to add MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE?

Due to the above reason, I'm not sure whether we should do this in this PR which we consider as a general upgrade.

Can you add the two defines as commented out and add a TODO comment to analyze their impact?

@kailun-qin kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.6.0 branch from 7af9ba0 to 23c2ad6 Compare April 8, 2024 02:47
Copy link
Contributor Author

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 14 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @woju)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why is this PR marked as draft?

Because the previous version failed our ra-tls-mbedtls EPID sample (due to some issues when curl is using mbedTLS as the TLS backend). Pls see the update.



-- commits line 15 at r6:
Pls see the check added in mbedtls since v3.6.0 -- Mbed-TLS/mbedtls@b422cab (https://github.com/Mbed-TLS/mbedtls/blob/v3.5.2/library/ssl_tls.c#L1357-L1359, https://github.com/Mbed-TLS/mbedtls/blob/v3.6.0/library/ssl_tls.c#L1379-L1386).

Also see the related code snippet in libcurl: https://github.com/curl/curl/blob/860cd5fc2dc8e165fadd2c19a9b7c73b3ae5069d/lib/vtls/mbedtls.c#L605-L641 for details.

I may upstream this to curl if it makes sense.

Code quote:

  * patching curl-8.7.1 (mbedTLS backend) to fix a bug of calling
    `mbedtls_ssl_setup()` without RNG provided.

subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 44 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can you add the two defines as commented out and add a TODO comment to analyze their impact?

Done.

@kailun-qin kailun-qin marked this pull request as ready for review April 8, 2024 04:19
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @woju)


-- commits line 15 at r6:

Previously, kailun-qin (Kailun Qin) wrote…

Pls see the check added in mbedtls since v3.6.0 -- Mbed-TLS/mbedtls@b422cab (https://github.com/Mbed-TLS/mbedtls/blob/v3.5.2/library/ssl_tls.c#L1357-L1359, https://github.com/Mbed-TLS/mbedtls/blob/v3.6.0/library/ssl_tls.c#L1379-L1386).

Also see the related code snippet in libcurl: https://github.com/curl/curl/blob/860cd5fc2dc8e165fadd2c19a9b7c73b3ae5069d/lib/vtls/mbedtls.c#L605-L641 for details.

I may upstream this to curl if it makes sense.

The patch looks correct to me. Please send it to the curl community. I'd like to hear their opinion before we merge this PR (with this patch).


subprojects/packagefiles/curl-8.7.1/compile.sh line 24 at r6 (raw file):

rm -rf "$PRIVATE_DIR"
cp -ar "$CURRENT_SOURCE_DIR" "$PRIVATE_DIR"
patch -p1 --directory "$PRIVATE_DIR" <"$CURRENT_SOURCE_DIR"/curl-8.7.1.patch

Not blocking, but I prefer a space after <


subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 10 at r6 (raw file):

#pragma once

/* mbedTLS v3.6.0 has below related TLS 1.3 features enabled by default.

This is hard to read. Also, isn't it just a recommendation from the mbedTLS team? Maybe:

mbedTLS v3.6.0 recommends to enable the following TLS 1.3 features:

#define ...

These features are currently *not* enabled in mbedTLS version used for internal Gramine PAL crypto/TLS.
TODO: analyze their impact ...

This commit introduces the following changes along with the upgrade of
mbedTLS to version 3.6.0:
* updating the sonames of mbedTLS-produced libs;
* using the uploaded release asset from the release tag instead of the
  GitHub-generated one to avoid the involvement of Git submodules;
* updating the subproject name from `mbedtls-mbedtls-` to `mbedtls-` in
  accordance with the above asset change;
* initializing PSA crypto in `ra-tls-mbedtls` example and `secret-prov`
  libs, required for TLS 1.3 -- enabled by default since this mbedTLS
  version;
* patching curl-8.7.1 (mbedTLS backend) to fix a bug of calling
  `mbedtls_ssl_setup()` without RNG provided.

This mbedTLS version includes fixes for CVE-2024-28755, CVE-2024-28836
and CVE-2024-28960.

Signed-off-by: Kailun Qin <kailun.qin@intel.com>
@kailun-qin kailun-qin force-pushed the kailun-qin/upgrade-mbedtls-3.6.0 branch from 23c2ad6 to 8945edd Compare April 8, 2024 14:26
Copy link
Contributor Author

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 14 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @woju)


-- commits line 15 at r6:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The patch looks correct to me. Please send it to the curl community. I'd like to hear their opinion before we merge this PR (with this patch).

Done, it's merged: curl/curl@b679efc. Pls see the updated comment.


subprojects/packagefiles/curl-8.7.1/compile.sh line 24 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Not blocking, but I prefer a space after <

Done.


subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 10 at r6 (raw file):

isn't it just a recommendation from the mbedTLS team?

I cannot tell it's recommended. From release note, it's just enabled by default: https://github.com/Mbed-TLS/mbedtls/releases/tag/v3.6.0.

Anyway, updated the wording following the suggestions (just w/o "recommends to").

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @woju)


-- commits line 15 at r6:

Previously, kailun-qin (Kailun Qin) wrote…

Done, it's merged: curl/curl@b679efc. Pls see the updated comment.

Scary how fast it was merged :)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r5, 1 of 3 files at r7.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @woju)


-- commits line 8 at r1:

The development branch and the mbedtls-3.6 long-term support branch of Mbed TLS use a Git submodule (framework). This is not needed to merely compile the library at a release tag. This is not needed to consume a release archive (zip or tar).

This sounds like building from the tags should still work, and only their development branches have submodules? (sounds weird, but that's how I understand it)
Asking because I'd prefer to rely on the repository/GitHub-autogenerated artifacts instead of manually uploaded artifacts, see e.g. the xz backdoor :) Ofc assuming this works on tags / is not too annoying to do.


-- commits line 18 at r7:
Bugfixes for these vulns don't seem urgent, maybe we could wait for the next Curl release, so we wouldn't need to ship that additional patching of Curl?


subprojects/packagefiles/mbedtls/include/mbedtls/config-pal.h line 10 at r7 (raw file):

#pragma once

/* mbedTLS v3.6.0 enables the following TLS 1.3 features:

Otherwise it seems to contradict the next sentence.

Suggestion:

mbedTLS v3.6.0 by default enables the following TLS 1.3 features:

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)


-- commits line 8 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

The development branch and the mbedtls-3.6 long-term support branch of Mbed TLS use a Git submodule (framework). This is not needed to merely compile the library at a release tag. This is not needed to consume a release archive (zip or tar).

This sounds like building from the tags should still work, and only their development branches have submodules? (sounds weird, but that's how I understand it)
Asking because I'd prefer to rely on the repository/GitHub-autogenerated artifacts instead of manually uploaded artifacts, see e.g. the xz backdoor :) Ofc assuming this works on tags / is not too annoying to do.

No, mbedtls-3.6 is not development branch, it's for 3.6.1, 3.6.2 etc. versions. The tags are against commits on this mbedtls-3.6 branch, and autogenerated tarballs miss the abovementioned "framework" module:

@@ -134,10 +108,12 @@
 mbedtls-3.6.0/doxygen/input/doc_tcpip.h
 mbedtls-3.6.0/doxygen/input/doc_x509.h
 mbedtls-3.6.0/doxygen/mbedtls.doxyfile
-mbedtls-3.6.0/framework/
+mbedtls-3.6.0/framework/CMakeLists.txt
+mbedtls-3.6.0/framework/exported.make
+mbedtls-3.6.0/framework/.gitignore
+mbedtls-3.6.0/framework/LICENSE
+mbedtls-3.6.0/framework/README.md
 mbedtls-3.6.0/.gitattributes
-mbedtls-3.6.0/.github/
-mbedtls-3.6.0/.github/ISSUE_TEMPLATE/
 mbedtls-3.6.0/.github/ISSUE_TEMPLATE/bug_report.md
 mbedtls-3.6.0/.github/ISSUE_TEMPLATE/config.yml
 mbedtls-3.6.0/.github/ISSUE_TEMPLATE/feature_request.md

So you can't build from that tarball, you can't even git submodule update the tree from the update tarball (because that requires working git repo). It's unusable.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @woju)


-- commits line 18 at r7:

Previously, mkow (Michał Kowalczyk) wrote…

Bugfixes for these vulns don't seem urgent, maybe we could wait for the next Curl release, so we wouldn't need to ship that additional patching of Curl?

@kailun-qin: I just merged your PR updating curl, can you update this PR accordingly and drop the patch?

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.

None yet

4 participants