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.

Sign up for GitHub

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

Comply with the received Record Size Limit extension #7455

Merged
merged 14 commits into from
Jan 9, 2024
Merged

Comply with the received Record Size Limit extension #7455

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f482dcc
Comply with the received Record Size Limit extension
KloolK Mar 15, 2023
9aec1c7
Add record size checking during handshake
waleed-elmelegy-arm Dec 5, 2023
419f841
Skip checking on maximum fragment length during handshake
waleed-elmelegy-arm Dec 7, 2023
05d670b
Revert "Skip checking on maximum fragment length during handshake"
waleed-elmelegy-arm Dec 14, 2023
26e3698
Revert back checking on handshake messages length
waleed-elmelegy-arm Dec 14, 2023
049cd30
Refactor record size limit extension handling
waleed-elmelegy-arm Dec 20, 2023
65e3046
Fix code style in ssl_tls.c
waleed-elmelegy-arm Dec 20, 2023
6a971fd
Refactor and improve Record size limit handling
waleed-elmelegy-arm Dec 28, 2023
87a373e
Improve Record size limit testing
waleed-elmelegy-arm Dec 28, 2023
ea03183
Adjust TLS 1.3 tests to new maximum output changes
waleed-elmelegy-arm Dec 29, 2023
bae705c
Fix TLS 1.2 test to use TLS 1.2 maximum output size
waleed-elmelegy-arm Jan 1, 2024
3d46b7f
Fix Max fragmen length test to use TLS 1.2 maximum output size
waleed-elmelegy-arm Jan 1, 2024
60f0f72
Add config dependencies to record size tests
waleed-elmelegy-arm Jan 4, 2024
e2d3db5
Update mbedtls_ssl_get_output_record_size_limit signature
waleed-elmelegy-arm Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/mbedtls/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,11 @@ struct mbedtls_ssl_session {
unsigned char MBEDTLS_PRIVATE(mfl_code); /*!< MaxFragmentLength negotiated by peer */
#endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */

/*!< RecordSizeLimit received from the peer */
#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
uint16_t MBEDTLS_PRIVATE(record_size_limit);
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

unsigned char MBEDTLS_PRIVATE(exported);

/** TLS version negotiated in the session. Used if and when renegotiating
Expand Down
18 changes: 18 additions & 0 deletions library/ssl_misc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,24 @@ size_t mbedtls_ssl_get_output_max_frag_len(const mbedtls_ssl_context *ssl);
size_t mbedtls_ssl_get_input_max_frag_len(const mbedtls_ssl_context *ssl);
#endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */

#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
/**
* \brief Return the RecordSizeLimit (in bytes) for
* the output buffer. This is less than the value requested by the
* peer (see RFC 8449), since it subtracts the space required for the
* content type and padding of the TLSInnerPlaintext struct (RFC 8446).
* Returns MBEDTLS_SSL_OUT_CONTENT_LEN if no limit was requested by the peer.
*
* \sa mbedtls_ssl_get_max_out_record_payload()
* ssl_compute_internal_record_size_limit()
*
* \param ssl SSL context
*
* \return Current record size limit for the output buffer.
*/
size_t mbedtls_ssl_get_output_record_size_limit(const mbedtls_ssl_context *ssl);
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH)
static inline size_t mbedtls_ssl_get_output_buflen(const mbedtls_ssl_context *ctx)
{
Expand Down
68 changes: 67 additions & 1 deletion library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2455,6 +2455,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite(
* uint8 ticket_flags;
* opaque resumption_key<0..255>;
* uint32 max_early_data_size;
* uint16 record_size_limit;
* select ( endpoint ) {
* case client: ClientOnlyData;
* case server: uint64 ticket_creation_time;
Expand Down Expand Up @@ -2490,6 +2491,9 @@ static int ssl_tls13_session_save(const mbedtls_ssl_session *session,
#if defined(MBEDTLS_SSL_EARLY_DATA)
needed += 4; /* max_early_data_size */
#endif
#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
needed += 2; /* record_size_limit */
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#if defined(MBEDTLS_HAVE_TIME)
needed += 8; /* ticket_creation_time or ticket_reception_time */
Expand Down Expand Up @@ -2534,6 +2538,10 @@ static int ssl_tls13_session_save(const mbedtls_ssl_session *session,
MBEDTLS_PUT_UINT32_BE(session->max_early_data_size, p, 0);
p += 4;
#endif
#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
MBEDTLS_PUT_UINT16_BE(session->record_size_limit, p, 0);
p += 2;
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C)
if (session->endpoint == MBEDTLS_SSL_IS_SERVER) {
Expand Down Expand Up @@ -2610,6 +2618,13 @@ static int ssl_tls13_session_load(mbedtls_ssl_session *session,
session->max_early_data_size = MBEDTLS_GET_UINT32_BE(p, 0);
p += 4;
#endif
#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
if (end - p < 2) {
return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to think that if someone was using Mbed TLS just before this PR, and had a saved session, then recompiled with this PR, and tried to resume, they'd get an MBEDTLS_ERR_SSL_VERSION_MISMATCH (considering it the version of the data, rather than just the version of the library) error, not a MBEDTLS_ERR_SSL_BAD_INPUT_DATA error, although I suspect the caller would treat both identically.

That would suggest that MBEDTLS_SSL_RECORD_SIZE_LIMIT should be incorporated in SSL_SERIALIZED_SESSION_CONFIG_BITFLAG

However, the same reasoning applies to MBEDTLS_SSL_EARLY_DATA, so this looks like pre-existing, rather than anything else, and I'm mainly making the observation here, rather than asking for a change.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would suggest that MBEDTLS_SSL_RECORD_SIZE_LIMIT should be incorporated in SSL_SERIALIZED_SESSION_CONFIG_BITFLAG

That's a good point. I think we overlooked this aspect of serialized sessions while working on new session tickets, early data and now record size limit. I am creating an issue to address that: #8686.

}
session->record_size_limit = MBEDTLS_GET_UINT16_BE(p, 0);
p += 2;
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C)
if (session->endpoint == MBEDTLS_SSL_IS_SERVER) {
Expand Down Expand Up @@ -3372,6 +3387,31 @@ const char *mbedtls_ssl_get_version(const mbedtls_ssl_context *ssl)
}
}

#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)

size_t mbedtls_ssl_get_output_record_size_limit(const mbedtls_ssl_context *ssl)
{
const size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN;
size_t record_size_limit = max_len;

if (ssl->session != NULL &&
ssl->session->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN &&
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me under what circumstances we would put a value < MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN into the session. Would it make more sense to verify when populating ssl->session->record_size_limit and then be able to skip the check here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do not receive any record size limit extension then ssl->session->record_size_limit is equal to zero. Thus to me this check is to detect if the peer sent a record size limit or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - if this test had been for > 0 then I might have thought that.

Would (in the future, not this PR) it make more sense to set this to the value to use for record size and use it unconditionally?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that would be an improvement.

ssl->session->record_size_limit < max_len) {
record_size_limit = ssl->session->record_size_limit;
}

// TODO: this is currently untested
/* During a handshake, use the value being negotiated */
if (ssl->session_negotiate != NULL &&
ssl->session_negotiate->record_size_limit >= MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN &&
ssl->session_negotiate->record_size_limit < max_len) {
record_size_limit = ssl->session_negotiate->record_size_limit;
}

return record_size_limit;
}
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH)
size_t mbedtls_ssl_get_input_max_frag_len(const mbedtls_ssl_context *ssl)
{
Expand Down Expand Up @@ -3458,6 +3498,7 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl)
size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN;

#if !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) && \
!defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT) && \
!defined(MBEDTLS_SSL_PROTO_DTLS)
(void) ssl;
#endif
Expand All @@ -3470,6 +3511,30 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl)
}
#endif

