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

Jump to bottom

Trying to use an invalid ticket should not mutate state #4110

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Trying to use an invalid ticket should not mutate state #4110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c59a04
Trying to use an invalid ticket should not mutate state
lrstewart Jul 20, 2023
f1d38f6
Address maddeleine@'s PR comments
lrstewart Jul 25, 2023
29787c0
Update docs
lrstewart Jul 25, 2023
a13b13d
Merge branch 'main' into resume
dougch Jul 25, 2023
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
242 changes: 182 additions & 60 deletions tests/unit/s2n_resume_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,66 @@ int main(int argc, char **argv)
"18df06843d13a08bf2a449844c5f8a"
"478001bc4d4c627984d5a41da8d0402919");

uint8_t tls12_ticket[S2N_TLS12_STATE_SIZE_IN_BYTES_WITHOUT_EMS] = {
S2N_SERIALIZED_FORMAT_TLS12_V1,
S2N_TLS12,
TLS_RSA_WITH_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
};

uint8_t tls12_ticket_with_ems[S2N_TLS12_STATE_SIZE_IN_BYTES] = {
S2N_SERIALIZED_FORMAT_TLS12_V3,
S2N_TLS12,
TLS_RSA_WITH_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
};

uint8_t tls13_ticket[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
EMPTY_EARLY_DATA_SIZE,
};

uint8_t tls13_server_ticket[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
KEYING_MATERIAL_EXPIRATION_BYTES,
EMPTY_EARLY_DATA_SIZE,
};

uint8_t tls13_ticket_with_early_data[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
0x00,
0x00,
0x00,
NONEMPTY_EARLY_DATA_SIZE,
APP_PROTOCOL_LEN,
APP_PROTOCOL,
0x00,
EARLY_DATA_CONTEXT_LEN,
EARLY_DATA_CONTEXT,
};

uint8_t faulty_format_ticket[] = {
0xFF,
};

/* s2n_connection_get_session_state_size */
{
/* Safety */
Expand Down Expand Up @@ -585,66 +645,6 @@ int main(int argc, char **argv)

/* s2n_deserialize_resumption_state */
{
uint8_t tls12_ticket[S2N_TLS12_STATE_SIZE_IN_BYTES_WITHOUT_EMS] = {
S2N_SERIALIZED_FORMAT_TLS12_V1,
S2N_TLS12,
TLS_RSA_WITH_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
};

uint8_t tls12_ticket_with_ems[S2N_TLS12_STATE_SIZE_IN_BYTES] = {
S2N_SERIALIZED_FORMAT_TLS12_V3,
S2N_TLS12,
TLS_RSA_WITH_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
};

uint8_t tls13_ticket[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
EMPTY_EARLY_DATA_SIZE,
};

uint8_t tls13_server_ticket[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
KEYING_MATERIAL_EXPIRATION_BYTES,
EMPTY_EARLY_DATA_SIZE,
};

uint8_t tls13_ticket_with_early_data[] = {
S2N_SERIALIZED_FORMAT_TLS13_V1,
S2N_TLS13,
TLS_AES_128_GCM_SHA256,
TICKET_ISSUE_TIME_BYTES,
TICKET_AGE_ADD_BYTES,
SECRET_LEN,
SECRET,
0x00,
0x00,
0x00,
NONEMPTY_EARLY_DATA_SIZE,
APP_PROTOCOL_LEN,
APP_PROTOCOL,
0x00,
EARLY_DATA_CONTEXT_LEN,
EARLY_DATA_CONTEXT,
};

uint8_t faulty_format_ticket[] = {
0xFF,
};

/* Deserialize ticket with incorrect format errors */
{
struct s2n_blob ticket_blob = { 0 };
Expand Down Expand Up @@ -1618,5 +1618,127 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_config_free(config));
};

/* Test s2n_connection_set_session */
{
uint8_t server_state[] = "encrypted state";

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
EXPECT_SUCCESS(s2n_config_set_wall_clock(config, mock_time, NULL));
EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "test_all"));

