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

fix: initial config should not influence sslv2 #4987

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −27
Open

fix: initial config should not influence sslv2 #4987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion tests/unit/s2n_client_hello_recv_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_hello));
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));

EXPECT_EQUAL(server_conn->server_protocol_version, i == 0 ? S2N_TLS12 : S2N_TLS13);
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS12);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

conn->server_protocol_version is set to s2n_highest_protocol_version in s2n_connection_new. s2n_highest_protocol_version is either TLS 1.2 or TLS 1.3, depending on whether s2n_disable/enable_tls13_test functions have been used.

Previously

server_protocol_version was never modified for SSLv2 client hellos. This line was just asserting on s2n_highest_protocol_version which was toggled between test runs by s2n_disable/enable_tls13_test.

Proposed

server_protocol_version is now modified by this section in process_client_hello.

s2n-tls/tls/s2n_client_hello.c

Lines 535 to 538 in cc2ea72

if (!s2n_connection_supports_tls13(conn) || !s2n_security_policy_supports_tls13(security_policy)) {
conn->server_protocol_version = MIN(conn->server_protocol_version, S2N_TLS12);
conn->actual_protocol_version = MIN(conn->server_protocol_version, S2N_TLS12);
}

From my reading/audit of the codebase, server_protocol_version is mostly used for various checks and assertions related to downgrade protection. From the brief audit that I did, I didn't see any places that this would be a noticeable behavior change, but this part of the codebase is pretty new to me and our SSLV2 test coverage is pretty low ☹️

I'll also throw out that server_protocol_version being set to TLS13 while using SSLV2 client hellos seems suspicious given this RFC requirement

Implementations MUST NOT negotiate TLS 1.3 or later using an SSL version 2.0 compatible CLIENT-HELLO.
https://www.rfc-editor.org/rfc/rfc8446#appendix-D.5

Note: I think the code chunk above should just always be assigning S2N_TLS12. When I made that change, all the test cases continued to pass exception one of the renegotiate ones. Need to investigate further

Copy link
Contributor

@lrstewart lrstewart Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

server_protocol_version was never modified for SSLv2 client hellos

That's definitely a bug. Later code is going to assume that server_protocol_version was set properly, regardless of what it's currently used for.

But the TLS1.3 case not working makes sense. SSLv2 ClientHellos don't contain extensions, and the only way to negotiate TLS1.3 is via extension. Even if that wasn't the case, we're using a hard-coded ClientHello so s2n_disable/enable_tls13_test would have no effect on it.

So yes, your fix is correct. But it's arguable whether the two different test setups are even necessary. Yes, s2n_disable/enable_tls13_test would still affect the server itself, but we're only setting the "tls12_config" on the server. I think this test needs a little more attention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just remove s2n_enable/disable_tls13_in_test() for this test? If we're explicitly setting the config then it's not necessary.

EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
EXPECT_EQUAL(server_conn->client_hello_version, S2N_SSLv2);
Expand Down
41 changes: 41 additions & 0 deletions tests/unit/s2n_config_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "tls/s2n_config.h"

#include <stdlib.h>
#include <testlib/s2n_sslv2_client_hello.h>

#include "api/s2n.h"
#include "crypto/s2n_fips.h"
Expand All @@ -27,6 +28,7 @@
#include "tls/s2n_internal.h"
#include "tls/s2n_record.h"
#include "tls/s2n_security_policies.h"
#include "tls/s2n_tls.h"
#include "tls/s2n_tls13.h"
#include "unstable/npn.h"
#include "utils/s2n_map.h"
Expand Down Expand Up @@ -1232,5 +1234,44 @@ int main(int argc, char **argv)
}
}