#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
const size_t record_size_limit = mbedtls_ssl_get_output_record_size_limit(ssl);

if (max_len > record_size_limit) {
max_len = record_size_limit;
}
#endif

if (ssl->transform_out != NULL &&
ssl->transform_out->tls_version == MBEDTLS_SSL_VERSION_TLS1_3) {
/* RFC 8449, section 4:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately clear why this is not within the MBEDTLS_SSL_RECORD_SIZE_LIMIT block. Some of the text in Ronald's comment might help here

*
* This value [record_size_limit] is the length of the plaintext
* of a protected record.
* The value includes the content type and padding added in TLS 1.3
* (that is, the complete length of TLSInnerPlaintext).
*
* Thus, round down to a multiple of MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY
* and subtract 1 (for the content type that will be added later)
*/
max_len = ((max_len / MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) *
MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY) - 1;
}

#if defined(MBEDTLS_SSL_PROTO_DTLS)
if (mbedtls_ssl_get_current_mtu(ssl) != 0) {
const size_t mtu = mbedtls_ssl_get_current_mtu(ssl);
Expand All @@ -3492,7 +3557,8 @@ int mbedtls_ssl_get_max_out_record_payload(const mbedtls_ssl_context *ssl)
#endif /* MBEDTLS_SSL_PROTO_DTLS */

#if !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) && \
!defined(MBEDTLS_SSL_PROTO_DTLS)
!defined(MBEDTLS_SSL_PROTO_DTLS) && \
!defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
((void) ssl);
#endif

Expand Down
22 changes: 16 additions & 6 deletions library/ssl_tls13_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2113,12 +2113,11 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl,

ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(
ssl, p, p + extension_data_len);

/* TODO: Return unconditionally here until we handle the record
* size limit correctly. Once handled correctly, only return in
* case of errors. */
return ret;

if (ret != 0) {
MBEDTLS_SSL_DEBUG_RET(
1, ("mbedtls_ssl_tls13_parse_record_size_limit_ext"), ret);
return ret;
}
break;
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

Expand All @@ -2132,6 +2131,17 @@ static int ssl_tls13_parse_encrypted_extensions(mbedtls_ssl_context *ssl,
p += extension_data_len;
}

if ((handshake->received_extensions & MBEDTLS_SSL_EXT_MASK(RECORD_SIZE_LIMIT)) &&
(handshake->received_extensions & MBEDTLS_SSL_EXT_MASK(MAX_FRAGMENT_LENGTH))) {
MBEDTLS_SSL_DEBUG_MSG(3,
(
"Record size limit extension cannot be used with max fragment length extension"));
MBEDTLS_SSL_PEND_FATAL_ALERT(
MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,
MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER);
return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER;
}

MBEDTLS_SSL_PRINT_EXTS(3, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS,
handshake->received_extensions);

Expand Down
11 changes: 4 additions & 7 deletions library/ssl_tls13_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1714,7 +1714,7 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl,

MBEDTLS_SSL_DEBUG_MSG(2, ("RecordSizeLimit: %u Bytes", record_size_limit));

/* RFC 8449, section 4
/* RFC 8449, section 4:
*
* Endpoints MUST NOT send a "record_size_limit" extension with a value
* smaller than 64. An endpoint MUST treat receipt of a smaller value
Expand All @@ -1727,14 +1727,11 @@ int mbedtls_ssl_tls13_parse_record_size_limit_ext(mbedtls_ssl_context *ssl,
return MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER;
}

MBEDTLS_SSL_DEBUG_MSG(
2, ("record_size_limit extension is still in development. Aborting handshake."));
ssl->session_negotiate->record_size_limit = record_size_limit;

MBEDTLS_SSL_PEND_FATAL_ALERT(
MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT,
MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION);
return MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION;
return 0;
}

#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

#endif /* MBEDTLS_SSL_TLS_C && MBEDTLS_SSL_PROTO_TLS1_3 */
13 changes: 5 additions & 8 deletions library/ssl_tls13_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1699,14 +1699,11 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl,

ret = mbedtls_ssl_tls13_parse_record_size_limit_ext(
ssl, p, extension_data_end);

/*
* TODO: Return unconditionally here until we handle the record
* size limit correctly.
* Once handled correctly, only return in case of errors.
*/
return ret;

if (ret != 0) {
MBEDTLS_SSL_DEBUG_RET(
1, ("mbedtls_ssl_tls13_parse_record_size_limit_ext"), ret);
return ret;
}
break;
#endif /* MBEDTLS_SSL_RECORD_SIZE_LIMIT */

Expand Down
2 changes: 1 addition & 1 deletion programs/ssl/ssl_server2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3520,7 +3520,7 @@ int main(int argc, char *argv[])
mbedtls_printf(" [ Record expansion is unknown ]\n");
}

