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

Update mbedtls to final 2.x release #42311

Conversation

jeremiahpslewis
Copy link
Contributor

@jeremiahpslewis jeremiahpslewis commented Sep 19, 2021

Potential security issues are present in earlier versions...

Link: https://tls.mbed.org/security

See here as well: JuliaPackaging/Yggdrasil#4179

@KristofferC
Copy link
Member

There seems to be a bunch of irrelevant commits on this branch. Perhaps rebasing on master would get rid of them.

@jeremiahpslewis
Copy link
Contributor Author

There seems to be a bunch of irrelevant commits on this branch. Perhaps rebasing on master would get rid of them.

Yep, thanks for the nudge. Fixed.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Looks like you need to submit a PR to build binaries for this (https://github.com/JuliaPackaging/Yggdrasil/tree/master/M/MbedTLS)

Then run make -f ./contrib/refresh_checksums.mk mbedtls in your Julia tree.

@vtjnash vtjnash added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Oct 13, 2021
@jeremiahpslewis
Copy link
Contributor Author

Yggdrasil is done. JuliaRegistries/General#46907

@jeremiahpslewis
Copy link
Contributor Author

I reran the hash update script, but there were no changes, think I already had this covered in an earlier commit (the second commit in the PR).

@vtjnash Think both points raised are ✅

@vtjnash vtjnash force-pushed the jeremiahpslewis/mbedtls branch from e6ab8a6 to 223ebd5 Compare October 18, 2021 16:01
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 18, 2021
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I updated the stdlib version number and reran the script to download the Yggdrasil files. All version numbers now look consistent to me.

@jeremiahpslewis
Copy link
Contributor Author

Nice, thanks! Didn't notice the issue with the makefile, good catch.

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

Apparently issues with mbedcrypto, needing updating simultaneously?

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 22, 2021
@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 22, 2021

Removing the merge me label, as the CI failures are related to the contents of this PR.

@jeremiahpslewis
Copy link
Contributor Author

Mbedcrypto is provided as part of mbedtls, so I don't think that is the issue, I suspect there's a problem with libgit2 linking to mbedtls/mbedcrypto binaries, but can't place the error more specifically.

@carlocab
Copy link
Contributor

FYI, 2.27 has fixes for two high-severity security advisories: https://tls.mbed.org/tech-updates/security-advisories/mbedtls-security-advisory-2021-07-1, https://tls.mbed.org/tech-updates/security-advisories/mbedtls-security-advisory-2021-07-2

I don't know if these vulnerabilities are exposed through Julia.

@mkitti
Copy link
Contributor

mkitti commented Jan 6, 2022

The issue is that we have Mbed TLS pinned to version 2.24 in three places in BinaryBuilder

  1. LibSSH2
  2. LibGit2
  3. Curl

Updating Mbed TLS to 2.27 changes the SO number from 5 to 7. That's why libmbedcrypto.so.5 is missing.

Compiling julia with this patch merged onto master and USE_BINARYBUILDER_LIBGIT2=0 USE_BINARYBUILDER_LIBSSH2=0 USE_BINARYBUILDER_LIBCURL=0 works.

Also there is now Mbed TLS 2.28 due CVE-2021-44732

@mkitti
Copy link
Contributor

mkitti commented Jan 7, 2022

@jeremiahpslewis Would you be interested in updating this PR to Mbed TLS 2.28 ?

@jeremiahpslewis jeremiahpslewis force-pushed the jeremiahpslewis/mbedtls branch from 223ebd5 to db5fabf Compare January 7, 2022 09:53
@jeremiahpslewis
Copy link
Contributor Author

@jeremiahpslewis Would you be interested in updating this PR to Mbed TLS 2.28 ?

Sure, superficially it's done. But until there's a 2.28.0 version available on Yggdrasil, I can't bump the _jll dependency.

@jeremiahpslewis
Copy link
Contributor Author

Just saw JuliaPackaging/Yggdrasil#4179 was merged. Now updated.

@jeremiahpslewis
Copy link
Contributor Author

One note...given that MbedTLS_jll has a lower bound of Julia 1.8, don't think this PR is a backport candidate...perhaps the labels should be removed?

@mkitti
Copy link
Contributor

mkitti commented Jan 7, 2022

Just a reminder that this will not work until we also update LibGit2 and Libssh2, and pin them on Mbed TLS 2.28 intead of 2.24 due to ABI changes in Mbed TLS.

https://github.com/JuliaPackaging/Yggdrasil/blob/90c82e2bb9c64f80893a8a25f5d2111400c7c333/L/LibGit2/build_tarballs.jl#L62
https://github.com/JuliaPackaging/Yggdrasil/blob/90c82e2bb9c64f80893a8a25f5d2111400c7c333/L/LibSSH2/build_tarballs.jl#L45

I think it's possible to update LibGit2 now since there is a new version available. There is not a new version of Libssh2 as far as I know.

@jeremiahpslewis
Copy link
Contributor Author

@mkitti Ok. Is it worth doing the fake version bump trick for Libssh2?

@mkitti
Copy link
Contributor

mkitti commented Jan 7, 2022

That's above my paygrade. Given that we likely have some time before everything is in place, we probably should just wait to see if Libssh2 releases anything. Another possibility is moving to Libssh instead of Libssh2, but that will require some action upstream:
libgit2/libgit2#4338
or moving away from Libgit2 entirely:
JuliaLang/Pkg.jl#2679

@ViralBShah
Copy link
Member

@jeremiahpslewis has an issue open on libssh2 and it seems that a 1.11 release is coming soon: libssh2/libssh2#657

@ViralBShah
Copy link
Member

I think we can close this one, since the other one (#43250) was merged and addresses the security issues (and there are no changes in the diff here).

@KristofferC KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels May 16, 2022
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.

7 participants