Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3 #8574

ronald-cron-arm · 2023-11-28T17:13:47Z

Description

Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3
Follow-up of #6788, partially supersede #8525

Fixes #7795

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog not required, internal changes to the TLS implementation only
backport not required
tests provided

yuhaoth

Just take a quick look.

include/mbedtls/ssl.h

library/ssl_tls.c

library/ssl_ticket.c

mpg · 2023-12-08T09:41:45Z

include/mbedtls/ssl.h

+     *  session, the ticket creation time may need to be saved for the ticket
+     *  module to be able to check the ticket age when the ticket is used.
+     *  That's the purpose of this field.
+     *  Before to create a new ticket, an Mbed TLS server set this field with


Minor: before creating a new ticket

Disable ticket module if either the TLS server or the support for session tickets is not enabled at build time as in that case the ticket module is not used by the TLS library. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ssl_ticket.c is a server module. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

The ticket module is removed from the build if the TLS server is not in the build now thus no need for the guard. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

When calculating the ticket age, remove the check that the endpoint is a server. The module is supposed to be used only server side. Furthermore, if such check was necessary, it should be at the beginning of all ssl_ticket.c APIs. As there is no such protection in any API, just remove the check. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Take into account that the lifetime of tickets can be changed through the mbedtls_ssl_ticket_rotate() API. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

The endpoint field is needed to serialize/deserialize a session in TLS 1.2 the same way it is needed in the TLS 1.3 case: client specific fields that should not be in the serialized version on server side if both TLS client and server are enabled in the TLS library. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

The purpose of this change is to eventually base the calculation in ssl_ticket.c of the ticket age when parsing a ticket on the ticket creation time both in TLS 1.2 and TLS 1.3 case. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Move the save/load of session endpoint and ciphersuite that are common to TLS 1.2 and TLS 1.3 serialized data from the specialized ssl_{tls12,tls13}_session_{save,load} functions to ssl__session_{save,load}. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm · 2024-01-15T09:31:41Z

I've rebased and addressed the comments.

gilles-peskine-arm

This mostly looks good to me, with a doubt about partly preexisting design and a few minor change requests.

gilles-peskine-arm · 2024-02-06T15:15:49Z

include/mbedtls/ssl.h

+     *  Before creating a new ticket, an Mbed TLS server set this field with
+     *  its current time in milliseconds. This time may then be saved in the
+     *  session ticket data by the session ticket writing function and
+     *  recovered by the ticket parsing function later when the ticket is used.


Info: why do it this way, rather than letting the ticket writing function query the current time if it wants? That would seem more natural to me.

If this was a session creation time used for multiple purposes, this approach would be necessary, since there would have to be a single point where the official session creation time is decided. But for the ticket creation time, the (single) ticket writing function is the natural place.