#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH)
#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) || defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
mbedtls_printf(" [ Maximum incoming record payload length is %u ]\n",
(unsigned int) mbedtls_ssl_get_max_in_record_payload(&ssl));
mbedtls_printf(" [ Maximum outgoing record payload length is %u ]\n",
Expand Down
7 changes: 2 additions & 5 deletions tests/scripts/all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5709,11 +5709,8 @@ component_test_tls13_only_record_size_limit () {
msg "test_suite_ssl: TLS 1.3 only, record size limit extension enabled"
cd tests; ./test_suite_ssl; cd ..

msg "ssl-opt.sh: (TLS 1.3 only, record size limit extension tests only)"
# Both the server and the client will currently abort the handshake when they encounter the
# record size limit extension. There is no way to prevent gnutls-cli from sending the extension
# which makes all G_NEXT_CLI + P_SRV tests fail. Thus, run only the tests for the this extension.
tests/ssl-opt.sh -f "Record Size Limit"
msg "ssl-opt.sh: (TLS 1.3 only, record size limit extension enabled)"
tests/ssl-opt.sh
}

component_build_mingw () {
Expand Down
4 changes: 4 additions & 0 deletions tests/src/test_helpers/ssl_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,6 +1776,10 @@ int mbedtls_test_ssl_tls13_populate_session(mbedtls_ssl_session *session,
}
#endif /* MBEDTLS_SSL_CLI_C */

#if defined(MBEDTLS_SSL_RECORD_SIZE_LIMIT)
session->record_size_limit = 2048;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN > 2048?

Copy link
Contributor

Choose a reason for hiding this comment

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

MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN is defined by RFC 8449 to be equal to 64. We should probably add a comment where MBEDTLS_SSL_RECORD_SIZE_LIMIT_MIN is defined to make that clear.

#endif

return 0;
}
#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */
Expand Down
Loading