/* Checks that servers don't use a config before the client hello callback is executed on a
* SSLv2-formatted client hello.
*
* Parsing SSLv2 hellos uses a different code path and need to be tested separately.
*/
{
uint8_t sslv2_client_hello[] = {
SSLv2_CLIENT_HELLO_PREFIX,
SSLv2_CLIENT_HELLO_CIPHER_SUITES,
SSLv2_CLIENT_HELLO_CHALLENGE,
};

struct s2n_blob client_hello = {
.data = sslv2_client_hello,
.size = sizeof(sslv2_client_hello),
.allocated = 0,
.growable = 0
};
Comment on lines +1249 to +1254
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid directly instantiating blobs and stuffers. That bypasses any safety checks / additional initialization logic. I'm guessing you copied from the other test, but our best practices have gotten better ;)

Instead it should probably be:

Suggested change
struct s2n_blob client_hello = {
.data = sslv2_client_hello,
.size = sizeof(sslv2_client_hello),
.allocated = 0,
.growable = 0
};
struct s2n_blob client_hello = { 0 };
EXPECT_SUCCESS(s2n_blob_init(&client_hello, sslv2_client_hello), sizeof(sslv2_client_hello));


/* Checks that the handshake gets as far as the client hello callback with a NULL config */
{
DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
EXPECT_NOT_NULL(server_conn);
server_conn->config = NULL;

/* Record version and protocol version are in the header for SSLv2 */
server_conn->client_hello_version = S2N_SSLv2;
server_conn->client_protocol_version = S2N_TLS12;

/* S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK is only called in one location, just before the
* client hello callback. Therefore, we can assert that if we hit this error, we
* have gotten as far as the client hello callback without dereferencing the config.
*/
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_hello));
EXPECT_FAILURE_WITH_ERRNO(s2n_client_hello_recv(server_conn), S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK);
}
}

END_TEST();
}
34 changes: 10 additions & 24 deletions tls/s2n_client_hello.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ int s2n_parse_client_hello(struct s2n_connection *conn)
}

if (conn->client_hello_version == S2N_SSLv2) {
POSIX_GUARD(s2n_sslv2_client_hello_recv(conn));
POSIX_GUARD(s2n_sslv2_client_hello_parse(conn));
return S2N_SUCCESS;
}

Expand Down Expand Up @@ -594,8 +594,13 @@ int s2n_process_client_hello(struct s2n_connection *conn)
POSIX_CHECKED_MEMCPY(previous_cipher_suite_iana, conn->secure->cipher_suite->iana_value, S2N_TLS_CIPHER_SUITE_LEN);

/* Now choose the ciphers we have certs for. */
POSIX_GUARD(s2n_set_cipher_as_tls_server(conn, client_hello->cipher_suites.data,
client_hello->cipher_suites.size / 2));
if (conn->client_hello_version == S2N_SSLv2) {
POSIX_GUARD(s2n_set_cipher_as_sslv2_server(conn, client_hello->cipher_suites.data,
client_hello->cipher_suites.size / S2N_SSLv2_CIPHER_SUITE_LEN));
} else {
POSIX_GUARD(s2n_set_cipher_as_tls_server(conn, client_hello->cipher_suites.data,
client_hello->cipher_suites.size / 2));
}
Comment on lines +597 to +603
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense for this branching to happen in s2n_set_cipher_as_tls_server?


/* Check if this is the second client hello in a hello retry handshake */
if (s2n_is_hello_retry_handshake(conn) && conn->handshake.message_number > 0) {
Expand Down Expand Up @@ -685,9 +690,7 @@ int s2n_client_hello_recv(struct s2n_connection *conn)
}
}

if (conn->client_hello_version != S2N_SSLv2) {
POSIX_GUARD(s2n_process_client_hello(conn));
}
POSIX_GUARD(s2n_process_client_hello(conn));