(I realize that this is not new from this pull request: it's from #6788, which isn't released yet, and which AFAICT wasn't part of the public API until this PR which exposes it via mbedtls_ssl_session_get_ticket_creation_time.)

First, thanks for the review.

Info: why do it this way, rather than letting the ticket writing function query the current time if it wants?

One important point is that when using the ticket the TLS 1.3 server needs the ticket_creation_time, it cannot be only a ticket module data. Thus it means that the ticket_creation_time needs to be in the ticket data.
In our implementation of mbedtls_ssl_ticket_write_t (and it is expected to be so in third party ones) the creation of the ticket data is based on mbedtls_ssl_session_save() thus when mbedtls_ssl_session_save() is called the
ticket_creation_time of the mbedtls_ssl_session structure must be set.

We could do it in mbedtls_ssl_ticket_write() but it is not straightforward as the session input parameter is a const: const mbedtls_ssl_session *session (we would need to copy the session ...). And if we do it that way with another implementation of mbedtls_ssl_ticket_write_t based on mbedtls_ssl_session_save() we would not add the ticket_creation_time in the ticket data.

include/mbedtls/ssl.h

tests/src/test_helpers/ssl_helpers.c

include/mbedtls/ssl.h

library/ssl_tls.c

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm · 2024-02-10T11:05:44Z

@gilles-peskine-arm the CI is green (but ABI-API check as expected) and I believe I have addressed your comment now, please have another look, thanks.

gilles-peskine-arm

LGTM

tom-cosgrove-arm · 2024-02-20T11:07:20Z

@mpg doing second review on this

mpg

LGTM!

mpg · 2024-02-21T09:39:38Z

partially supersede #8525

Was it really partial? Or can #8525 be closed now?

ronald-cron-arm · 2024-02-21T09:46:20Z

partially supersede #8525

Was it really partial? Or can #8525 be closed now?

I have to check but yes I think there is still some work to do.

Since the merge of Mbed-TLS#8574 it is not the case anymore that the lifetime of keys is twice the lifetime of tickets. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm added enhancement component-tls needs-ci Needs to pass CI tests component-tls13 priority-high High priority - will be reviewed soon labels Nov 28, 2023

yuhaoth mentioned this pull request Nov 29, 2023

TLS 1.3 Session Ticket: Add ticket lifetime check for TLS1.3 server #8525

Closed

3 tasks

yuhaoth suggested changes Nov 29, 2023

View reviewed changes

include/mbedtls/ssl.h Outdated Show resolved Hide resolved

library/ssl_tls.c Outdated Show resolved Hide resolved

library/ssl_tls.c Outdated Show resolved Hide resolved

ronald-cron-arm force-pushed the ssl-tickets branch from 9c4ca84 to dba6fbd Compare November 29, 2023 08:21

yuhaoth reviewed Nov 30, 2023

View reviewed changes

library/ssl_ticket.c Show resolved Hide resolved

yuhaoth mentioned this pull request Nov 30, 2023

Add ticket lifetime check for tls13 server solution 2 #8581

Closed

3 tasks

mpg reviewed Dec 8, 2023

View reviewed changes

ronald-cron-arm linked an issue Dec 19, 2023 that may be closed by this pull request

Rare intermittent failure of a TLS 1.3 session resumption test #7795

Closed

ronald-cron-arm added 12 commits January 15, 2024 08:50

Disable ticket module when useless

d1100b0

Disable ticket module if either the TLS server or the support for session tickets is not enabled at build time as in that case the ticket module is not used by the TLS library. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ssl_ticket.c: Remove client code

ce72763

ssl_ticket.c is a server module. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ssl_ticket.c: Remove TLS server guard

3c3e2e6

The ticket module is removed from the build if the TLS server is not in the build now thus no need for the guard. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ssl_ticket.c: Fix ticket lifetime enforcement

ba5165e

Take into account that the lifetime of tickets can be changed through the mbedtls_ssl_ticket_rotate() API. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Add endpoint in TLS 1.2 session serialization data

7b1921a

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Fix TLS 1.2 session serialization on server side

feb577a

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Define ticket creation time in TLS 1.2 case as well

d1c106c

The purpose of this change is to eventually base the calculation in ssl_ticket.c of the ticket age when parsing a ticket on the ticket creation time both in TLS 1.2 and TLS 1.3 case. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Add ticket creation time to TLS 1.2 session serialization

c57f86e

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ssl_ticket.c: Base ticket age check on the ticket creation time

3c0072b

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm force-pushed the ssl-tickets branch from dba6fbd to 40a4ab0 Compare January 15, 2024 09:30

ronald-cron-arm requested a review from mpg January 15, 2024 09:34

ronald-cron-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 and removed needs-ci Needs to pass CI tests labels Feb 6, 2024

gilles-peskine-arm self-requested a review February 6, 2024 15:13

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Feb 6, 2024

gilles-peskine-arm requested changes Feb 6, 2024

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 6, 2024

ronald-cron-arm added 4 commits February 9, 2024 09:26

ssl_session: Reorder some fields to reduce padding

7fdee8b

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: Improve test parameter sanity check

c7fa82e

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Add change log for mbedtls_ssl_session_get_ticket_creation_time()

7b0ac0b

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tls12: Fix documentation of TLS 1.2 session serialized data

a93e25e

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm added needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Feb 9, 2024

ronald-cron-arm requested a review from gilles-peskine-arm February 10, 2024 11:04

gilles-peskine-arm approved these changes Feb 12, 2024

View reviewed changes

tom-cosgrove-arm added needs-reviewer This PR needs someone to pick it up for review and removed needs-reviewer This PR needs someone to pick it up for review labels Feb 20, 2024

mpg approved these changes Feb 21, 2024

View reviewed changes

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Feb 21, 2024

mpg added this pull request to the merge queue Feb 21, 2024

mpg added the approved Design and code approved - may be waiting for CI or backports label Feb 21, 2024

Merged via the queue into Mbed-TLS:development with commit 0ecb5fd Feb 21, 2024
4 of 6 checks passed

ronald-cron-arm mentioned this pull request Mar 11, 2024

TLS 1.3: Enforce ticket maximum lifetime and discard tickets with 0 lifetime #8913

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3 #8574

Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3 #8574

ronald-cron-arm commented Nov 28, 2023 •

edited by mpg

Loading

yuhaoth left a comment

mpg Dec 8, 2023

ronald-cron-arm commented Jan 15, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 6, 2024

ronald-cron-arm Feb 7, 2024 •

edited

Loading

ronald-cron-arm commented Feb 10, 2024

gilles-peskine-arm left a comment

tom-cosgrove-arm commented Feb 20, 2024

mpg left a comment

mpg commented Feb 21, 2024

ronald-cron-arm commented Feb 21, 2024

Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3 #8574

Fix and align ticket age check in ssl_ticket.c for TLS 1.2 and TLS 1.3 #8574

Conversation

ronald-cron-arm commented Nov 28, 2023 • edited by mpg Loading

Description

PR checklist

yuhaoth left a comment

Choose a reason for hiding this comment

mpg Dec 8, 2023

Choose a reason for hiding this comment

ronald-cron-arm commented Jan 15, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 6, 2024

Choose a reason for hiding this comment

ronald-cron-arm Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm commented Feb 10, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Feb 20, 2024

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 21, 2024

ronald-cron-arm commented Feb 21, 2024

ronald-cron-arm commented Nov 28, 2023 •

edited by mpg

Loading

ronald-cron-arm Feb 7, 2024 •

edited

Loading