Import/mbedtls 3.6.2 #7135
Import/mbedtls 3.6.2 #7135
Conversation
Imports Mbed TLS 3.6.2 from https://github.com/Mbed-TLS/mbedtls.git tags mbedtls-3.6.2, v3.6.2 Files that are not needed are removed: cd lib/libmbedtls rm -rf mbedtls cp -R path/to/mbedtls-3.6.2/mbedtls . cd mbedtls rm CMakeLists.txt DartConfiguration.tcl Makefile rm .gitignore .travis.yml .pylintrc .globalrc .mypy.ini BRANCHES.md rm include/.gitignore include/CMakeLists.txt library/.gitignore rm library/CMakeLists.txt library/Makefile rm -r cmake rm -rf .git .github doxygen configs programs scripts tests visualc rm -rf 3rdparty ChangeLog.d docs pkgconfig .gitmodules .readthedocs.yaml rm library/mps_* cd .. git add mbedtls This is a complete overwrite of previous code so earlier changes in the previous branch import/mbedtls-3.6.0 will be added on top of this commit. Signed-off-by: Sungbae Yoo <[email protected]>
Removes default config include/mbedtls/config.h Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebased onto mbedtls-2.22.0] [jf: rebased onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jf: rebased onto mbedtls-2.28.1] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
Configures mbedtls to reach outside the imported source tree for configuration .h file. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebased onto mbedtls-2.22.0] [jf: rebased onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0 and removed inclusion of check_config.h] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
from
bfd6c44
to
832aca4
Compare
|
Hi @ysbnim, thanks for putting this together. This looks good overall but I do have a concern with commit "libmbedtls: bignum: restore mbedtls_mpi_exp_mod() from v3.5.2", which was a temporary measure until the performance regression in MBed TLS 3.6.0 was fixed (it was addressed upstream in Mbed-TLS/mbedtls#9281). Ideally I would like to drop the above commit and instead use the upstream code. However if I understand correctly the performance regression was fixed for some use cases only (public exponents). The fix may be useless for our test case ( |
|
The changelog of mbedtls 3.6.1 mentions the issue you reported:
(edited) Discard my comment, you already put more detailed on that. Let's try and see how OP-TEE behaves w/ and w/o this patch. |
|
Tested on my 32bit platform: |
|
Tested
|
|
Commit "libmbedtls: bignum: restore mbedtls_mpi_exp_mod() from v3.5.2" should be replaced by this one: "libmbedtls: use 'unsafe' algorithm for mbedtls_mpi_exp_mod(). |
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
2 times, most recently
from
dfe22af
to
0973860
Compare
|
Checked that it takes 26.12s on my machine. Thanks @jforissier ! |
|
Test OK on my 32bit platforms. I see the 'unsafe' implementation seems <10% slower compared to the patch that restore the mbedTLS v3.5.2 implementation, but that is not an issue at all. I wonder whether there should be a config switch to define which of the safe/unsafe modular exponentiation scheme to embed in mbedTLS? It should somewhat clear that a timing-attack defense of mbedTLS is withdrawn when the 'unsafe' function is used. |
That would make sense, although adding config flags is not good for test coverage... TBH xtest 4011 should not be the only reason for choosing the unsafe algorithm. If all the other tests are reasonably quick with the safe algorithm, then we could introduce an "unsafe" CFG ( |
|
How about intoducing new I think QEMUv7 is the only environment that needs unsafe mod exp. 64bit QEMU also has some performance regression but it would be acceptible. |
|
@ysbnim sounds good to me. Would you please update this PR to do just that? Thanks! |
Makes mbedtls_mpi_montg_init(), mbedtls_mpi_montmul() and mbedtls_mpi_montred() available for external use. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebased onto mbedtls-2.22.0] [jf: rebased onto mbedtls-2.27.0, keep static functions] Signed-off-by: Jerome Forissier <[email protected]> [jf: rebased onto mbedtls-2.28.1] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0, replace original functions] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
Adds mbedtls_mpi_init_mempool() which initializes a mbedtls_mpi struct to use the mempool mbedtls_mpi_mempool if configured for memory allocation. All local memory allocation are changed to use mbedtls_mpi_init_mempool() instead of mbedtls_mpi_init(). This will give a stack like alloc/free pattern for which the mempool is optimized. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebased onto mbedtls-2.22.0] [jf: rebased onto mbedtls-2.27.0, fold fixup commit: 2df910b ("libmbedtls: mbedtls_mpi_shrink(): fix possible unwanted truncation"), adjust macro ECP_MPI_INIT] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0, adjust new coding style] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0, reintroduce mbedtls_mpi_zeroize] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
Increase the count limit when generating the witness in the Rabin-Miller primality test. The previous number 30 was too low to reliably detect 000000022770A7DC599BC90B2FF981CCB5CF05703344C8F350418AAD as a prime number. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebased onto mbedtls-2.22.0] [jf: rebased onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
For integrating into OPTEE_OS, it needs add some interfaces: 1. add mbedtls_cipher_clone() for cipher to copy context between two operations. 2. add mbedtls_cipher_setup_info() for cipher. cipher need to get its "cipher_info" according the key length, while the key length is not an input in allocate function. So, use a default key len in the beginning. It need to reset the cipher info again in init function. 3. add mbedtls_cipher_cmac_setup() for cmac. This function is separate from mbedtls_cipher_cmac_starts(). 4. copy hmac context in md. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Edison Ai <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebase onto mbedtls-2.22.0] [jf: rebase onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jf: rebase onto mbedtls-2.28.1, fix typo in comment] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebase onto mbedtls-3.4.0, adjust new coding style] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebase onto mbedtls-3.6.0, adjust for changes between 3.4 and 3.6] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
In NO_CRT mode, Q and P may be invalid. But Q and P will be re-filled again if PRNG function is valid. So add judgement process if it is in NO_CRT mode. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Summer Qin <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [jf: rebase onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebase onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
When MBEDTLS_ECP_DP_SM2_ENABLED is set, adds support for the ECC curve defined for the Chinese SM2 algorithm (G/MT 0003 Part 5, [1]). Link: [1] http://www.gmbz.org.cn/upload/2018-07-24/1532401863206085511.pdf Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]> [jf: rebased onto mbedtls-2.27.0] Signed-off-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
Adds fault mitigation in mbedtls_rsa_rsassa_pss_verify_ext() by using the macro FTMN_CALLEE_DONE_MEMCMP() instead of memcmp() when checking that the hash in the RSA signature is matching the expected value. FTMN_CALLEE_DONE_MEMCMP() saves on success the result in a thread local storage if fault mitigations was enabled when the function was called. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
…fy() Adds fault mitigation in mbedtls_rsa_rsassa_pkcs1_v15_verify() by using the macro FTMN_CALLEE_DONE_MEMCMP() instead of just mbedtls_safer_memcmp() when checking that the hash in the RSA signature is matching the expected value. FTMN_CALLEE_DONE_MEMCMP() saves on success the result in a thread local storage if fault mitigations was enabled when the function was called. Acked-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
For chacha20 and chachapoly, the *_ctx_clone() function is missing and therefore the wrong function pointers are assigned to .ctx_clone_func and .ctx_free_func when MBEDTLS_CHACHA20_C or MBEDTLS_CHACHAPOLY_C is enabled. Signed-off-by: Simon Ott <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> [jw: rebased onto mbedtls-3.4.0] Signed-off-by: Jens Wiklander <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
For AES Key Wrap mode, the *_ctx_clone() function is missing and therefore the wrong function pointers are assigned to .ctx_clone_func and .ctx_free_func when MBEDTLS_NIST_KW_C is enabled. Signed-off-by: Jens Wiklander <[email protected]> Acked-By: Jerome Forissier <[email protected]> [tve: rebased onto mbedtls-3.6.0] Signed-off-by: Tom Van Eyck <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
mbedtls/library/common.h includes arm_neon.h since 3.5.0, which collides with the definition of __section and __data in compiler.h. Temporarily remove those definitions while including arm_neon.h. Signed-off-by: Tom Van Eyck <[email protected]> Acked-by: Jens Wiklander <[email protected]> [sby: rebased onto mbedtls-3.6.2] Signed-off-by: Sungbae Yoo <[email protected]>
I agree, but 32bit platform maintainers should also be warned. I think i'll also default enable this switch in |
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
2 times, most recently
from
0959841
to
7707afc
Compare
There was a problem hiding this comment.
CFG_CORE_UNSAFE_MODEXP, as per its name, relates to OP-TEE core but the switch is also applied when libmbedtls is built as a user land trusted library.
It would be nice the 2 configs are distinguished. Or the swith may be renamed CFG_MBEDTLS_UNSAFE_MODEXP but IMHO 2 config switches would be better. @jforissier, what do you think?
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
from
7707afc
to
5c8b606
Compare
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
from
5c8b606
to
aab1ddc
Compare
There was a problem hiding this comment.
No need for a blank line after the Link: tags.
With my comment below addressed, please add:
Reviewed-by: Jerome Forissier <[email protected]>`
...to commit "libmbedtls: add CFG_CORE_UNSAFE_MODEXP and CFG_TA_MEBDTLS_UNSAFE_MODEXP".
Thanks!
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
from
aab1ddc
to
ee1655e
Compare
|
Removed blank line and add review tag |
There was a problem hiding this comment.
Reviewed-by: Etienne Carriere <[email protected]> for commit
"libmbedtls: add CFG_CORE_UNSAFE_MODEXP and CFG_TA_MEBDTLS_UNSAFE_MODEXP".
Modular exponentiation in MBed TLS v3.6.2 comes in two flavors: an 'unsafe' one, mbedtls_mpi_exp_mod_unsafe(), and a 'safe' one called mbedtls_mpi_exp_mod(). Here safe/unsafe refers to resistance against timing attacks (the safe variant is constant-time and usually much slower). The reason for having the two variants is that the fastest may be used with public keys while the slowest should be used with private keys. This commit introduces CFG_CORE_UNSAFE_MODEXP for TEE core and CFG_TA_MEBDTLS_UNSAFE_MODEXP for the MBedTLS library for TAs. Those configurations switch mbedtls_mpi_exp_mod() to the unsafe variant for better performance. This commit adds CFG_CORE_UNSAFE_MODEXP=y to QEMUv7 tests as well because the problem with the safe variant which is now the default is that it introduces a large performance regression in "time xtest 4011" which makes the QEMUv7 tests in particular impractical: QEMUv8 QEMUv7 3.4.0 (OP-TEE 4.2.0) 0m 0.85s 0m 14.29s 3.6.2 w/o this commit 0m 21.83s 8m 3.04s 3.6.2 w/ this commit 0m 0.93s 0m 14.34s Prior to v3.6.0, MBed TLS had no constant time implementation. Link: https://optee.readthedocs.io/en/latest/building/devices/qemu.html#qemu-v7 [1] Link: Mbed-TLS/mbedtls@1ba4058 Signed-off-by: Sungbae Yoo <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>
ysbnim
force-pushed
the
import/mbedtls-3.6.2
branch
from
ee1655e
to
d6555c7
Compare
|
@ysbnim thank you for your contribution. I will now merge this PR into the import branch, and I will later take care of creating the squashed commit and pull request to merge into master. |
Default enable CFG_CORE_UNSAFE_MODEXP on plat-stm32mp1 to run xtest regression and pkcs11 TA tests in a decent amount of time. Link: OP-TEE#7135 (comment) Signed-off-by: Etienne Carriere <[email protected]>
Default enable CFG_CORE_UNSAFE_MODEXP for plat-stm32mp1 to run xtest regression and pkcs11 TA tests in a decent amount of time. Link: OP-TEE#7135 (comment) Signed-off-by: Etienne Carriere <[email protected]>
This pull request imports MbedTLS 3.6.2 to fix CVEs (Related issue : #7133)
This also rebases several commits according to PR #6797.