When parsing a certificate with the basic constraints extension
the max_pathlen that was read from it was incremented regardless
of its value. However, if the max_pathlen is equal to INT_MAX (which
is highly unlikely), an undefined behaviour would occur.
This commit adds a check to ensure that such value is not accepted
as valid. Relevant tests for INT_MAX and INT_MAX-1 are also introduced.
Certificates added in this commit were generated using the
test_suite_x509write, function test_x509_crt_check. Input data taken
from the "Certificate write check Server1 SHA1" test case, so the generated
files are like the "server1.crt", but with the "is_ca" field set to 1 and
max_pathlen as described by the file name.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Signed-off-by: Piotr Nowicki <piotr.nowicki@arm.com>
See the comments in the code for how an attack would go, and the ChangeLog
entry for an impact assessment. (For ECDSA, leaking a few bits of the scalar
over several signatures translates to full private key recovery using a
lattice attack.)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
We keep track of the current epoch and record sequence number in out_ctr,
which was overwritten when writing the record containing the
HelloVerifyRequest starting from out_buf. We can avoid that by only using the
rest of the buffer.
Using MBEDTLS_SSL_MAX_CONTENT_LEN as the buffer size is still correct, as it
was a pretty conservative value when starting from out_buf.
Note: this bug was also fixed unknowingly in 2.13 by introducing a new buffer
that holds the current value of the sequence number (including epoch), while
working on datagram packing: 198594709b
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The current logging was sub-standard, in particular there was no trace
whatsoever of the HelloVerifyRequest being sent. Now it's being logged with
the usual levels: 4 for full content, 2 return of f_send, 1 decision about
sending it (or taking other branches in the same function) because that's the
same level as state changes in the handshake, and also same as the "possible
client reconnect" message" to which it's the logical continuation (what are we
doing about it?).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
In x509.c, the self-test code is dependent on MBEDTLS_CERTS_C and
MBEDTLS_SHA256_C being enabled. At some point in the recent past that dependency
was on MBEDTLS_SHA1_C but changed to SHA256, but the comment wasn't updated.
This commit updates the comment.
Signed-off-by: Simon Butcher <simon.butcher@arm.com>
Some code paths want to access members of the mbedtls_rsa_context structure.
We can only do that when using our own implementation, as otherwise we don't
know anything about that structure.
When parsing a PKCS#1 RSAPrivateKey structure, all parameters are always
present. After importing them, we need to call rsa_complete() for the sake of
alternative implementations. That function interprets zero as a signal for
"this parameter was not provided". As that's never the case, we mustn't pass
any zero value to that function, so we need to explicitly check for it.
This reverts commit 130e136439, reversing
changes made to 071b3e170e.
stat() will never return S_IFLNK as the file type, as stat() explicitly
follows symlinks.
Fixes#3005.
If Y was constructed through functions in this module, then Y->n == 0
iff Y->p == NULL. However we do not prevent filling mpi structures
manually, and zero may be represented with n=0 and p a valid pointer.
Most of the code can cope with such a representation, but for the
source of mbedtls_mpi_copy, this would cause an integer underflow.
Changing the test for zero from Y->p==NULL to Y->n==0 causes this case
to work at no extra cost.
The comment on TEST_SRV_CRT_RSA_SHA256 that it was
tests/data_files/server2-sha256.crt was a lie, the contents were actually
those of the mbedtls-2.16 version of the same file.
While it didn't have a noticeable impact on its own, it was confusing and
distracting while investigating an issue that cause gnutls-cli to not trust
the default RSA-SHA256 cert given test-ca.crt as a root, so worth fixing.
When mbedtls_x509_crt_parse_path() checks each object in the supplied path, it only processes regular files. This change makes it also accept a symlink to a file. Fixes#3005.
This was observed to be a problem on Fedora/CentOS/RHEL systems, where the ca-bundle in the default location is actually a symlink.
* origin/mbedtls-2.7:
Enable more test cases without MBEDTLS_MEMORY_DEBUG
More accurate test case description
Clarify that the "FATAL" message is expected
Note that mbedtls_ctr_drbg_seed() must not be called twice
Fix CTR_DRBG benchmark
Changelog entry for xxx_drbg_set_entropy_len before xxx_drbg_seed
CTR_DRBG: support set_entropy_len() before seed()
CTR_DRBG: Don't use functions before they're defined
HMAC_DRBG: support set_entropy_len() before seed()
The functions mbedtls_ctr_drbg_random() and
mbedtls_ctr_drbg_random_with_add() could return 0 if an AES function
failed. This could only happen with alternative AES
implementations (the built-in implementation of the AES functions
involved never fail), typically due to a failure in a hardware
accelerator.
Bug reported and fix proposed by Johan Uppman Bruce and Christoffer
Lauri, Sectra.
In ssl_parse_hello_verify_request, we read 3 bytes (version and cookie
length) without checking that there are that many bytes left in
ssl->in_msg. This could potentially read from memory outside of the
ssl->receive buffer (which would be a remotely exploitable
crash).
In ssl_parse_hello_verify_request, we print cookie_len bytes without
checking that there are that many bytes left in ssl->in_msg. This
could potentially log data outside the received message (not a big
deal) and could potentially read from memory outside of the receive
buffer (which would be a remotely exploitable crash).
* restricted/pr/666: (24 commits)
Add ChangeLog entry
mpi_lt_mpi_ct: fix condition handling
mpi_lt_mpi_ct: Add further tests
mpi_lt_mpi_ct: Fix test numbering
mpi_lt_mpi_ct perform tests for both limb size
ct_lt_mpi_uint: cast the return value explicitely
mbedtls_mpi_lt_mpi_ct: add tests for 32 bit limbs
mbedtls_mpi_lt_mpi_ct: simplify condition
Rename variable for better readability
mbedtls_mpi_lt_mpi_ct: Improve documentation
Make mbedtls_mpi_lt_mpi_ct more portable
Bignum: Document assumptions about the sign field
Add more tests for mbedtls_mpi_lt_mpi_ct
mpi_lt_mpi_ct test: hardcode base 16
Document ct_lt_mpi_uint
mpi_lt_mpi_ct: make use of unsigned consistent
ct_lt_mpi_uint: make use of biL
Change mbedtls_mpi_cmp_mpi_ct to check less than
mbedtls_mpi_cmp_mpi_ct: remove multiplications
Remove excess vertical space
...
This issue has been reported by Tuba Yavuz, Farhaan Fowze, Ken (Yihang) Bai,
Grant Hernandez, and Kevin Butler (University of Florida) and
Dave Tian (Purdue University).
In AES encrypt and decrypt some variables were left on the stack. The value
of these variables can be used to recover the last round key. To follow best
practice and to limit the impact of buffer overread vulnerabilities (like
Heartbleed) we need to zeroize them before exiting the function.
In the case of *ret we might need to preserve a 0 value throughout the
loop and therefore we need an extra condition to protect it from being
overwritten.
The value of done is always 1 after *ret has been set and does not need
to be protected from overwriting. Therefore in this case the extra
condition can be removed.
The code relied on the assumptions that CHAR_BIT is 8 and that unsigned
does not have padding bits.
In the Bignum module we already assume that the sign of an MPI is either
-1 or 1. Using this, we eliminate the above mentioned dependency.
The signature of mbedtls_mpi_cmp_mpi_ct() meant to support using it in
place of mbedtls_mpi_cmp_mpi(). This meant full comparison functionality
and a signed result.
To make the function more universal and friendly to constant time
coding, we change the result type to unsigned. Theoretically, we could
encode the comparison result in an unsigned value, but it would be less
intuitive.
Therefore we won't be able to represent the result as unsigned anymore
and the functionality will be constrained to checking if the first
operand is less than the second. This is sufficient to support the
current use case and to check any relationship between MPIs.
The only drawback is that we need to call the function twice when
checking for equality, but this can be optimised later if an when it is
needed.
Multiplication is known to have measurable timing variations based on
the operands. For example it typically is much faster if one of the
operands is zero. Remove them from constant time code.
