Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport 3.6: dtls_server: allow unexpected message on second handshake #9655

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Oct 1, 2024

In dtls_server, don't treat MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE as a real error if it happens during a handshake. This error can happen if the client sends a close_notify alert after the server has decided to close the connection, or if there's a duplicated packet from the first connection.

In testing, this error happens reliably when the client is dtls_client or ssl_client2 dtls=1. It also happens reliably when the client is openssl s_client -dtls_12 and I type into it interactively. When the client is echo stuff | openssl s_client -dtls_12, I don't see the error locally, but it's happening with low frequency on the CI.

Fixes #9652. Improvement filed as #9666.

PR checklist

If MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE happens during the handshake, don't
show it as an "error". It might be an error, but it might also be a fact of
life if it happens during the second or more handshake: it can be a
duplicated packet or a close_notify alert from the previous connection,
which is hard to avoid and harmless.

Fixes Mbed-TLS#9652.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Now that dtls_server doesn't print "error" when it receives stray messages
while it's waiting for a second handshake, have the tests fail if "error" is
printed for some other reason.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added component-tls needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Oct 1, 2024
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-very-high Highest priority - prioritise this over other review work and removed needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Oct 1, 2024
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 2, 2024
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@davidhorstmann-arm davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 2, 2024
@mpg mpg added this pull request to the merge queue Oct 2, 2024
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 711d583 Oct 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-tls priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most)
Development

Successfully merging this pull request may close these issues.

3 participants