Co-authored-by: minosgalanakis <30719586+minosgalanakis@users.noreply.github.com>
Signed-off-by: Ben Taylor <32939606+bjwtaylor@users.noreply.github.com>
(cherry picked from commit 8de9f6aa776d5e92d77b10dfca6666f83f542ab2)
Co-authored-by: minosgalanakis <30719586+minosgalanakis@users.noreply.github.com>
Signed-off-by: Ben Taylor <32939606+bjwtaylor@users.noreply.github.com>
(cherry picked from commit 9d6a54795c96f7f0d7dfc230071dc16c5f47f94a)
Use a more structured way of isolating and translating sensitive error codes
from RSA PKCS#1v1.5 decryption.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Since Mbed TLS 3.6.3, the field `badmac_seen_or_in_hsfraglen` in the
`mbedtls_ssl_context` structure has a dual purpose: it is the new name of
`badmac_seen` for DTLS, and it has a new meaning `in_hsfraglen` for TLS.
This was done to add support for handshake message fragmentation in TLS
while preserving the library ABI.
There was a bug in some places in the code, which reads the dual-purpose
field in the wrong protocol. Specifically:
* `mbedtls_ssl_prepare_handshake_record()` (beginning): mistakenly reads
`badmac_seen_or_in_hsfraglen` in DTLS to mean `in_hsfraglen`. (DTLS
handshake defragmentation does not use `in_hsfraglen`.) As a
consequence, in DTLS, after receiving a record with a bad MAC, the field
`ssl->in_hslen` was not correctly updated, which causes problems further
in handshake message processing.
* `mbedtls_ssl_prepare_handshake_record()` (end): correctly updates
`badmac_seen_or_in_hsfraglen` in TLS with the `in_hsfraglen` meaning.
* `ssl_consume_current_message()`: incorrectly read
`badmac_seen_or_in_hsfraglen` in DTLS to mean `in_hsfraglen`, and thus
stops processing messages after receiving a record with a bad MAC.
* `ssl_get_next_record()`: accesses `badmac_seen_or_in_hsfraglen` to mean
`badmac_seen` in DTLS. No bug here.
* `mbedtls_ssl_handle_message_type()`: reads `badmac_seen_or_in_hsfraglen`
to mean `in_hsfraglen` in TLS. No bug here.
* `mbedtls_ssl_session_reset_msg_layer()`: both fields need to be reset
when not partial, and a partial reset only applies to DTLS where we do
want to preserve `badmac_seen` in a partial reset. No bug here.
* `mbedtls_ssl_context_save()`, `mbedtls_ssl_context_load()`: saving
`badmac_seen` in DTLS is desired and done. Saving `in_hsfraglen` in TLS
is harmless.
To fix this, in `ssl_msg.c`, always access the dual-purpose field
`badmac_seen_or_in_hsfraglen` through getters and setters that are specific
to one purpose and that check that the purpose matches the protocol.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In `ssl_client2`, the existing option `renegotiate=1` causes the client to
initiate renegotiation immediately after the initial handshake. Add a
variant `renegotiate=2` that initiates renegotiation after one data exchange.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
After a message with a bad MAC (invalid application data record), try to
renegotiate:
* Test cases where renegotiation (nominal or delayed) is rejected.
Nothing bad happens.
* A test where renegotiation is misparsed and rejected.
The subsequent parsing fails.
* A test where the renegotiation request is delayed.
This causes a heap buffer overflow in `ssl_buffer_message()`.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This can be used to simulate data messages received in the wrong epoch, or
to cause a handshake message to be buffered.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Non-regression test for a bug introduced with TLS handshake defragmentation
support in Mbed TLS 3.6.2, whereby the confusion between the `badmac_seen`
meaning and the `in_hsfraglen` meaning of `ssl->badmac_seen_or_in_hsfraglen`
causes renegotiation to be processed incorrectly after receiving a message
with a bad MAC.
This bug will be fixed in a subsequent commit.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Let test code customize the kitchen sink function
`mbedtls_test_ssl_perform_handshake()` at a few points.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
I believe the existing link was to the TF wiki which has been retired
for some time now.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Announce the security fix together with the overflow fix, since the two bugs
are pretty much indistinguishable from a black-box perspective, despite
being due to independent problems in the code.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In an X.509 certificate, a basicConstraints extension where the cA
field (BOOLEAN) is omitted but the pathLenConstraint field (INTEGER )is
present is syntactically valid, but RFC 5280 states that CAs MUST NOT emit
such a certificate.
Historically, since XySSL 0.9 in 2008, if a basicConstraints extension
started with an INTEGER field, we interpreted that field as a cA value. This
was a deliberate change:
> Fixed x509_get_ext() to accept some rare certificates (like
> www.openssl.org:443) which have an INTEGER instead of a BOOLEAN for
> Extension::BasicConstraints::cA.
However, 18 years later, this does not seem to be relevant, and the details
seem to have been lost. Furthermore, major implementations follow RFC 5280
and interpret `SEQUENCE { INTEGER n }` with cA being set to its default
value (FALSE) and pathLenConstraint set to `n`. This means that such a
certificate with n != 0 would be interpreted as a CA certificate in Mbed TLS
but as a leaf certificate elsewhere, which is dangerous.
Since CAs are not supposed to emit such certificates, stop accepting them
altogether.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In test cases for pathLenConstraint parsing in a basicConstraints extension
of an X.509 certificate, use the standard type for the leading cA field,
namely BOOLEAN. The test data was encoding that field as INTEGER, which was
accepted as a deviation from the standard since XySSL 0.9.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
