Update to a development version of Mbed TLS 2.16.0 #43
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the doxygen.sh script to the pre-push git script
Fixes #1883.
Remove the trailing whitespace from the inline assembly for AMD64 target, to overcome a warning in Clang, which was objecting to the string literal generated by the inline assembly being greater than 4096 characters specified by the ISO C99 standard. (-Woverlength-strings) This is a cosmetic change and doesn't change the logic of the code in any way. This change only fixes the problem for AMD64 target, and leaves other targets as they are. Fixes #482.
Add Changelog entry for inline assembly/literal strings too long issue with Clang.
Polish the beginning of mbedtls_rsa_rsaes_pkcs1_v15_decrypt a little, to prepare for some behavior changes.
mbedtls_rsa_rsaes_pkcs1_v15_decrypt took care of calculating the padding length without leaking the amount of padding or the validity of the padding. However it then skipped the copying of the data if the padding was invalid, which could allow an adversary to find out whether the padding was valid through precise timing measurements, especially if for a local attacker who could observe memory access via cache timings. Avoid this leak by always copying from the decryption buffer to the output buffer, even when the padding is invalid. With invalid padding, copy the same amount of data as what is expected on valid padding: the minimum valid padding size if this fits in the output buffer, otherwise the output buffer size. To avoid leaking payload data from an unsuccessful decryption, zero the decryption buffer before copying if the padding was invalid.
Make the function more robust by taking an arbitrary zero/nonzero argument instead of insisting on zero/all-bits-one. Update and fix its documentation.
mbedtls_rsa_rsaes_pkcs1_v15_decrypt takes care not to reveal whether the padding is valid or not, even through timing or memory access patterns. This is a defense against an attack published by Bleichenbacher. The attacker can also obtain the same information by observing the length of the plaintext. The current implementation leaks the length of the plaintext through timing and memory access patterns. This commit is a first step towards fixing this leak. It reduces the leak to a single memmove call inside the working buffer.
Replace memmove(to, to + offset, length) by a functionally equivalent function that strives to make the same memory access patterns regardless of the value of length. This fixes an information leak through timing (especially timing of memory accesses via cache probes) that leads to a Bleichenbacher-style attack on PKCS#1 v1.5 decryption using the plaintext length as the observable.
In mbedtls_rsa_rsaes_pkcs1_v15_decrypt, use size_greater_than (which is based on bitwise operations) instead of the < operator to compare sizes when the values being compared must not leak. Some compilers compile < to a branch at least under some circumstances (observed with gcc 5.4 for arm-gnueabi -O9 on a toy program).
Rather than doing the quadratic-time constant-memory-trace on the whole working buffer, do it on the section of the buffer where the data to copy has to lie, which can be significantly smaller if the output buffer is significantly smaller than the working buffer, e.g. for TLS RSA ciphersuites (48 bytes vs MBEDTLS_MPI_MAX_SIZE).
Get rid of the variable p. This makes it more apparent where the code accesses the buffer at an offset whose value is sensitive. No intended behavior change in this commit.
Functional tests for various payload sizes and output buffer sizes. When the padding is bad or the plaintext is too large for the output buffer, verify that function writes some outputs. This doesn't validate that the implementation is time-constant, but it at least validates that it doesn't just return early without outputting anything.
The code was making two unsequenced reads from volatile locations. This is undefined behavior. It was probably harmless because we didn't care in what order the reads happened and the reads were from ordinary memory, but UB is UB and IAR8 complained.
The code assumed that `int x = - (unsigned) u` with 0 <= u < INT_MAX sets `x` to the negative of u, but actually this calculates (UINT_MAX - u) and then converts this value to int, which overflows. Cast to int before applying the unary minus operator to guarantee the desired behavior.
Previously, mbedtls_pkcs5_pbes2() was unconditionally declared in `pkcs5.h` but defined as a stub returning `MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE` in case MBEDTLS_ASN1_PARSE_C was not defined. In line with the previous commits, this commit removes declaration and definition from both `pkcs5.h` and `pkcs5.c` in case MBEDTLS_ASN1_PARSE_C is not defined.
This commit duplicates the public function mbedtls_asn1_find_named_data() defined in library/asn1parse.c within library/asn1write.c in order to avoid a dependency of the ASN.1 writing module on the ASN.1 parsing module. The duplication is unproblematic from a semantic and an efficiency perspective becasue it is just a short list traversal that doesn't actually do any ASN.1 parsing.
This commit fixes issue #1212 related to platform-specific entropy polling in an syscall-emulated environment. Previously, the implementation of the entropy gathering function `mbedtls_platform_entropy_poll()` for linux machines used the following logic to determine how to obtain entropy from the kernel: 1. If the getrandom() system call identifier SYS_getrandom is present and the kernel version is 3.17 or higher, use syscall( SYS_getrandom, ... ) 2. Otherwise, fall back to reading from /dev/random. There are two issues with this: 1. Portability: When cross-compiling the code for a different architecture and running it through system call emulation in qemu, qemu reports the host kernel version through uname but, as of v.2.5.0, doesn't support emulating the getrandom() syscall. This leads to `mbedtls_platform_entropy_poll()` failing even though reading from /dev/random would have worked. 2. Style: Extracting the linux kernel version from the output of `uname` is slightly tedious. This commit fixes both by implementing the suggestion in #1212: - It removes the kernel-version detection through uname(). - Instead, it checks whether `syscall( SYS_getrandom, ... )` fails with errno set to ENOSYS indicating an unknown system call. If so, it falls through to trying to read from /dev/random. Fixes #1212.
Patater
added
needs: review
gilles-peskine-arm
requested changes
Feb 5, 2019
ChangeLog
Outdated
| @@ -6,6 +6,58 @@ Changes | |||
| * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() | |||
| from the cipher abstraction layer. Fixes #2198. | |||
|
|
|||
| = mbed TLS 2.16.0 branch released xxxx-xx-xx | |||
There was a problem hiding this comment.
This looks like it lists the changes in Mbed TLS 2.16.0 without a release date, but actually it only takes some of the changes of 2.16.0. Please don't use a version number here.
We don't have a precedent for splitting unreleased changes into two sections based on their origin. I think it would be fine to have a single x.xx.xx section here with the changes from both branches. But if you really think there should be separate sections, please use a non-misleading presentation, e.g. mbed TLS 2.14+01b34fb316a5d8c37244719be3f11ccf2ae85e41 unreleased
There was a problem hiding this comment.
As explained in the commit message, we have two sections because Mbed Crypto has a ChangeLog entry already added to the x.xx.x version for a future release. 2.16.0 without a release date was used as the marker for a development version of 2.16.0 (pre-release). Seems like this explanation wasn't clear enough, was in a location not looked at, and the marker used was too confusing in isolation. Will use 2.14+01b34fb316a5 as the version marker.
There was a problem hiding this comment.
I know what you did, but I don't understand why you did it, and the 2.16.0 would have to stand on its own because it doesn't look like one would need to track down the commit that introduced it to realize that 2.16.0 does not in fact mean 2.16.0.
There was a problem hiding this comment.
I've added a commit to change the identifier to something hopefully more clear.
library/rsa.c
Outdated
|
|
||
| /* Finally copy the decrypted plaintext plus trailing zeros | ||
| * into the output buffer. */ | ||
| memcpy( output, buf + ilen - plaintext_max_size, plaintext_max_size ); |
There was a problem hiding this comment.
This breaks when output_size is 0 and output is not a valid pointer (e.g. null). It's a known bug which came up when I was writing tests on the PSA branch. I thought CI would catch it, but it turns out that there's a test case with max_output_len=0 only for OAEP, not for v15 decryption. So this passes CI, but it's still a functional regression.
I suggest the following change, which may be better done in a subsequent commit than in the merge itself since it touches highly sensitive code:
/* Finally copy the decrypted plaintext plus trailing zeros
* into the output buffer. */
memcpy( output, buf + ilen - plaintext_max_size, plaintext_max_size );
* into the output buffer. If output_max_len is 0, then output
* may be an invalid pointer and memcpy would be undefined;
* check for this depending only on output_max_len which is public
* information. */
if( output_max_len != 0 )
memcpy( output, buf + ilen - plaintext_max_size, plaintext_max_size );There was a problem hiding this comment.
I've added a commit to fix this use case.
Patater
force-pushed
the
update-2.16-dev
branch
from
aa38c88 to
f30c7fe
Compare
|
Force pushed to fix typo in commit message. |
gilles-peskine-arm
requested changes
Feb 6, 2019
There was a problem hiding this comment.
- Ok for resolving the issues from the merge commit.
- There is an issue with the changes in the tests.
- Before merging this PR, please modify the merge commit:
- Squash the commit that modifies the changelog into the merge commit and reword the commit description accordingly.
- In the explanation about RSA, please state the consequences of taking the Mbed TLS branch, namely, the loss of support for v1.5 decryption with an empty output buffer.
| @@ -3258,6 +3258,7 @@ void asymmetric_decrypt_fail( int key_type_arg, | |||
| int alg_arg, | |||
| data_t *input_data, | |||
| data_t *label, | |||
| int expected_output_size, | |||
There was a problem hiding this comment.
I think you need to go through the int expected_output_size_arg and size_t expected_output_size = expected_output_size_arg dance, otherwise some compilers we use reject output_length == output_size as comparison between signed and unsigned.
There was a problem hiding this comment.
expected_output_size is assigned to output_size, a size_t, before use. I'll rename to match naming expectations and make the code less confusing.
| @@ -1389,59 +1397,59 @@ asymmetric_decrypt:PSA_KEY_TYPE_RSA_KEYPAIR:"3082025e02010002818100af057d396ee84 | |||
|
|
|||
| PSA decrypt: RSA OAEP-SHA-256, 30 bytes, wrong label (should be empty) | |||
| depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V21:MBEDTLS_SHA256_C | |||
| asymmetric_decrypt_fail:PSA_KEY_TYPE_RSA_KEYPAIR:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5a654aa8aa2470b541ee1feb0b12d25c79711531249e1129628042dbbb6c120d1443524ef4c0e6e1d8956eeb2077af12349ddeee54483bc06c2c61948cd02b202e796aebd94d3a7cbf859c2c1819c324cb82b9cd34ede263a2abffe4733f077869e8660f7d6834da53d690ef7985f6bc3020301000102818100874bf0ffc2f2a71d14671ddd0171c954d7fdbf50281e4f6d99ea0e1ebcf82faa58e7b595ffb293d1abe17f110b37c48cc0f36c37e84d876621d327f64bbe08457d3ec4098ba2fa0a319fba411c2841ed7be83196a8cdf9daa5d00694bc335fc4c32217fe0488bce9cb7202e59468b1ead119000477db2ca797fac19eda3f58c1024100e2ab760841bb9d30a81d222de1eb7381d82214407f1b975cbbfe4e1a9467fd98adbd78f607836ca5be1928b9d160d97fd45c12d6b52e2c9871a174c66b488113024100c5ab27602159ae7d6f20c3c2ee851e46dc112e689e28d5fcbbf990a99ef8a90b8bb44fd36467e7fc1789ceb663abda338652c3c73f111774902e840565927091024100b6cdbd354f7df579a63b48b3643e353b84898777b48b15f94e0bfc0567a6ae5911d57ad6409cf7647bf96264e9bd87eb95e263b7110b9a1f9f94acced0fafa4d024071195eec37e8d257decfc672b07ae639f10cbb9b0c739d0c809968d644a94e3fd6ed9287077a14583f379058f76a8aecd43c62dc8c0f41766650d725275ac4a1024100bb32d133edc2e048d463388b7be9cb4be29f4b6250be603e70e3647501c97ddde20a4e71be95fd5e71784e25aca4baf25be5738aae59bbfe1c997781447a2b24":PSA_ALG_RSA_OAEP(PSA_ALG_SHA_256):"3fd3c81e3919a19014400d91098090f273312e0150e09eff7f66fb9624d2ec9764fc80befcb592e9d102493c882b8bc0334a257e73aba23a0ee13f826cbc64f8200b9150784d004ccb2955c877c95ab888e3917f423dd52f3c8a49cb61c1966ec04f336068729ae0bce7d7fb3e680f9d15d658db9b906efcbf2c2fae45e75429":"00":PSA_ERROR_INVALID_PADDING | |||
| asymmetric_decrypt_fail:PSA_KEY_TYPE_RSA_KEYPAIR:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5a654aa8aa2470b541ee1feb0b12d25c79711531249e1129628042dbbb6c120d1443524ef4c0e6e1d8956eeb2077af12349ddeee54483bc06c2c61948cd02b202e796aebd94d3a7cbf859c2c1819c324cb82b9cd34ede263a2abffe4733f077869e8660f7d6834da53d690ef7985f6bc3020301000102818100874bf0ffc2f2a71d14671ddd0171c954d7fdbf50281e4f6d99ea0e1ebcf82faa58e7b595ffb293d1abe17f110b37c48cc0f36c37e84d876621d327f64bbe08457d3ec4098ba2fa0a319fba411c2841ed7be83196a8cdf9daa5d00694bc335fc4c32217fe0488bce9cb7202e59468b1ead119000477db2ca797fac19eda3f58c1024100e2ab760841bb9d30a81d222de1eb7381d82214407f1b975cbbfe4e1a9467fd98adbd78f607836ca5be1928b9d160d97fd45c12d6b52e2c9871a174c66b488113024100c5ab27602159ae7d6f20c3c2ee851e46dc112e689e28d5fcbbf990a99ef8a90b8bb44fd36467e7fc1789ceb663abda338652c3c73f111774902e840565927091024100b6cdbd354f7df579a63b48b3643e353b84898777b48b15f94e0bfc0567a6ae5911d57ad6409cf7647bf96264e9bd87eb95e263b7110b9a1f9f94acced0fafa4d024071195eec37e8d257decfc672b07ae639f10cbb9b0c739d0c809968d644a94e3fd6ed9287077a14583f379058f76a8aecd43c62dc8c0f41766650d725275ac4a1024100bb32d133edc2e048d463388b7be9cb4be29f4b6250be603e70e3647501c97ddde20a4e71be95fd5e71784e25aca4baf25be5738aae59bbfe1c997781447a2b24":PSA_ALG_RSA_OAEP(PSA_ALG_SHA_256):"3fd3c81e3919a19014400d91098090f273312e0150e09eff7f66fb9624d2ec9764fc80befcb592e9d102493c882b8bc0334a257e73aba23a0ee13f826cbc64f8200b9150784d004ccb2955c877c95ab888e3917f423dd52f3c8a49cb61c1966ec04f336068729ae0bce7d7fb3e680f9d15d658db9b906efcbf2c2fae45e75429":"00":0:PSA_ERROR_INVALID_PADDING | |||
There was a problem hiding this comment.
decrypt-fail tests should use a sufficient output buffer size (i.e. the key size) unless the expected failure is that the buffer is too small.
mpg
previously approved these changes
Feb 7, 2019
There was a problem hiding this comment.
I've performed the merge myself and checked that the differences between my merge and d27db6b here were all OK (I mistakenly used a slightly more recent development as the base for my merge).
I then went over the commits from the d27db6b to the tip of this PR and everything looks good to me, except it seems to be that you're saying you'll rename some arg in a test function but don't seem to have done it yet.
Overall, LGTM.
Patater
force-pushed
the
update-2.16-dev
branch
from
f30c7fe to
c57bd66
Compare
Merge a development version of Mbed TLS 2.16.0 that doesn't have parameter validation into development. The following conflicts were resolved: - Update ChangeLog to include release notes merged from development so far, with a version of "2.14.0+01b34fb316a5" and release date of "xxxx-xx-xx" to show this is not a released version, but instead a snapshot of the development branch equivalent to version of the 2.14.0 with additional commits from the mbedtls/development branch up through 01b34fb included. Entries added for unreleased versions of Mbed Crypto remain at the top of the file for Mbed TLS 2.xx.x. - Replace the Mbed Crypto version of mbedtls_rsa_rsaes_pkcs1_v15_decrypt() with the version from Mbed TLS which fixes timing variations and memory access variations that could lead to a Bleichenbacher-style padding oracle attack. This will prevent using psa_asymmetric_decrypt() with zero-length output buffers until a follow up commit is made to restore this capability. - In ssl_srv.c, include changes for both the new ECDH interface and opaque PSK as already added to development previously.
After merging the latest RSA implementation from Mbed TLS, we have a regression in that we no longer properly handle zero-length null output in PKCS1 v1.5 decryption. Prevent undefined behavior by avoiding a memcpy() to zero-length null output buffers.
When RSA decrypting, unlike with RSA encrypting, we sometimes expect the output length will be less than the key size. For instance, in the case where the plaintext is zero-length we expect the output length of the decryption to be zero-length as well, not key size in length. For must-fail tests, we don't expect output-buffer-sized RSA-decryption, only that the output length is less than or equal to the output size, so these tests remain unchanged. Change the must-pass tests to expect that the actual output size is equal to the expected length of the output buffer instead of always being the key size.
For must-fail asymmetric decryption tests, add an output size parameter so that tests can directly control what output buffer size they allocate and use independently from the key size used. This enables better testing of behavior with various output buffer sizes.
The tests use a ciphertext for PKCS#1 v1.5 encryption of a zero-length buffer that was created with a call to psa_asymmetric_encrypt().
The macro PSA_HASH_FINAL_SIZE no longer exists and all instances of it should be replaced by PSA_HASH_SIZE. Replace all remaining instances of PSA_HASH_FINAL_SIZE with PSA_HASH_SIZE.
Patater
force-pushed
the
update-2.16-dev
branch
from
c57bd66 to
7f04214
Compare
|
Rebased to address review comments. Modified how must-fail asymmetric decryption tests are updated to handle test-case-controlled output buffer sizes. Applied fixup commit to merge commit. Old branch at https://github.com/Patater/mbed-crypto/tree/update-2.16-dev-1 |
gilles-peskine-arm
approved these changes
Feb 7, 2019
mpg
removed
the
needs: review
|
The Jenkins run on the merged PR only failed on a in a 3d+fragmentation test that is known to be flaky with the improper version of GnuTLS currently in our CI. That's as good as a pass, so I'm removing "needs: ci". |
gilles-peskine-arm
pushed a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
May 22, 2019
The fact that self-signed end-entity certs can be explicitly trusted by putting them in the CA list even if they don't have the CA bit was not documented though it's intentional, and tested by "Certificate verification ARMmbed#73 (selfsigned trusted without CA bit)" in test_suite_x509parse.data It is unclear to me whether the restriction that explicitly trusted end-entity certs must be self-signed is a good one. However, it seems intentional as it is tested in tests ARMmbed#42 and ARMmbed#43, so I'm not touching it for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update to a development version of Mbed TLS 2.16.0 without parameter validation as a step along the way to getting the full Mbed TLS 2.16.0 changes included in Mbed Crypto. The particular commit chosen as an intermediate point begins before parameter validation to avoid having to resolve parameter validation conflicts in both ARMmbed/mbedtls:development-psa and ARMmbed/mbed-crypto:development. The idea is to bring in as much into Mbed Crypto as soon as we can, and this is a good point in Mbed TLS 2.16.0 history from which to bring in those changes.
This brings in ECDH changes that are needed to support using Mbed Crypto as a submodule in Mbed TLS versions 2.16.0 or greater.