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

Don't ask for an invalid group in an HRR #21163

Closed
wants to merge 4 commits into from
Closed

Don't ask for an invalid group in an HRR #21163

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68270f1
Don't ask for an invalid group in an HRR
mattcaswell Jun 9, 2023
fc26f41
Add a test for an invalid group in the HRR
mattcaswell Jun 9, 2023
35afe2a
fixup! Add a test for an invalid group in the HRR
mattcaswell Jun 9, 2023
f3e3a8a
fixup! Add a test for an invalid group in the HRR
mattcaswell Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion ssl/statem/extensions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,7 +1449,11 @@ static int final_key_share(SSL_CONNECTION *s, unsigned int context, int sent)
group_id = pgroups[i];

if (check_in_list(s, group_id, clntgroups, clnt_num_groups,
1))
1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does line-length allow for this to be put with the previous line?

Copy link
Member Author

Choose a reason for hiding this comment

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

No - line-length prevents this

&& tls_group_allowed(s, group_id,
SSL_SECOP_CURVE_SUPPORTED)
&& tls_valid_group(s, group_id, TLS1_3_VERSION,
TLS1_3_VERSION, 0, NULL))
break;
}

Expand Down
42 changes: 35 additions & 7 deletions test/recipes/70-test_tls13hrr.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ my $proxy = TLSProxy::Proxy->new(
use constant {
CHANGE_HRR_CIPHERSUITE => 0,
CHANGE_CH1_CIPHERSUITE => 1,
DUPLICATE_HRR => 2
DUPLICATE_HRR => 2,
INVALID_GROUP => 3
};

#Test 1: A client should fail if the server changes the ciphersuite between the
Expand All @@ -49,7 +50,7 @@ if (disabled("ec")) {
}
my $testtype = CHANGE_HRR_CIPHERSUITE;
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
plan tests => 3;
plan tests => 4;
ok(TLSProxy::Message->fail(), "Server ciphersuite changes");

#Test 2: It is an error if the client changes the offered ciphersuites so that
Expand Down Expand Up @@ -78,6 +79,24 @@ $testtype = DUPLICATE_HRR;
$proxy->start();
ok($fatal_alert, "Server duplicated HRR");

#Test 4: If the client sends a group that is in the supported_groups list but
# otherwise not valid (e.g. not suitable for TLSv1.3) we should reject it
# and not consider it when sending the HRR. We send brainpoolP512r1 in
# the ClientHello, which is acceptable to the server but is not valid in
# TLSv1.3. We expect the server to select X25519 in the HRR and the
# handshake to complete successfully
SKIP: {
skip "EC/TLSv1.2 is disabled in this build", 1
if disabled("ec") || disabled("tls1_2");

$proxy->clear();
$proxy->clientflags("-groups P-256:brainpoolP512r1:X25519");
$proxy->serverflags("-groups brainpoolP512r1:X25519");
$testtype = INVALID_GROUP;
$proxy->start();
ok(TLSProxy::Message->success(), "Invalid group with HRR");
}

sub hrr_filter
{
my $proxy = shift;
Expand Down Expand Up @@ -131,16 +150,25 @@ sub hrr_filter
return;
}

# CHANGE_CH1_CIPHERSUITE
if ($proxy->flight != 0) {
return;
}

my $ch1 = ${$proxy->message_list}[0];

# The server will always pick TLS_AES_256_GCM_SHA384
my @ciphersuites = (TLSProxy::Message::CIPHER_TLS13_AES_128_GCM_SHA256);
$ch1->ciphersuite_len(2 * scalar @ciphersuites);
$ch1->ciphersuites(\@ciphersuites);
if ($testtype == CHANGE_CH1_CIPHERSUITE) {
# The server will always pick TLS_AES_256_GCM_SHA384
my @ciphersuites = (TLSProxy::Message::CIPHER_TLS13_AES_128_GCM_SHA256);
$ch1->ciphersuite_len(2 * scalar @ciphersuites);
$ch1->ciphersuites(\@ciphersuites);
} else {
mattcaswell marked this conversation as resolved.
Show resolved Hide resolved
# INVALID_GROUP
my $ext = pack "C7",
0x00, 0x05, #List Length
0x00, 0x1c, #brainpoolP512r1 (not compatible with TLSv1.3)
0x00, 0x01, 0xff; #key_exchange data
$ch1->set_extension(
TLSProxy::Message::EXT_KEY_SHARE, $ext);
}
$ch1->repack();
}