Improve error reporting for corrupt messages

This commit is contained in:
ComplexSpaces 2023-02-05 16:47:45 -06:00 committed by Dirkjan Ochtman
parent cd4f5c90ca
commit fe94dec981
13 changed files with 141 additions and 68 deletions

View File

@ -67,7 +67,7 @@ If you'd like to help out, please see [CONTRIBUTING.md](CONTRIBUTING.md).
- The minimum supported Rust version is now 1.56.0 due to several dependencies
requiring it.
* 0.20.6 (2022-05-18)
- 0.20.5 included a change to track more context for the `Error::CorruptMessage`
- 0.20.5 included a change to track more context for the `Error::InvalidMessage`
which made API-incompatible changes to the `Error` type. We yanked 0.20.5
and have reverted that change as part of 0.20.6.
* 0.20.5 (2022-05-14)

View File

@ -101,7 +101,7 @@ mod online {
fn too_many_sans() {
connect("10000-sans.badssl.com")
.fails()
.expect(r"TLS error: CorruptMessagePayload\(Handshake\)")
.expect(r"TLS error: InvalidMessage\(HandshakePayloadTooLarge\)")
.go()
.unwrap();
}

View File

@ -9,6 +9,7 @@ use env_logger;
use rustls;
use rustls::internal::msgs::codec::Codec;
use rustls::internal::msgs::enums::KeyUpdateRequest;
use rustls::internal::msgs::persist;
use rustls::quic::{self, ClientQuicExt, QuicExt, ServerQuicExt};
use rustls::server::ClientHello;
@ -592,7 +593,7 @@ fn quit_err(why: &str) -> ! {
fn handle_err(err: rustls::Error) -> ! {
use rustls::{AlertDescription, ContentType};
use rustls::{Error, PeerMisbehaved};
use rustls::{Error, InvalidMessage, PeerMisbehaved};
use std::{thread, time};
println!("TLS error: {:?}", err);
@ -610,13 +611,22 @@ fn handle_err(err: rustls::Error) -> ! {
Error::AlertReceived(AlertDescription::InternalError) => {
quit(":PEER_ALERT_INTERNAL_ERROR:")
}
Error::CorruptMessagePayload(ContentType::Alert) => quit(":BAD_ALERT:"),
Error::CorruptMessagePayload(ContentType::ChangeCipherSpec) => {
Error::InvalidMessage(InvalidMessage::MissingPayload(ContentType::Alert)) => {
quit(":BAD_ALERT:")
}
Error::InvalidMessage(InvalidMessage::MissingPayload(ContentType::ChangeCipherSpec)) => {
quit(":BAD_CHANGE_CIPHER_SPEC:")
}
Error::CorruptMessagePayload(ContentType::Handshake) => quit(":BAD_HANDSHAKE_MSG:"),
Error::CorruptMessagePayload(ContentType::Unknown(42)) => quit(":GARBAGE:"),
Error::CorruptMessage => quit(":GARBAGE:"),
Error::InvalidMessage(InvalidMessage::MissingPayload(ContentType::Handshake)) => {
quit(":BAD_HANDSHAKE_MSG:")
}
Error::InvalidMessage(InvalidMessage::InvalidKeyUpdate(KeyUpdateRequest::Unknown(42))) => {
quit(":BAD_HANDSHAKE_MSG:")
}
Error::InvalidMessage(InvalidMessage::InvalidCertRequest)
| Error::InvalidMessage(InvalidMessage::InvalidDhParams)
| Error::InvalidMessage(InvalidMessage::MissingKeyExchange) => quit(":BAD_HANDSHAKE_MSG:"),
Error::InvalidMessage(_) => quit(":GARBAGE:"),
Error::DecryptError => quit(":DECRYPTION_FAILED_OR_BAD_RECORD_MAC:"),
Error::PeerIncompatible(_) => quit(":INCOMPATIBLE:"),
Error::PeerMisbehaved(PeerMisbehaved::TooMuchEarlyDataReceived) => {
@ -764,7 +774,8 @@ fn exec(opts: &Options, mut sess: Connection, count: usize) {
let mut one_byte = [0u8];
let mut cursor = io::Cursor::new(&mut one_byte[..]);
sess.write_tls(&mut cursor).unwrap();
conn.write(&one_byte).expect("IO error");
conn.write_all(&one_byte)
.expect("IO error");
quench_writes = true;
}

View File

@ -1,8 +1,9 @@
use crate::check::{inappropriate_handshake_message, inappropriate_message};
use crate::conn::{CommonState, ConnectionRandoms, Side, State};
use crate::enums::ProtocolVersion;
use crate::error::{Error, PeerMisbehaved};
use crate::error::{Error, InvalidMessage, PeerMisbehaved};
use crate::hash_hs::HandshakeHash;
use crate::kx;
#[cfg(feature = "logging")]
use crate::log::{debug, trace};
use crate::msgs::base::{Payload, PayloadU8};
@ -22,7 +23,7 @@ use crate::suites::PartiallyExtractedSecrets;
use crate::suites::SupportedCipherSuite;
use crate::ticketer::TimeBase;
use crate::tls12::{self, ConnectionSecrets, Tls12CipherSuite};
use crate::{kx, verify};
use crate::verify;
use super::client_conn::ClientConnectionData;
use super::hs::ClientContext;
@ -405,7 +406,7 @@ impl State<ClientConnectionData> for ExpectServerKx {
.ok_or_else(|| {
cx.common
.send_fatal_alert(AlertDescription::DecodeError);
Error::CorruptMessagePayload(ContentType::Handshake)
InvalidMessage::MissingKeyExchange
})?;
// Save the signature and signed parameters for later verification.

View File

@ -5,7 +5,7 @@ use crate::conn::Protocol;
use crate::conn::Side;
use crate::conn::{CommonState, ConnectionRandoms, State};
use crate::enums::{ProtocolVersion, SignatureScheme};
use crate::error::{Error, PeerIncompatible, PeerMisbehaved};
use crate::error::{Error, InvalidMessage, PeerIncompatible, PeerMisbehaved};
use crate::hash_hs::{HandshakeHash, HandshakeHashBuffer};
use crate::kx;
#[cfg(feature = "logging")]
@ -522,7 +522,7 @@ impl State<ClientConnectionData> for ExpectCertificateRequest {
warn!("Server sent non-empty certreq context");
cx.common
.send_fatal_alert(AlertDescription::DecodeError);
return Err(Error::CorruptMessagePayload(ContentType::Handshake));
return Err(InvalidMessage::InvalidCertRequest.into());
}
let tls13_sign_schemes = sign::supported_sign_tls13();
@ -588,7 +588,7 @@ impl State<ClientConnectionData> for ExpectCertificate {
warn!("certificate with non-empty context during handshake");
cx.common
.send_fatal_alert(AlertDescription::DecodeError);
return Err(Error::CorruptMessagePayload(ContentType::Handshake));
return Err(InvalidMessage::InvalidCertRequest.into());
}
if cert_chain.any_entry_has_duplicate_extension()

View File

@ -1,6 +1,5 @@
use crate::enums::ProtocolVersion;
use crate::error::{Error, PeerMisbehaved};
use crate::key;
#[cfg(feature = "logging")]
use crate::log::{debug, error, trace, warn};
use crate::msgs::alert::AlertMessagePayload;
@ -22,6 +21,7 @@ use crate::suites::{ExtractedSecrets, PartiallyExtractedSecrets};
#[cfg(feature = "tls12")]
use crate::tls12::ConnectionSecrets;
use crate::vecbuf::ChunkVecBuffer;
use crate::{key, InvalidMessage};
#[cfg(feature = "quic")]
use std::collections::VecDeque;
@ -666,7 +666,7 @@ impl<Data> ConnectionCommon<Data> {
Ok(Some(message))
}
Ok(None) => Ok(None),
Err(err @ Error::CorruptMessage | err @ Error::CorruptMessagePayload(_)) => {
Err(err @ Error::InvalidMessage(_)) => {
#[cfg(feature = "quic")]
if self.common_state.is_quic() {
self.common_state.quic.alert = Some(AlertDescription::DecodeError);
@ -1329,7 +1329,7 @@ impl CommonState {
KeyUpdateRequest::UpdateRequested => Ok(self.queued_key_update_message.is_none()),
_ => {
self.send_fatal_alert(AlertDescription::IllegalParameter);
Err(Error::CorruptMessagePayload(ContentType::Handshake))
Err(InvalidMessage::InvalidKeyUpdate(*key_update_request).into())
}
}
}

View File

@ -1,4 +1,4 @@
use crate::msgs::enums::{AlertDescription, ContentType, HandshakeType};
use crate::msgs::enums::{AlertDescription, ContentType, HandshakeType, KeyUpdateRequest};
use crate::rand;
use std::error::Error as StdError;
@ -31,11 +31,8 @@ pub enum Error {
got_type: HandshakeType,
},
/// The peer sent us a syntactically incorrect TLS message.
CorruptMessage,
/// The peer sent us a TLS message with invalid contents.
CorruptMessagePayload(ContentType),
InvalidMessage(InvalidMessage),
/// The peer didn't give us any certificates.
NoCertificatesPresented,
@ -94,6 +91,42 @@ pub enum Error {
BadMaxFragmentSize,
}
/// A corrupt TLS message payload that resulted in an error.
#[non_exhaustive]
#[derive(Debug, Clone, Copy)]
pub enum InvalidMessage {
/// The peer sent us a syntactically incorrect TLS message.
IncorrectFrame,
/// An advertised message was larger then expected.
HandshakePayloadTooLarge,
/// A message was zero-length when its record kind forbids it.
InvalidEmptyPayload,
/// A TLS message payload was larger then allowed by the specification.
MessageTooLarge,
/// An unknown content type was encountered during message decoding.
InvalidContentType,
/// An unknown TLS protocol was encountered during message decoding.
UnknownProtocolVersion,
/// Decoding a message payload for a [ContentType] failed because a different
/// type was encountered.
MissingPayload(ContentType),
/// A peer did not advertise its supported key exchange groups.
MissingKeyExchange,
/// Context was incorrectly attached to a certificate request during a handshake.
InvalidCertRequest,
/// A peer sent an unexpected key update request.
InvalidKeyUpdate(KeyUpdateRequest),
/// A peer's DH params could not be decoded
InvalidDhParams,
}
impl From<InvalidMessage> for Error {
#[inline]
fn from(e: InvalidMessage) -> Self {
Self::InvalidMessage(e)
}
}
#[non_exhaustive]
#[allow(missing_docs)]
#[derive(Debug, PartialEq, Clone)]
@ -298,7 +331,7 @@ impl fmt::Display for Error {
got_type,
join::<HandshakeType>(expect_types)
),
Self::CorruptMessagePayload(ref typ) => {
Self::InvalidMessage(ref typ) => {
write!(f, "received corrupt message of type {:?}", typ)
}
Self::PeerIncompatible(ref why) => write!(f, "peer is incompatible: {:?}", why),
@ -307,7 +340,6 @@ impl fmt::Display for Error {
Self::InvalidCertificate(ref err) => {
write!(f, "invalid peer certificate: {:?}", err)
}
Self::CorruptMessage => write!(f, "received corrupt message"),
Self::NoCertificatesPresented => write!(f, "peer sent no certificates"),
Self::UnsupportedNameType => write!(f, "presented server name type wasn't supported"),
Self::DecryptError => write!(f, "cannot decrypt peer's message"),
@ -343,7 +375,7 @@ impl From<rand::GetRandomFailed> for Error {
#[cfg(test)]
mod tests {
use super::Error;
use super::{Error, InvalidMessage};
#[test]
fn smoke() {
@ -359,8 +391,7 @@ mod tests {
expect_types: vec![HandshakeType::ClientHello, HandshakeType::Finished],
got_type: HandshakeType::ServerHello,
},
Error::CorruptMessage,
Error::CorruptMessagePayload(ContentType::Alert),
Error::InvalidMessage(InvalidMessage::IncorrectFrame),
Error::NoCertificatesPresented,
Error::DecryptError,
super::PeerIncompatible::Tls12NotOffered.into(),

View File

@ -371,7 +371,7 @@ pub use crate::conn::{
CommonState, Connection, ConnectionCommon, IoState, Reader, Side, SideData, Writer,
};
pub use crate::enums::{CipherSuite, ProtocolVersion, SignatureScheme};
pub use crate::error::{CertificateError, Error, PeerIncompatible, PeerMisbehaved};
pub use crate::error::{CertificateError, Error, InvalidMessage, PeerIncompatible, PeerMisbehaved};
pub use crate::key::{Certificate, PrivateKey};
pub use crate::key_log::{KeyLog, NoKeyLog};
pub use crate::key_log_file::KeyLogFile;

View File

@ -8,7 +8,7 @@ use crate::error::{Error, PeerMisbehaved};
use crate::msgs::codec;
use crate::msgs::message::{MessageError, OpaqueMessage};
use crate::record_layer::{Decrypted, RecordLayer};
use crate::ProtocolVersion;
use crate::{InvalidMessage, ProtocolVersion};
/// This deframer works to reconstruct TLS messages from a stream of arbitrary-sized reads.
///
@ -41,7 +41,7 @@ impl MessageDeframer {
/// `Ok(Some(_))` if a valid message was found and decrypted successfully.
pub fn pop(&mut self, record_layer: &mut RecordLayer) -> Result<Option<Deframed>, Error> {
if self.desynced {
return Err(Error::CorruptMessage);
return Err(InvalidMessage::IncorrectFrame.into());
} else if self.used == 0 {
return Ok(None);
}
@ -70,12 +70,22 @@ impl MessageDeframer {
let mut rd = codec::Reader::init(&self.buf[start..self.used]);
let m = match OpaqueMessage::read(&mut rd) {
Ok(m) => m,
Err(MessageError::TooShortForHeader | MessageError::TooShortForLength) => {
return Ok(None)
}
Err(_) => {
Err(msg_err) => {
let err_kind = match msg_err {
MessageError::TooShortForHeader | MessageError::TooShortForLength => {
return Ok(None)
}
MessageError::EmptyDataForDisallowedRecord => {
InvalidMessage::InvalidEmptyPayload
}
MessageError::MessageTooLarge => InvalidMessage::MessageTooLarge,
MessageError::InvalidContentType => InvalidMessage::InvalidContentType,
MessageError::UnknownProtocolVersion => {
InvalidMessage::UnknownProtocolVersion
}
};
self.desynced = true;
return Err(Error::CorruptMessage);
return Err(Error::InvalidMessage(err_kind));
}
};
@ -383,7 +393,7 @@ fn payload_size(buf: &[u8]) -> Result<Option<usize>, Error> {
let (header, _) = buf.split_at(HEADER_SIZE);
match codec::u24::decode(&header[1..]) {
Some(len) if len.0 > MAX_HANDSHAKE_SIZE => {
Err(Error::CorruptMessagePayload(ContentType::Handshake))
Err(InvalidMessage::HandshakePayloadTooLarge.into())
}
Some(len) => Ok(Some(HEADER_SIZE + usize::from(len))),
_ => Ok(None),
@ -417,7 +427,7 @@ mod tests {
use super::MessageDeframer;
use crate::msgs::message::{Message, OpaqueMessage};
use crate::record_layer::RecordLayer;
use crate::{ContentType, Error};
use crate::{ContentType, Error, InvalidMessage};
use std::io;
@ -624,7 +634,10 @@ mod tests {
);
let mut rl = RecordLayer::new();
assert!(matches!(d.pop(&mut rl).unwrap_err(), Error::CorruptMessage));
assert!(matches!(
d.pop(&mut rl).unwrap_err(),
Error::InvalidMessage(InvalidMessage::InvalidContentType)
));
}
#[test]
@ -636,7 +649,10 @@ mod tests {
);
let mut rl = RecordLayer::new();
assert!(matches!(d.pop(&mut rl).unwrap_err(), Error::CorruptMessage));
assert!(matches!(
d.pop(&mut rl).unwrap_err(),
Error::InvalidMessage(InvalidMessage::UnknownProtocolVersion)
));
}
#[test]
@ -648,7 +664,10 @@ mod tests {
);
let mut rl = RecordLayer::new();
assert!(matches!(d.pop(&mut rl).unwrap_err(), Error::CorruptMessage));
assert!(matches!(
d.pop(&mut rl).unwrap_err(),
Error::InvalidMessage(InvalidMessage::MessageTooLarge)
));
}
#[test]
@ -676,9 +695,15 @@ mod tests {
);
let mut rl = RecordLayer::new();
assert!(matches!(d.pop(&mut rl).unwrap_err(), Error::CorruptMessage));
assert!(matches!(
d.pop(&mut rl).unwrap_err(),
Error::InvalidMessage(InvalidMessage::InvalidEmptyPayload)
));
// CorruptMessage has been fused
assert!(matches!(d.pop(&mut rl).unwrap_err(), Error::CorruptMessage));
assert!(matches!(
d.pop(&mut rl).unwrap_err(),
Error::InvalidMessage(InvalidMessage::IncorrectFrame)
));
}
#[test]

View File

@ -6,6 +6,7 @@ use crate::msgs::ccs::ChangeCipherSpecPayload;
use crate::msgs::codec::{Codec, Reader};
use crate::msgs::enums::{AlertDescription, AlertLevel, ContentType, HandshakeType};
use crate::msgs::handshake::HandshakeMessagePayload;
use crate::InvalidMessage;
#[derive(Debug)]
pub enum MessagePayload {
@ -54,7 +55,7 @@ impl MessagePayload {
_ => None,
};
parsed.ok_or(Error::CorruptMessagePayload(typ))
parsed.ok_or(InvalidMessage::MissingPayload(typ).into())
}
pub fn content_type(&self) -> ContentType {
@ -91,23 +92,23 @@ impl OpaqueMessage {
// implemented per section 5.1 of RFC8446 (TLSv1.3)
// per section 6.2.1 of RFC5246 (TLSv1.2)
if typ != ContentType::ApplicationData && len == 0 {
return Err(MessageError::IllegalLength);
return Err(MessageError::EmptyDataForDisallowedRecord);
}
// Reject oversize messages
if len >= Self::MAX_PAYLOAD {
return Err(MessageError::IllegalLength);
return Err(MessageError::MessageTooLarge);
}
// Don't accept any new content-types.
if let ContentType::Unknown(_) = typ {
return Err(MessageError::IllegalContentType);
return Err(MessageError::InvalidContentType);
}
// Accept only versions 0x03XX for any XX.
match version {
ProtocolVersion::Unknown(ref v) if (v & 0xff00) != 0x0300 => {
return Err(MessageError::IllegalProtocolVersion);
return Err(MessageError::UnknownProtocolVersion);
}
_ => {}
};
@ -284,7 +285,8 @@ impl<'a> BorrowedPlainMessage<'a> {
pub enum MessageError {
TooShortForHeader,
TooShortForLength,
IllegalLength,
IllegalContentType,
IllegalProtocolVersion,
EmptyDataForDisallowedRecord,
MessageTooLarge,
InvalidContentType,
UnknownProtocolVersion,
}

View File

@ -1,14 +1,14 @@
use crate::cipher::{MessageDecrypter, MessageEncrypter};
use crate::conn::{CommonState, ConnectionRandoms, Side};
use crate::enums::{CipherSuite, SignatureScheme};
use crate::kx;
use crate::msgs::codec::{Codec, Reader};
use crate::msgs::enums::{AlertDescription, ContentType};
use crate::msgs::enums::AlertDescription;
use crate::msgs::handshake::KeyExchangeAlgorithm;
use crate::suites::{BulkAlgorithm, CipherSuiteCommon, SupportedCipherSuite};
#[cfg(feature = "secret_extraction")]
use crate::suites::{ConnectionTrafficSecrets, PartiallyExtractedSecrets};
use crate::Error;
use crate::{kx, InvalidMessage};
use ring::aead;
use ring::digest::Digest;
@ -500,7 +500,7 @@ pub(crate) fn decode_ecdh_params<T: Codec>(
) -> Result<T, Error> {
decode_ecdh_params_::<T>(kx_params).ok_or_else(|| {
common.send_fatal_alert(AlertDescription::DecodeError);
Error::CorruptMessagePayload(ContentType::Handshake)
Error::InvalidMessage(InvalidMessage::InvalidDhParams)
})
}

View File

@ -13,11 +13,9 @@ use crate::common::{
use rustls::client::WebPkiVerifier;
use rustls::internal::msgs::base::PayloadU16;
use rustls::server::{ClientCertVerified, ClientCertVerifier};
use rustls::AlertDescription;
use rustls::ContentType;
use rustls::{
Certificate, ClientConnection, DistinguishedNames, Error, ServerConfig, ServerConnection,
SignatureScheme,
AlertDescription, Certificate, ClientConnection, ContentType, DistinguishedNames, Error,
InvalidMessage, ServerConfig, ServerConnection, SignatureScheme,
};
use std::sync::Arc;
@ -104,8 +102,8 @@ fn client_verifier_no_schemes() {
let err = do_handshake_until_error(&mut client, &mut server);
assert_debug_eq(
err,
Err(ErrorFromPeer::Client(Error::CorruptMessagePayload(
ContentType::Handshake,
Err(ErrorFromPeer::Client(Error::InvalidMessage(
InvalidMessage::MissingPayload(ContentType::Handshake),
))),
);
}

View File

@ -11,8 +11,7 @@ use rustls::client::{
HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier, WebPkiVerifier,
};
use rustls::internal::msgs::handshake::DigitallySignedStruct;
use rustls::AlertDescription;
use rustls::{Certificate, Error, SignatureScheme};
use rustls::{AlertDescription, Certificate, Error, InvalidMessage, SignatureScheme};
use std::sync::Arc;
#[test]
@ -39,7 +38,7 @@ fn client_can_override_certificate_verification() {
fn client_can_override_certificate_verification_and_reject_certificate() {
for kt in ALL_KEY_TYPES.iter() {
let verifier = Arc::new(MockServerVerifier::rejects_certificate(
Error::CorruptMessage,
Error::InvalidMessage(InvalidMessage::HandshakePayloadTooLarge),
));
let server_config = Arc::new(make_server_config(*kt));
@ -56,7 +55,9 @@ fn client_can_override_certificate_verification_and_reject_certificate() {
assert_debug_eq(
errs,
Err(vec![
ErrorFromPeer::Client(Error::CorruptMessage),
ErrorFromPeer::Client(Error::InvalidMessage(
InvalidMessage::HandshakePayloadTooLarge,
)),
ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)),
]),
);
@ -70,7 +71,7 @@ fn client_can_override_certificate_verification_and_reject_tls12_signatures() {
for kt in ALL_KEY_TYPES.iter() {
let mut client_config = make_client_config_with_versions(*kt, &[&rustls::version::TLS12]);
let verifier = Arc::new(MockServerVerifier::rejects_tls12_signatures(
Error::CorruptMessage,
Error::InvalidMessage(InvalidMessage::HandshakePayloadTooLarge),
));
client_config
@ -85,7 +86,9 @@ fn client_can_override_certificate_verification_and_reject_tls12_signatures() {
assert_debug_eq(
errs,
Err(vec![
ErrorFromPeer::Client(Error::CorruptMessage),
ErrorFromPeer::Client(Error::InvalidMessage(
InvalidMessage::HandshakePayloadTooLarge,
)),
ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)),
]),
);
@ -97,7 +100,7 @@ fn client_can_override_certificate_verification_and_reject_tls13_signatures() {
for kt in ALL_KEY_TYPES.iter() {
let mut client_config = make_client_config_with_versions(*kt, &[&rustls::version::TLS13]);
let verifier = Arc::new(MockServerVerifier::rejects_tls13_signatures(
Error::CorruptMessage,
Error::InvalidMessage(InvalidMessage::HandshakePayloadTooLarge),
));
client_config
@ -112,7 +115,9 @@ fn client_can_override_certificate_verification_and_reject_tls13_signatures() {
assert_debug_eq(
errs,
Err(vec![
ErrorFromPeer::Client(Error::CorruptMessage),
ErrorFromPeer::Client(Error::InvalidMessage(
InvalidMessage::HandshakePayloadTooLarge,
)),
ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)),
]),
);