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>
It is now unreachable because:
- end_set == end_attr_data is enforced at library/x509_csr.c:236.
- end_exts == end_set is enforced at library/x509_csr.c:248.
- x509_csr_parse_extensions() only returns success if *p == end_exts, enforced
at library/x509_csr.c:188.
So after a successful x509_csr_parse_extensions() call, *p == end_exts ==
end_set == end_attr_data. The condition at line 257 cannot be true.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Don't allow parts of a compound (SEQUENCE or SET) to go beyond the
containing compound.
The inline CSR regression test is derived from the existing "X509 CSR
ASN.1 (OK)" DER test vector. In the extensionRequest attribute, the original
well-formed fragment is:
a029 3027 06092a864886f70d01090e 311a 3018 ...
where 31 1a is the SET containing the Extensions sequence and 30 18 is the
contained Extensions sequence. The malformed test changes only the SET
length byte, from 1a to 19:
a029 3027 06092a864886f70d01090e 3119 3018 ...
The attribute SEQUENCE length and the inner Extensions sequence length are
left unchanged. This makes the SET one byte too short for its containing
attribute, so the parser must reject it with
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH instead of parsing the trailing byte as
data outside the SET.
There is no known security impact, just some risk reduction.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