return 0;
}
Expand Down Expand Up @@ -821,7 +824,7 @@ int s2n_client_hello_send(struct s2n_connection *conn)
* Alternatively, the TLS1.0 RFC includes a more modern description of the format:
* https://tools.ietf.org/rfc/rfc2246 Appendix E.1
*/
int s2n_sslv2_client_hello_recv(struct s2n_connection *conn)
int s2n_sslv2_client_hello_parse(struct s2n_connection *conn)
{
struct s2n_client_hello *client_hello = &conn->client_hello;
client_hello->sslv2 = true;
Expand All @@ -831,15 +834,6 @@ int s2n_sslv2_client_hello_recv(struct s2n_connection *conn)
POSIX_GUARD(s2n_stuffer_skip_write(&in_stuffer, client_hello->raw_message.size));
struct s2n_stuffer *in = &in_stuffer;

const struct s2n_security_policy *security_policy = NULL;
POSIX_GUARD(s2n_connection_get_security_policy(conn, &security_policy));

if (conn->client_protocol_version < security_policy->minimum_protocol_version) {
POSIX_GUARD(s2n_queue_reader_unsupported_protocol_version_alert(conn));
POSIX_BAIL(S2N_ERR_PROTOCOL_VERSION_UNSUPPORTED);
}
conn->actual_protocol_version = MIN(conn->client_protocol_version, conn->server_protocol_version);

/* We start 5 bytes into the record */
uint16_t cipher_suites_length = 0;
POSIX_GUARD(s2n_stuffer_read_uint16(in, &cipher_suites_length));
Expand All @@ -858,14 +852,6 @@ int s2n_sslv2_client_hello_recv(struct s2n_connection *conn)
client_hello->cipher_suites.data = s2n_stuffer_raw_read(in, cipher_suites_length);
POSIX_ENSURE_REF(client_hello->cipher_suites.data);

/* Find potential certificate matches before we choose the cipher. */
POSIX_GUARD(s2n_conn_find_name_matching_certs(conn));

POSIX_GUARD(s2n_set_cipher_as_sslv2_server(conn, client_hello->cipher_suites.data,
client_hello->cipher_suites.size / S2N_SSLv2_CIPHER_SUITE_LEN));
POSIX_GUARD_RESULT(s2n_signature_algorithm_select(conn));
POSIX_GUARD(s2n_select_certs_for_server_auth(conn, &conn->handshake_params.our_chain_and_key));

S2N_ERROR_IF(session_id_length > s2n_stuffer_data_available(in), S2N_ERR_BAD_MESSAGE);
POSIX_GUARD(s2n_blob_init(&client_hello->session_id, s2n_stuffer_raw_read(in, session_id_length), session_id_length));
if (session_id_length > 0 && session_id_length <= S2N_TLS_SESSION_ID_MAX_LEN) {
Expand Down
2 changes: 1 addition & 1 deletion tls/s2n_record_read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ int s2n_sslv2_record_header_parse(
*
* The protocol version read here will likely not be SSLv2, since we only
* accept SSLv2 ClientHellos offering higher protocol versions.
* See s2n_sslv2_client_hello_recv.
* See s2n_sslv2_client_hello_parse.
*/
uint8_t protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
POSIX_GUARD(s2n_stuffer_read_bytes(header_in, protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));
Expand Down
3 changes: 2 additions & 1 deletion tls/s2n_tls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *more);
S2N_RESULT s2n_client_hello_request_validate(struct s2n_connection *conn);
S2N_RESULT s2n_client_hello_request_recv(struct s2n_connection *conn);
int s2n_client_hello_send(struct s2n_connection *conn);
int s2n_parse_client_hello(struct s2n_connection *conn);
Comment on lines 29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to make this method "public" than you should probably give it a better name: s2n_client_hello_parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something 🤔 ? Why do you need to make this method public at all?

int s2n_client_hello_recv(struct s2n_connection *conn);
int s2n_establish_session(struct s2n_connection *conn);
int s2n_sslv2_client_hello_recv(struct s2n_connection *conn);
int s2n_sslv2_client_hello_parse(struct s2n_connection *conn);
int s2n_server_hello_retry_send(struct s2n_connection *conn);
int s2n_server_hello_retry_recv(struct s2n_connection *conn);
int s2n_server_hello_write_message(struct s2n_connection *conn);
Expand Down
Loading