mirror of https://github.com/ctz/rustls
Prefer `supported_groups` extension in kx group choice
Prior to this, we preferred to avoid a `HelloRetryRequest` when any supported `KeyShare` was supplied. But as [1] describes, this means a client which sends a `KeyShare` for a less-preferred group would end up using that, rather than a more-preferred group supported by both peers. [1]: https://www.ietf.org/archive/id/draft-davidben-tls-key-share-prediction-00.html#name-downgrades
This commit is contained in:
Joseph Birr-Pixton
Joe Birr-Pixton
parent
e178646af5
commit
d23e58c3db
|
|
@ -197,75 +197,16 @@ mod client_hello {
|
|||
});
|
||||
}
|
||||
|
||||
// choose a share that we support
|
||||
let chosen_share_and_kxg = self
|
||||
// Choose the most preferred common group.
|
||||
let mutually_preferred_group = match self
|
||||
.config
|
||||
.provider
|
||||
.kx_groups
|
||||
.iter()
|
||||
.find_map(|group| {
|
||||
shares_ext
|
||||
.iter()
|
||||
.find(|share| share.group == group.name())
|
||||
.map(|share| (share, *group))
|
||||
});
|
||||
|
||||
let chosen_share_and_kxg = match chosen_share_and_kxg {
|
||||
Some(s) => s,
|
||||
.find(|group| groups_ext.contains(&group.name()))
|
||||
{
|
||||
Some(group) => *group,
|
||||
None => {
|
||||
// We don't have a suitable key share. Choose a suitable group and
|
||||
// send a HelloRetryRequest.
|
||||
let retry_group_maybe = self
|
||||
.config
|
||||
.provider
|
||||
.kx_groups
|
||||
.iter()
|
||||
.find(|group| groups_ext.contains(&group.name()))
|
||||
.cloned();
|
||||
|
||||
self.transcript.add_message(chm);
|
||||
|
||||
if let Some(group) = retry_group_maybe {
|
||||
if self.done_retry {
|
||||
return Err(cx.common.send_fatal_alert(
|
||||
AlertDescription::IllegalParameter,
|
||||
PeerMisbehaved::RefusedToFollowHelloRetryRequest,
|
||||
));
|
||||
}
|
||||
|
||||
emit_hello_retry_request(
|
||||
&mut self.transcript,
|
||||
self.suite,
|
||||
client_hello.session_id,
|
||||
cx.common,
|
||||
group.name(),
|
||||
);
|
||||
emit_fake_ccs(cx.common);
|
||||
|
||||
let skip_early_data = max_early_data_size(self.config.max_early_data_size);
|
||||
|
||||
let next = Box::new(hs::ExpectClientHello {
|
||||
config: self.config,
|
||||
transcript: HandshakeHashOrBuffer::Hash(self.transcript),
|
||||
#[cfg(feature = "tls12")]
|
||||
session_id: SessionId::empty(),
|
||||
#[cfg(feature = "tls12")]
|
||||
using_ems: false,
|
||||
done_retry: true,
|
||||
send_tickets: self.send_tickets,
|
||||
extra_exts: self.extra_exts,
|
||||
});
|
||||
|
||||
return if early_data_requested {
|
||||
Ok(Box::new(ExpectAndSkipRejectedEarlyData {
|
||||
skip_data_left: skip_early_data,
|
||||
next,
|
||||
}))
|
||||
} else {
|
||||
Ok(next)
|
||||
};
|
||||
}
|
||||
|
||||
return Err(cx.common.send_fatal_alert(
|
||||
AlertDescription::HandshakeFailure,
|
||||
PeerIncompatible::NoKxGroupsInCommon,
|
||||
|
|
@ -273,6 +214,60 @@ mod client_hello {
|
|||
}
|
||||
};
|
||||
|
||||
// See if there is a KeyShare for that group.
|
||||
let chosen_share_and_kxg = shares_ext.iter().find_map(|share| {
|
||||
(share.group == mutually_preferred_group.name())
|
||||
.then(|| (share, mutually_preferred_group))
|
||||
});
|
||||
|
||||
let chosen_share_and_kxg = match chosen_share_and_kxg {
|
||||
Some(s) => s,
|
||||
None => {
|
||||
// We don't have a suitable key share. Send a HelloRetryRequest
|
||||
// for the mutually_preferred_group.
|
||||
self.transcript.add_message(chm);
|
||||
|
||||
if self.done_retry {
|
||||
return Err(cx.common.send_fatal_alert(
|
||||
AlertDescription::IllegalParameter,
|
||||
PeerMisbehaved::RefusedToFollowHelloRetryRequest,
|
||||
));
|
||||
}
|
||||
|
||||
emit_hello_retry_request(
|
||||
&mut self.transcript,
|
||||
self.suite,
|
||||
client_hello.session_id,
|
||||
cx.common,
|
||||
mutually_preferred_group.name(),
|
||||
);
|
||||
emit_fake_ccs(cx.common);
|
||||
|
||||
let skip_early_data = max_early_data_size(self.config.max_early_data_size);
|
||||
|
||||
let next = Box::new(hs::ExpectClientHello {
|
||||
config: self.config,
|
||||
transcript: HandshakeHashOrBuffer::Hash(self.transcript),
|
||||
#[cfg(feature = "tls12")]
|
||||
session_id: SessionId::empty(),
|
||||
#[cfg(feature = "tls12")]
|
||||
using_ems: false,
|
||||
done_retry: true,
|
||||
send_tickets: self.send_tickets,
|
||||
extra_exts: self.extra_exts,
|
||||
});
|
||||
|
||||
return if early_data_requested {
|
||||
Ok(Box::new(ExpectAndSkipRejectedEarlyData {
|
||||
skip_data_left: skip_early_data,
|
||||
next,
|
||||
}))
|
||||
} else {
|
||||
Ok(next)
|
||||
};
|
||||
}
|
||||
};
|
||||
|
||||
let mut chosen_psk_index = None;
|
||||
let mut resumedata = None;
|
||||
|
||||
|
|
|
|||
|
|
@ -4743,6 +4743,92 @@ fn test_client_attempts_to_use_unsupported_kx_group() {
|
|||
));
|
||||
}
|
||||
|
||||
#[cfg(feature = "tls12")]
|
||||
#[test]
|
||||
fn test_client_sends_share_for_less_preferred_group() {
|
||||
// this is a test for the case described in:
|
||||
// https://datatracker.ietf.org/doc/draft-davidben-tls-key-share-prediction/
|
||||
|
||||
// common to both client configs
|
||||
let shared_storage = Arc::new(ClientStorage::new());
|
||||
|
||||
// first, client sends a secp384r1 share and server agrees. secp384r1 is inserted
|
||||
// into kx group cache.
|
||||
let mut client_config_1 =
|
||||
make_client_config_with_kx_groups(KeyType::Rsa, vec![provider::kx_group::SECP384R1]);
|
||||
client_config_1.resumption = Resumption::store(shared_storage.clone());
|
||||
|
||||
// second, client supports (x25519, secp384r1) and so kx group cache
|
||||
// contains a supported but less-preferred group.
|
||||
let mut client_config_2 = make_client_config_with_kx_groups(
|
||||
KeyType::Rsa,
|
||||
vec![provider::kx_group::X25519, provider::kx_group::SECP384R1],
|
||||
);
|
||||
client_config_2.resumption = Resumption::store(shared_storage.clone());
|
||||
|
||||
let server_config = make_server_config(KeyType::Rsa);
|
||||
|
||||
// first handshake
|
||||
let (mut client_1, mut server) = make_pair_for_configs(client_config_1, server_config.clone());
|
||||
do_handshake_until_error(&mut client_1, &mut server).unwrap();
|
||||
|
||||
let ops = shared_storage.ops();
|
||||
println!("storage {:#?}", ops);
|
||||
assert_eq!(ops.len(), 9);
|
||||
assert!(matches!(
|
||||
ops[3],
|
||||
ClientStorageOp::SetKxHint(_, rustls::NamedGroup::secp384r1)
|
||||
));
|
||||
|
||||
// second handshake (this must HRR to the most-preferred group)
|
||||
let assert_client_sends_secp384_share = |msg: &mut Message| -> Altered {
|
||||
match &msg.payload {
|
||||
MessagePayload::Handshake { parsed, .. } => match &parsed.payload {
|
||||
HandshakePayload::ClientHello(ch) => {
|
||||
let keyshares = ch
|
||||
.keyshare_extension()
|
||||
.expect("missing key share extension");
|
||||
assert_eq!(keyshares.len(), 1);
|
||||
assert_eq!(keyshares[0].group(), rustls::NamedGroup::secp384r1);
|
||||
}
|
||||
_ => panic!("unexpected handshake message {:?}", parsed),
|
||||
},
|
||||
_ => panic!("unexpected non-handshake message {:?}", msg),
|
||||
};
|
||||
Altered::InPlace
|
||||
};
|
||||
|
||||
let assert_server_requests_retry_to_x25519 = |msg: &mut Message| -> Altered {
|
||||
match &msg.payload {
|
||||
MessagePayload::Handshake { parsed, .. } => match &parsed.payload {
|
||||
HandshakePayload::HelloRetryRequest(hrr) => {
|
||||
let group = hrr.requested_key_share_group();
|
||||
assert_eq!(group, Some(rustls::NamedGroup::X25519));
|
||||
}
|
||||
_ => panic!("unexpected handshake message {:?}", parsed),
|
||||
},
|
||||
MessagePayload::ChangeCipherSpec(_) => (),
|
||||
_ => panic!("unexpected non-handshake message {:?}", msg),
|
||||
};
|
||||
Altered::InPlace
|
||||
};
|
||||
|
||||
let (client_2, server) = make_pair_for_configs(client_config_2, server_config);
|
||||
let (mut client_2, mut server) = (client_2.into(), server.into());
|
||||
transfer_altered(
|
||||
&mut client_2,
|
||||
assert_client_sends_secp384_share,
|
||||
&mut server,
|
||||
);
|
||||
server.process_new_packets().unwrap();
|
||||
transfer_altered(
|
||||
&mut server,
|
||||
assert_server_requests_retry_to_x25519,
|
||||
&mut client_2,
|
||||
);
|
||||
client_2.process_new_packets().unwrap();
|
||||
}
|
||||
|
||||
#[cfg(feature = "tls12")]
|
||||
#[test]
|
||||
fn test_tls13_client_resumption_does_not_reuse_tickets() {
|
||||
|
|
|
|||
Loading…
Reference in New Issue