Let the caller handle returning on error. This is less surprising, and it's
more flexible in case the caller needs to perform some cleanup (which is
currently not the case).
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Remove the "srv" variant of "DTLS proxy: server: get invalid AD record then
accept renego", where the server initiates the renegotiation process by
sending a HelloRequest. The HelloRequest message is not relevant to this
test. The first message of the actual renegotiation is the ClientHello
message from the client, and the interesting part of the test happens when
the client receives the server's response.
Remove "DTLS proxy: server: get invalid AD record then accept early
renego (cli)" and the corresponding "reject" control. This test case doesn't
really do what it is advertised to do, and in particular does not lead to a
call to `ssl_consume_current_message()` in the server. What happens is that
the client sends a ClientHello message to start the renegotiation, then it
blocks until the server has replied. So there is nothing to delay that
ClientHello after:(unlike what happens in the corresponding client-side
test, where the first flight of the renegotiation protocol contains multiple
messages, so we can delay the first of these messages after the second one).
What happens in practice is that the client times out, then resends its
ClientHello, and the proxy sends the delayed ClientHello immediately after
the resent one, which does not cause any interesting behavior in the server.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Only set `badmac_limit` on the side that actually receives a message with a
bad MAC/tag (injected by the proxy).
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Add a test entry point for a handshake with a specially crafted context.
This can be used for defense in depths for scenarios that we think can't
happen, but might actually happen due to a bug, or due to a new feature that
is insufficiently tested. This can also be a proxy for testing aspects of
renegotiation or session resumption in circumstances that are hard to
arrange.
In this commit, use it to test a DTLS handshake with `ssl->badmac_seen`
:(actually `ssl->badmac_seen_or_in_hsfraglen` in the 3.6 branch) set to a
nonzero value. This can't happen, as far as I know (`badmac_seen` can only
be nonzero during renegotiation, not during a nominal handshake or in
session resumption). But it could happen, for example, if
`mbedtls_ssl_session_reset()` failed to reset `badmac_seen`, as was the case
in Mbed TLS 4.0.0 to 4.1.0.
Signed-off-by: Gilles Peskine <Gilles.Peskine@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>
Clarify that the 'sig_algs' set through this function are only enforced
during the key exchange and that 'mbedtls_ssl_conf_cert_profile()' should
instead be used to enforce the same algorithms when verifying certificates.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
We want to reject small order points for two reasons:
- Those give rise to side channels attacks where the underlying field
arithmetic is not constant time (which our bignum isn't so far),
see the "May the Fourth" paper mentioned in ecp.
- Those force the shared secret to be all-bytes zero, which is bad for
protocols that might require contributory behaviour.
Honest peers will never send low-order points anyway.
While at it, test that we are clipping the input to 255 bits (ie, that
we are unsetting bit 256) as required by the RFC.
Again, honest peers will never send strings with bit 256 set. But the
RFC says to accept such strings anyway, so that's what we do.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The exact value doesn't matter anyway, but since we mention it, it might
as well be correct.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Those no longer allocate, so they can no longer return that error. This
was missed in the commit that removed heap allocations.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Add comments explaining why 3 passes are enough (except for 224 where 1 is enough).
For p256 and p384, this is a bit different
from some other functions as the carry is signed here.
Rm FIPS reference from the documentation, it's about the implementation.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
On principle:
- If the code is written with a loop, the compiler can easily unroll the
loop at high optimisation levels; if we unroll the loop in the source,
the compiler is unlikely to roll it up at -Os/-Oz. So loops are more
adaptable.
- All the other functions are using loops, and there's nothing special
about those functions that would justify a different treatement.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The comments already explained why the result is in the desired range.
However 2^P521_WIDTH didn't make sense as P521_WIDTH is a number of
limbs no bits, so fix that typo.
Remove the FIPS reference as we're not following it and it doesn't bring
anything useful.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