/* Invalid TLS1.2 tickets should not modify the connection.
*
* This basically tests that deserialization errors aren't fatal / unrecoverable.
*/
{
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
DEFER_CLEANUP(struct s2n_stuffer ticket_stuffer = { 0 }, s2n_stuffer_free);
EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&ticket_stuffer, 0));
EXPECT_SUCCESS(s2n_stuffer_write_uint8(&ticket_stuffer, S2N_STATE_WITH_SESSION_TICKET));
EXPECT_SUCCESS(s2n_stuffer_write_uint16(&ticket_stuffer, sizeof(server_state)));
EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, server_state, sizeof(server_state)));
EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer,
tls12_ticket_with_ems, sizeof(tls12_ticket_with_ems)));

size_t ticket_size = s2n_stuffer_data_available(&ticket_stuffer);
uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size);
EXPECT_NOT_NULL(ticket_bytes);

DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

/* Test that deserialize modifies the connection in limited ways.
*
* No mechanism exists to do more than a shallow comparison of two connections.
* To prove that a shallow comparison is sufficient, we need to prove
* that s2n_deserialize_resumption_state does not modify the memory
* associated with pointers on the connection. To prove that, we can
* test that s2n_deserialize_resumption_state can successfully operate
* on an s2n_connection with a limited number of its pointers initialized.
*/
struct s2n_connection empty_conn = { 0 };
struct s2n_crypto_parameters crypto_params = { .cipher_suite = &s2n_null_cipher_suite };
empty_conn.secure = &crypto_params;
empty_conn.mode = S2N_CLIENT;
/* We can safely assume that a connection doesn't modify its config */
empty_conn.config = conn->config;
EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size));
EXPECT_SUCCESS(s2n_free(&empty_conn.client_ticket));

/* Trigger the deserialization failure as late as possible.
* The last byte is optional, so drop the last two bytes.
*/
ticket_size -= 2;

/* Test the connection is not modified by a failed deserialize */
uint8_t saved_conn[sizeof(struct s2n_connection)] = { 0 };
EXPECT_MEMCPY_SUCCESS(saved_conn, conn, sizeof(struct s2n_connection));
uint8_t saved_secure[sizeof(struct s2n_crypto_parameters)] = { 0 };
EXPECT_MEMCPY_SUCCESS(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters));
EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, ticket_size),
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
S2N_ERR_STUFFER_OUT_OF_DATA);
EXPECT_BYTEARRAY_EQUAL(saved_conn, conn, sizeof(struct s2n_connection));
EXPECT_BYTEARRAY_EQUAL(saved_secure, conn->secure, sizeof(struct s2n_crypto_parameters));

/* No valid ticket */
EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0);
};

/* Invalid TLS1.3 tickets should not modify the connection.
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
*
* This basically tests that deserialization errors aren't fatal / unrecoverable.
*/
{
DEFER_CLEANUP(struct s2n_stuffer ticket_stuffer = { 0 }, s2n_stuffer_free);
EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&ticket_stuffer, 0));
EXPECT_SUCCESS(s2n_stuffer_write_uint8(&ticket_stuffer, S2N_STATE_WITH_SESSION_TICKET));
EXPECT_SUCCESS(s2n_stuffer_write_uint16(&ticket_stuffer, sizeof(server_state)));
EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, server_state, sizeof(server_state)));
EXPECT_SUCCESS(s2n_stuffer_write_bytes(&ticket_stuffer, tls13_ticket, sizeof(tls13_ticket)));

size_t ticket_size = s2n_stuffer_data_available(&ticket_stuffer);
uint8_t *ticket_bytes = s2n_stuffer_raw_read(&ticket_stuffer, ticket_size);
EXPECT_NOT_NULL(ticket_bytes);

DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

