The previous code triggered a compiler warning because of a comparison
of a signed and an unsigned integer.
The conversion is safe because `len` is representable by 16-bits,
hence smaller than the maximum integer.
In the previous bounds check `(*p) > end - len`, the computation
of `end - len` might underflow if `end` is within the first 64KB
of the address space (note that the length `len` is controlled by
the peer). In this case, the bounds check will be bypassed, leading
to `*p` exceed the message bounds by up to 64KB when leaving
`ssl_parse_server_psk_hint()`. In a pure PSK-based handshake,
this doesn't seem to have any consequences, as `*p*` is not accessed
afterwards. In a PSK-(EC)DHE handshake, however, `*p` is read from
in `ssl_parse_server_ecdh_params()` and `ssl_parse_server_dh_params()`
which might lead to an application crash of information leakage.
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.
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).
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).
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.
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.
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 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.
Deprecate mbedtls_hmac_drbg_update (which returns void) in favor of a
new function mbedtls_hmac_drbg_update_ret which reports error. The old
function is not officially marked as deprecated in this branch because
this is a stable maintenance branch.
Deprecate mbedtls_ctr_drbg_update (which returns void) in favor of a
new function mbedtls_ctr_drbg_update_ret which reports error. The old
function is not officially marked as deprecated in this branch because
this is a stable maintenance branch.
When writing a private EC key, use a constant size for the private
value, as specified in RFC 5915. Previously, the value was written
as an ASN.1 INTEGER, which caused the size of the key to leak
about 1 bit of information on average, and could cause the value to be
1 byte too large for the output buffer.
The debugging functions
- mbedtls_debug_print_ret,
- mbedtls_debug_print_buf,
- mbedtls_debug_print_mpi, and
- mbedtls_debug_print_crt
return immediately if the SSL configuration bound to the
passed SSL context is NULL, has no debugging functions
configured, or if the debug threshold is below the debugging
level.
However, they do not check whether the provided SSL context
is not NULL before accessing the SSL configuration bound to it,
therefore leading to a segmentation fault if it is.
In contrast, the debugging function
- mbedtls_debug_print_msg
does check for ssl != NULL before accessing ssl->conf.
This commit unifies the checks by always returning immediately
if ssl == NULL.
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.
Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.
This commit fixes this bug. Fixes#1914.
In `mbedtls_ccm_self_test()`, enforce input and output
buffers sent to the ccm API to be contigous and aligned,
by copying the test vectors to buffers on the stack.
- in x509_profile_check_pk_alg
- in x509_profile_check_md_alg
- in x509_profile_check_key
and in ssl_cli.c : unsigned char gets promoted to signed integer
In ecp_mul_comb(), if (!p_eq_g && grp->T == NULL) and then ecp_precompute_comb() fails (which can
happen due to OOM), then the new array of points T will be leaked (as it's newly allocated, but
hasn't been asigned to grp->T yet).
Symptom was a memory leak in ECDHE key exchange under low memory conditions.
The length to the debug message could conceivably leak through the time it
takes to print it, and that length would in turn reveal whether padding was
correct or not.
The basis for the Lucky 13 family of attacks is for an attacker to be able to
distinguish between (long) valid TLS-CBC padding and invalid TLS-CBC padding.
Since our code sets padlen = 0 for invalid padding, the length of the input to
the HMAC function, and the location where we read the MAC, give information
about that.
A local attacker could gain information about that by observing via a
cache attack whether the bytes at the end of the record (at the location of
would-be padding) have been read during MAC verification (computation +
comparison).
Let's make sure they're always read.
The basis for the Lucky 13 family of attacks is for an attacker to be able to
distinguish between (long) valid TLS-CBC padding and invalid TLS-CBC padding.
Since our code sets padlen = 0 for invalid padding, the length of the input to
the HMAC function gives information about that.
Information about this length (modulo the MD/SHA block size) can be deduced
from how much MD/SHA padding (this is distinct from TLS-CBC padding) is used.
If MD/SHA padding is read from a (static) buffer, a local attacker could get
information about how much is used via a cache attack targeting that buffer.
Let's get rid of this buffer. Now the only buffer used is the internal MD/SHA
one, which is always read fully by the process() function.
Move definition of `MBEDTLS_CIPHER_MODE_STREAM` to header file
(`mbedtls_cipher_internal.h`), because it is used by more than
one file. Raised by TrinityTonic in #1719
Address review comments:
1. add `mbedtls_cipher_init()` after freeing context, in test code
2. style comments
3. set `ctx->iv_size = 0` in case `IV == NULL && iv_len == 0`
Fix compilation warnings with IAR toolchain, on 32 bit platform.
Reported by rahmanih in #683
This is based on work by Ron Eldor in PR #750, some of which was independently
fixed by Azim Khan and already merged in PR #1655.
This PR fixes multiple issues in the source code to address issues raised by
tests/scripts/check-files.py. Specifically:
* incorrect file permissions
* missing newline at the end of files
* trailing whitespace
* Tabs present
* TODOs in the souce code
As a protection against the Lucky Thirteen attack, the TLS code for
CBC decryption in encrypt-then-MAC mode performs extra MAC
calculations to compensate for variations in message size due to
padding. The amount of extra MAC calculation to perform was based on
the assumption that the bulk of the time is spent in processing
64-byte blocks, which was correct for most supported hashes but not for
SHA-384. Adapt the formula to 128-byte blocks for SHA-384.
Fix IAR compiler warnings
Two warnings have been fixed:
1. code 'if( len <= 0xFFFFFFFF )' gave warning 'pointless integer comparison'.
This was fixed by wraping the condition in '#if SIZE_MAX > 0xFFFFFFFF'.
2. code 'diff |= A[i] ^ B[i];' gave warning 'the order of volatile accesses is undefined in'.
This was fixed by read the volatile data in temporary variables before the computation.
Explain IAR warning on volatile access
Consistent use of CMAKE_C_COMPILER_ID
Clarify what MBEDTLS_ERR_ECP_SIG_LEN_MISMATCH and
MBEDTLS_ERR_PK_SIG_LEN_MISMATCH mean. Add comments to highlight that
this indicates that a valid signature is present, unlike other error
codes. See
https://github.com/ARMmbed/mbedtls/pull/1149#discussion_r178130705
The relevant ASN.1 definitions for a PKCS#8 encoded Elliptic Curve key are:
PrivateKeyInfo ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKey PrivateKey,
attributes [0] IMPLICIT Attributes OPTIONAL
}
AlgorithmIdentifier ::= SEQUENCE {
algorithm OBJECT IDENTIFIER,
parameters ANY DEFINED BY algorithm OPTIONAL
}
ECParameters ::= CHOICE {
namedCurve OBJECT IDENTIFIER
-- implicitCurve NULL
-- specifiedCurve SpecifiedECDomain
}
ECPrivateKey ::= SEQUENCE {
version INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),
privateKey OCTET STRING,
parameters [0] ECParameters {{ NamedCurve }} OPTIONAL,
publicKey [1] BIT STRING OPTIONAL
}
Because of the two optional fields, there are 4 possible variants that need to
be parsed: no optional fields, only parameters, only public key, and both
optional fields. Previously mbedTLS was unable to parse keys with "only
parameters". Also, only "only public key" was tested. There was a test for "no
optional fields", but it was labelled incorrectly as SEC.1 and not run because
of a great renaming mixup.
Conflict resolution:
* ChangeLog
* tests/data_files/Makefile: concurrent additions, order irrelevant
* tests/data_files/test-ca.opensslconf: concurrent additions, order irrelevant
* tests/scripts/all.sh: one comment change conflicted with a code
addition. In addition some of the additions in the
iotssl-1381-x509-verify-refactor-restricted branch need support for
keep-going mode, this will be added in a subsequent commit.
In mbedtls_ssl_derive_keys, don't call mbedtls_md_hmac_starts in
ciphersuites that don't use HMAC. This doesn't change the behavior of
the code, but avoids relying on an uncaught error when attempting to
start an HMAC operation that hadn't been initialized.
Found by running:
CC=clang cmake -D CMAKE_BUILD_TYPE="Check"
tests/scripts/depend-pkalgs.pl
(Also tested with same command but CC=gcc)
Another PR will address improving all.sh and/or the depend-xxx.pl scripts
themselves to catch this kind of thing.
If RSA-CRT is used for signing, and if an attacker can cause a glitch
in one of the two computations modulo P or Q, the difference between
the faulty and the correct signature (which is not secret) will be
divisible by P or Q, but not by both, allowing to recover the private
key by taking the GCD with the public RSA modulus N. This is known as
the Bellcore Glitch Attack. Verifying the RSA signature before handing
it out is a countermeasure against it.
Fix the x509_get_subject_alt_name() function to not accept invalid
tags. The problem was that the ASN.1 class for tags consists of two
bits. Simply doing bit-wise and of the CONTEXT_SPECIFIC macro with the
input tag has the potential of accepting tag values 0x10 (private)
which would indicate that the certificate has an incorrect format.
This is the beginning of a series of commits refactoring the chain
building/verification functions in order to:
- make it simpler to understand and work with
- prepare integration of restartable ECC
md() already checks for md_info == NULL. Also, in the future it might also
return other errors (eg hardware errors if acceleration is used), so it make
more sense to check its return value than to check for NULL ourselves and then
assume no other error can occur.
Also, currently, md_info == NULL can never happen except if the MD and OID modules
get out of sync, or if the user messes with members of the x509_crt structure
directly.
This commit does not change the current behaviour, which is to treat MD errors
the same way as a bad signature or no trusted root.
Fix warnings from gcc -O -Wall about `ret` used uninitialized in
CMAC selftest auxiliary functions. The variable was indeed
uninitialized if the function was called with num_tests=0 (which
never happens).
In 2.7.0, we replaced a number of MD functions with deprecated inline
versions. This causes ABI compatibility issues, as the functions are no
longer guaranteed to be callable when built into a shared library.
Instead, deprecate the functions without also inlining them, to help
maintain ABI backwards compatibility.
Add missing MBEDTLS_DEPRECATED_REMOVED guards around the definitions
of mbedtls_aes_decrypt and mbedtls_aes_encrypt.
This fixes the build under -Wmissing-prototypes -Werror.
Fixes#1388
Currently only SHA1 is supported as PRF algorithm for PBKDF2
(PKCS#5 v2.0).
This means that keys encrypted and authenticated using
another algorithm of the SHA family cannot be decrypted.
This deficiency has become particularly incumbent now that
PKIs created with OpenSSL1.1 are encrypting keys using
hmacSHA256 by default (OpenSSL1.0 used PKCS#5 v1.0 by default
and even if v2 was forced, it would still use hmacSHA1).
Enable support for all the digest algorithms of the SHA
family for PKCS#5 v2.0.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
- Rephrase file/function/parameter/enum/define/error descriptions into full
and clear sentences.
- Make sure to adhere to the Arm writing guidelines.
- Fix missing/incorrect Doxygen tags.
- Standardize terminology used within the file.
- Add full standard name in file description.
GitHub PR: #1316
- Rephrase file/function/parameter/enum/define/error descriptions into full
and clear sentences.
- Make sure to adhere to the Arm writing guidelines.
- Fix missing/incorrect Doxygen tags.
- Standardize terminology used within the file.
- Rephrase the descriptions of all md_alg and hashlen parameters.
GitHub PR: #1327
- Rephrase file/function/parameter/enum/define/error descriptions into full
and clear sentences.
- Make sure to adhere to the Arm writing guidelines.
- Fix missing/incorrect Doxygen tags.
- Standardize terminology used within the file.
- Standardize defines documentation
GitHub PR: #1323
- Rephrase function/parameter/enum/define/error descriptions into full and
clear sentences.
- Make sure to adhere to the Arm writing guidelines.
- Fix missing/incorrect Doxygen tags.
- Standardize terminology used within the file.
GitHub PR: #1306
- Rephrase function/parameter/enum/define/error descriptions into full and
clear sentences.
- Make sure to adhering to the Arm writing guidelines.
- Fix missing/incorrect Doxygen tags.
- Standardize terminology used within the file.
- Fix iv_len values per the standard.
GitHub PR: #1305
- Separate "\file" blocks from copyright, so that Doxygen doesn't repeat
the copyright information in all the Detailed Descriptions.
- Improve phrasing and clarity of functions, parameters, defines and enums.
GitHub PR: #1292
A new test for mbedtls_timing_alarm(0) was introduced in PR 1136, which also
fixed it on Unix. Apparently test results on MinGW were not checked at that
point, so we missed that this new test was also failing on this platform.
Add MBEDTLS_ERR_XXX_HW_ACCEL_FAILED error codes for all cryptography
modules where the software implementation can be replaced by a hardware
implementation.
This does not include the individual message digest modules since they
currently have no way to return error codes.
This does include the higher-level md, cipher and pk modules since
alternative implementations and even algorithms can be plugged in at
runtime.
This commit allows users to provide alternative implementations of the
ECJPAKE interface through the configuration option MBEDTLS_ECJPAKE_ALT.
When set, the user must add `ecjpake_alt.h` declaring the same
interface as `ecjpake.h`, as well as add some compilation unit which
implements the functionality. This is in line with the preexisting
support for alternative implementations of other modules.
The corner cases fixed include:
* Allocating a buffer of size 0. With this change, the allocator now
returns a NULL pointer in this case. Note that changes in pem.c and
x509_crl.c were required to fix tests that did not work under this
assumption.
* Initialising the allocator with less memory than required for headers.
* Fix header chain checks for uninitialised allocator.
