Update to mbedtls 2.28.0 #173
Conversation
1. Added conditional compilation flags "MBEDTLS_FORCE_AESNI" and "MBEDTLS_FORCE_PADLOCK" 2. This allows us to supress cpuid based feature detection on sgx platforms. 3. "MBEDTLS_FORCE_AESNI" gets set if "force_aesni_support" flag is enabled. 4. Please refer to the previous commit for rust side changes. (cherry picked from commit d2317b0)
(cherry picked from commit 38522c2)
mbedtls_pk_sign() and mbedtls_pk_verify() and their extended and restartable variants now require at least the specified hash length if nonzero. Before, for RSA, hash_len was ignored in favor of the length of the specified hash algorithm. See: https://github.com/ARMmbed/mbedtls/releases
MihirLuthra
force-pushed
the
mihir/update-to-2.28
branch
from
4bda3a2 to
96f37bd
Compare
mbedtls-sys/build/config.rs
Outdated
| @@ -146,6 +146,7 @@ const DEFAULT_DEFINES: &'static [CDefine] = &[ | |||
| ("MBEDTLS_AES_ROM_TABLES", Undefined), | |||
| ("MBEDTLS_AES_FEWER_TABLES", Undefined), | |||
| ("MBEDTLS_CAMELLIA_SMALL_MEMORY", Undefined), | |||
| ("MBEDTLS_CHECK_RETURN_WARNING", Undefined), | |||
There was a problem hiding this comment.
Let's set this. Bindgen ignores it but it may be able to use it in the futures.
| @@ -377,6 +379,7 @@ const DEFAULT_DEFINES: &'static [CDefine] = &[ | |||
| ("MBEDTLS_PLATFORM_VSNPRINTF_MACRO", Undefined), // default: vsnprintf | |||
| ("MBEDTLS_PLATFORM_NV_SEED_READ_MACRO", Undefined), // default: mbedtls_platform_std_nv_seed_read | |||
| ("MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO", Undefined), // default: mbedtls_platform_std_nv_seed_write | |||
| ("MBEDTLS_CHECK_RETURN", Undefined), // default: __attribute__((__warn_unused_result__)) | |||
There was a problem hiding this comment.
As discussed, not adding this as this is a function-like macro (although, I am less knowledgeable about why). For reference:
/** \def MBEDTLS_IGNORE_RETURN
*
* This macro requires one argument, which should be a C function call.
* If that function call would cause a #MBEDTLS_CHECK_RETURN warning, this
* warning is suppressed.
*/
//#define MBEDTLS_IGNORE_RETURN( result ) ((void) !(result))
mbedtls/src/pk/mod.rs
Outdated
| @@ -1259,23 +1259,42 @@ iy6KC991zzvaWY/Ys+q/84Afqa+0qJKQnPuy/7F5GkVdQA/lfbhi | |||
| ]; | |||
|
|
|||
| for digest in &digests { | |||
| let digest = *digest; | |||
There was a problem hiding this comment.
Making changes to the test is not appropriate as this implies a public API change, which is backwards-incompatible. Instead, change Pk::sign to deal with the new behavior. The release notes imply this is a RSA-specific change, so please also add some ECDSA tests to make sure you're not making any changes to the behavior there.
There was a problem hiding this comment.
Also please add a test with hash=&[] as the release notes indicate special before if zero.
There was a problem hiding this comment.
What kind of changes are you expecting in Pk::sign? AFAIU, the change was a bugfix for a behaviour that was already incorrect. This change was backported in 2.26 requiring the argument hash_len to be at least the length of message digest. In 2.28, the length needs to be equal.
Passing a lesser length is surely a bug but we could probably add a patch to vendor to accept hash_len which is at least the length of message digest instead of equal and bump the patch version of mbedtls crate?
References:
[1] Mbed-TLS/mbedtls#4561
[2] Mbed-TLS/mbedtls#4707
There was a problem hiding this comment.
When I wrote my comment I was thinking we could make hash shorter in sign/verify. But actually, silently signing less than the full hash input by the user is also bad. So I think we need to make the incompatible change for security reasons. I've simplified your changes though (force-pushed onto your branch), please review.
There was a problem hiding this comment.
Thanks, those changes LGTM.
mbedtls_pk_sign() and mbedtls_pk_verify() and their extended and restartable variants now require at least the specified hash length if nonzero. Before, for RSA, hash_len was ignored in favor of the length of the specified hash algorithm. See: https://github.com/ARMmbed/mbedtls/releases
Although, bindgen needs .enable_function_attribute_detection() to process __attribute__((__warn_unused_result__)) because parsing attrs can be really slow in certain cases. Benches were performed to confirm our case doesn't face that issue. References: rust-lang/rust-bindgen#2149 rust-lang/rust-bindgen#1465 rust-lang/rust-bindgen#1466 rust-lang/rust-bindgen#1467
|
bors r+ |
|
Build succeeded: |
No description provided.