/* Test that the connection is only shallowly modified by a successful deserialize.
*
* No mechanism exists to do more than a shallow comparison of two connections.
* To prove that a shallow comparison is sufficient, we need to prove
* that s2n_deserialize_resumption_state does not modify the memory
* associated with pointers on the connection. To prove that, we can
* test that s2n_deserialize_resumption_state can successfully operate
* on an s2n_connection with none of its pointers initialized.
*/
struct s2n_connection empty_conn = { 0 };
empty_conn.mode = S2N_CLIENT;
/* We can safely assume that a connection doesn't modify its config */
empty_conn.config = conn->config;
EXPECT_SUCCESS(s2n_connection_set_session(&empty_conn, ticket_bytes, ticket_size));
EXPECT_OK(s2n_psk_parameters_wipe(&empty_conn.psk_params));

/* Trigger the deserialization failure as late as possible in parsing.
* Drop the last byte we expect.
*/
size_t wrong_ticket_size = ticket_size - 1;

/* Test the connection is not modified by a failed deserialize */
uint8_t saved[sizeof(struct s2n_connection)] = { 0 };
EXPECT_MEMCPY_SUCCESS(saved, conn, sizeof(struct s2n_connection));
EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_session(conn, ticket_bytes, wrong_ticket_size),
S2N_ERR_STUFFER_OUT_OF_DATA);
EXPECT_BYTEARRAY_EQUAL(saved, conn, sizeof(struct s2n_connection));

/* No valid ticket */
EXPECT_EQUAL(s2n_connection_get_session_length(conn), 0);
};
};

END_TEST();
}
60 changes: 41 additions & 19 deletions tls/s2n_resume.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,27 +228,53 @@ static int s2n_client_serialize_resumption_state(struct s2n_connection *conn, st
return 0;
}

static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connection *conn, struct s2n_stuffer *from)
static S2N_RESULT s2n_tls12_client_deserialize_session_state(struct s2n_connection *conn,
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
struct s2n_blob *ticket, struct s2n_stuffer *from)
{
RESULT_ENSURE_REF(conn);
RESULT_ENSURE_REF(from);

maddeleine marked this conversation as resolved.
Show resolved Hide resolved
RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &conn->resume_protocol_version));
/* Operate on a copy of the connection to avoid mutating the connection on
* failure. We have tests in s2n_resume_test.c that prove this level of copy
* is sufficient.
*/
struct s2n_crypto_parameters *secure = conn->secure;
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
RESULT_ENSURE_REF(secure);
struct s2n_connection resume_conn = *conn;
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
struct s2n_crypto_parameters resume_secure = *secure;
resume_conn.secure = &resume_secure;

RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &resume_conn.resume_protocol_version));

uint8_t *cipher_suite_wire = s2n_stuffer_raw_read(from, S2N_TLS_CIPHER_SUITE_LEN);
RESULT_ENSURE_REF(cipher_suite_wire);
RESULT_GUARD_POSIX(s2n_set_cipher_as_client(conn, cipher_suite_wire));
RESULT_GUARD_POSIX(s2n_set_cipher_as_client(&resume_conn, cipher_suite_wire));

uint64_t then = 0;
RESULT_GUARD_POSIX(s2n_stuffer_read_uint64(from, &then));

RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(from, conn->secrets.version.tls12.master_secret, S2N_TLS_SECRET_LEN));
RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(from, resume_conn.secrets.version.tls12.master_secret,
S2N_TLS_SECRET_LEN));

if (s2n_stuffer_data_available(from)) {
uint8_t ems_negotiated = 0;
RESULT_GUARD_POSIX(s2n_stuffer_read_uint8(from, &ems_negotiated));
conn->ems_negotiated = ems_negotiated;
resume_conn.ems_negotiated = ems_negotiated;
}

DEFER_CLEANUP(struct s2n_blob client_ticket = { 0 }, s2n_free);
if (ticket) {
RESULT_GUARD_POSIX(s2n_dup(ticket, &client_ticket));
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
}

