Skip to content

Commit 4220a4d

Browse files
goatgoosegoatgoose
authored
fix: Wipe conn->in on all record parse failures (#4499)
1 parent 171c96a commit 4220a4d

File tree

6 files changed

+73
-20
lines changed

6 files changed

+73
-20
lines changed

‎tests/sidetrail/working/s2n-record-read-cbc-negative-test/record_read_cbc.patch

+9-9
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
diff --git a/tls/s2n_record_read_cbc.c b/tls/s2n_record_read_cbc.c
2-
index bb06523..60a2eed 100644
2+
index 346e6571..0f2527e9 100644
33
--- a/tls/s2n_record_read_cbc.c
44
+++ b/tls/s2n_record_read_cbc.c
5-
@@ -30,6 +30,8 @@
6-
#include "utils/s2n_safety.h"
5+
@@ -26,6 +26,8 @@
76
#include "utils/s2n_blob.h"
7+
#include "utils/s2n_safety.h"
88

99
+extern int g_padding_length;
1010
+
1111
int s2n_record_parse_cbc(
12-
const struct s2n_cipher_suite *cipher_suite,
13-
struct s2n_connection *conn,
14-
@@ -86,6 +88,8 @@ int s2n_record_parse_cbc(
12+
const struct s2n_cipher_suite *cipher_suite,
13+
struct s2n_connection *conn,
14+
@@ -82,6 +84,8 @@ int s2n_record_parse_cbc(
1515

1616
/* Subtract the padding length */
1717
POSIX_ENSURE_GT(en.size, 0);
@@ -20,11 +20,11 @@ index bb06523..60a2eed 100644
2020
uint32_t out = 0;
2121
POSIX_GUARD(s2n_sub_overflow(payload_length, en.data[en.size - 1] + 1, &out));
2222
payload_length = out;
23-
@@ -107,6 +111,7 @@ int s2n_record_parse_cbc(
23+
@@ -103,6 +107,7 @@ int s2n_record_parse_cbc(
2424

25-
/* Padding */
25+
/* Padding. This finalizes the provided HMAC. */
2626
if (s2n_verify_cbc(conn, mac, &en) < 0) {
2727
+ __VERIFIER_assume(0);
28-
POSIX_GUARD(s2n_stuffer_wipe(&conn->in));
2928
POSIX_BAIL(S2N_ERR_BAD_MESSAGE);
3029
}
30+

‎tests/sidetrail/working/s2n-record-read-cbc/record_read_cbc.patch

+9-9
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
diff --git a/tls/s2n_record_read_cbc.c b/tls/s2n_record_read_cbc.c
2-
index bb06523..60a2eed 100644
2+
index 346e6571..0f2527e9 100644
33
--- a/tls/s2n_record_read_cbc.c
44
+++ b/tls/s2n_record_read_cbc.c
5-
@@ -30,6 +30,8 @@
6-
#include "utils/s2n_safety.h"
5+
@@ -26,6 +26,8 @@
76
#include "utils/s2n_blob.h"
7+
#include "utils/s2n_safety.h"
88

99
+extern int g_padding_length;
1010
+
1111
int s2n_record_parse_cbc(
12-
const struct s2n_cipher_suite *cipher_suite,
13-
struct s2n_connection *conn,
14-
@@ -86,6 +88,8 @@ int s2n_record_parse_cbc(
12+
const struct s2n_cipher_suite *cipher_suite,
13+
struct s2n_connection *conn,
14+
@@ -82,6 +84,8 @@ int s2n_record_parse_cbc(
1515

1616
/* Subtract the padding length */
1717
POSIX_ENSURE_GT(en.size, 0);
@@ -20,11 +20,11 @@ index bb06523..60a2eed 100644
2020
uint32_t out = 0;
2121
POSIX_GUARD(s2n_sub_overflow(payload_length, en.data[en.size - 1] + 1, &out));
2222
payload_length = out;
23-
@@ -107,6 +111,7 @@ int s2n_record_parse_cbc(
23+
@@ -103,6 +107,7 @@ int s2n_record_parse_cbc(
2424

25-
/* Padding */
25+
/* Padding. This finalizes the provided HMAC. */
2626
if (s2n_verify_cbc(conn, mac, &en) < 0) {
2727
+ __VERIFIER_assume(0);
28-
POSIX_GUARD(s2n_stuffer_wipe(&conn->in));
2928
POSIX_BAIL(S2N_ERR_BAD_MESSAGE);
3029
}
30+

‎tests/unit/s2n_record_read_test.c

+52
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ int main(int argc, char *argv[])
2525
{
2626
BEGIN_TEST();
2727

28+
DEFER_CLEANUP(struct s2n_cert_chain_and_key *chain_and_key = NULL,
29+
s2n_cert_chain_and_key_ptr_free);
30+
EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&chain_and_key,
31+
S2N_DEFAULT_TEST_CERT_CHAIN, S2N_DEFAULT_TEST_PRIVATE_KEY));
32+
2833
/* Test s2n_sslv2_record_header_parse */
2934
{
3035
const struct {
@@ -159,5 +164,52 @@ int main(int argc, char *argv[])
159164
};
160165
};
161166

167+
/* Ensure that the input buffer is wiped after failing to read a record */
168+
{
169+
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free);
170+
EXPECT_NOT_NULL(config);
171+
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key));
172+
EXPECT_SUCCESS(s2n_config_disable_x509_verification(config));
173+
174+
DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
175+
s2n_connection_ptr_free);
176+
EXPECT_NOT_NULL(client);
177+
DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
178+
s2n_connection_ptr_free);
179+
EXPECT_NOT_NULL(server);
180+
181+
EXPECT_SUCCESS(s2n_connection_set_config(client, config));
182+
EXPECT_SUCCESS(s2n_connection_set_config(server, config));
183+
184+
EXPECT_SUCCESS(s2n_connection_set_blinding(server, S2N_SELF_SERVICE_BLINDING));
185+
186+
DEFER_CLEANUP(struct s2n_test_io_stuffer_pair stuffer_pair = { 0 },
187+
s2n_io_stuffer_pair_free);
188+
EXPECT_OK(s2n_io_stuffer_pair_init(&stuffer_pair));
189+
EXPECT_OK(s2n_connections_set_io_stuffer_pair(client, server, &stuffer_pair));
190+
191+
EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));
192+
193+
/* Send some test data to the server. */
194+
uint8_t test_data[] = "hello world";
195+
s2n_blocked_status blocked = S2N_NOT_BLOCKED;
196+
ssize_t send_size = s2n_send(client, test_data, sizeof(test_data), &blocked);
197+
EXPECT_EQUAL(send_size, sizeof(test_data));
198+
199+
/* Invalidate an encrypted byte to cause decryption to fail. */
200+
struct s2n_stuffer invalidation_stuffer = stuffer_pair.server_in;
201+
uint8_t *first_byte = s2n_stuffer_raw_read(&invalidation_stuffer, 1);
202+
EXPECT_NOT_NULL(first_byte);
203+
*first_byte += 1;
204+
205+
/* Receive the invalid data. */
206+
uint8_t buffer[sizeof(test_data)] = { 0 };
207+
ssize_t ret = s2n_recv(server, buffer, sizeof(buffer), &blocked);
208+
EXPECT_FAILURE_WITH_ERRNO(ret, S2N_ERR_DECRYPT);
209+
210+
/* Ensure that the invalid data has been wiped from the input buffer. */
211+
EXPECT_TRUE(s2n_stuffer_is_wiped(&server->in));
212+
}
213+
162214
END_TEST();
163215
}

‎tls/s2n_connection.c

+3
Original file line numberDiff line numberDiff line change
@@ -1224,6 +1224,9 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
12241224
return S2N_RESULT_OK;
12251225
}
12261226

1227+
/* Ensure that conn->in doesn't contain any leftover invalid or unauthenticated data. */
1228+
RESULT_GUARD_POSIX(s2n_stuffer_wipe(&(*conn)->in));
1229+
12271230
int error_code = s2n_errno;
12281231
int error_type = s2n_error_get_type(error_code);
12291232

‎tls/s2n_record_read_cbc.c

-1
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ int s2n_record_parse_cbc(
103103

104104
/* Padding. This finalizes the provided HMAC. */
105105
if (s2n_verify_cbc(conn, mac, &en) < 0) {
106-
POSIX_GUARD(s2n_stuffer_wipe(&conn->in));
107106
POSIX_BAIL(S2N_ERR_BAD_MESSAGE);
108107
}
109108

‎tls/s2n_record_read_stream.c

-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@ int s2n_record_parse_stream(
7676
POSIX_GUARD(s2n_hmac_digest(mac, check_digest, mac_digest_size));
7777

7878
if (s2n_hmac_digest_verify(en.data + payload_length, check_digest, mac_digest_size) < 0) {
79-
POSIX_GUARD(s2n_stuffer_wipe(&conn->in));
8079
POSIX_BAIL(S2N_ERR_BAD_MESSAGE);
8180
}
8281

0 commit comments

Comments
 (0)