/* Finally, actually update the connection */
RESULT_GUARD_POSIX(s2n_free(&conn->client_ticket));
*secure = resume_secure;
*conn = resume_conn;
conn->secure = secure;
conn->client_ticket = client_ticket;
ZERO_TO_DISABLE_DEFER_CLEANUP(client_ticket);

return S2N_RESULT_OK;
}

Expand Down Expand Up @@ -337,7 +363,8 @@ static S2N_RESULT s2n_tls13_deserialize_session_state(struct s2n_connection *con
return S2N_RESULT_OK;
}

S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn, struct s2n_blob *psk_identity, struct s2n_stuffer *from)
S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn,
struct s2n_blob *ticket, struct s2n_stuffer *from)
{
RESULT_ENSURE_REF(conn);
RESULT_ENSURE_REF(from);
Expand All @@ -349,16 +376,10 @@ S2N_RESULT s2n_deserialize_resumption_state(struct s2n_connection *conn, struct
if (conn->mode == S2N_SERVER) {
RESULT_GUARD_POSIX(s2n_tls12_deserialize_resumption_state(conn, from));
} else {
RESULT_GUARD(s2n_tls12_client_deserialize_session_state(conn, from));
RESULT_GUARD(s2n_tls12_client_deserialize_session_state(conn, ticket, from));
}
} else if (format == S2N_SERIALIZED_FORMAT_TLS13_V1) {
RESULT_GUARD(s2n_tls13_deserialize_session_state(conn, psk_identity, from));
if (conn->mode == S2N_CLIENT) {
/* Free the client_ticket after setting a psk on the connection.
* This prevents s2n_connection_get_session from returning a TLS1.3
* ticket before a ticket has been received from the server. */
RESULT_GUARD_POSIX(s2n_free(&conn->client_ticket));
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
}
RESULT_GUARD(s2n_tls13_deserialize_session_state(conn, ticket, from));
} else {
RESULT_BAIL(S2N_ERR_INVALID_SERIALIZED_SESSION_STATE);
}
Expand Down Expand Up @@ -386,18 +407,19 @@ static int s2n_client_deserialize_with_session_id(struct s2n_connection *conn, s

static int s2n_client_deserialize_with_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *from)
{
uint16_t session_ticket_len;
uint16_t session_ticket_len = 0;
POSIX_GUARD(s2n_stuffer_read_uint16(from, &session_ticket_len));

if (session_ticket_len == 0 || session_ticket_len > s2n_stuffer_data_available(from)) {
POSIX_BAIL(S2N_ERR_INVALID_SERIALIZED_SESSION_STATE);
}

POSIX_GUARD(s2n_realloc(&conn->client_ticket, session_ticket_len));
POSIX_GUARD(s2n_stuffer_read(from, &conn->client_ticket));

POSIX_GUARD_RESULT(s2n_deserialize_resumption_state(conn, &conn->client_ticket, from));
struct s2n_blob session_ticket = { 0 };
uint8_t *session_ticket_bytes = s2n_stuffer_raw_read(from, session_ticket_len);
POSIX_ENSURE_REF(session_ticket_bytes);
POSIX_GUARD(s2n_blob_init(&session_ticket, session_ticket_bytes, session_ticket_len));

POSIX_GUARD_RESULT(s2n_deserialize_resumption_state(conn, &session_ticket, from));
return 0;
}

Expand Down
2 changes: 2 additions & 0 deletions utils/s2n_mem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ int s2n_free_object(uint8_t **p_data, uint32_t size)
int s2n_dup(struct s2n_blob *from, struct s2n_blob *to)
{
POSIX_ENSURE(initialized, S2N_ERR_NOT_INITIALIZED);
POSIX_ENSURE_REF(to);
POSIX_ENSURE_REF(from);
POSIX_ENSURE_EQ(to->size, 0);
POSIX_ENSURE_EQ(to->data, NULL);
POSIX_ENSURE_NE(from->size, 0);
Expand Down
Loading