fix(proto): reject invalid echo packet lengths
This commit is contained in:
5 files changed
+68
-6
No files matched your search
@@ -1,7 +1,7 @@
|
|||||||
use crate::{
|
use crate::{
|
||||||
flags::{has, FLAG_HMAC, FLAG_OPEN, FLAG_REPLY},
|
flags::{has, FLAG_HMAC, FLAG_OPEN, FLAG_REPLY},
|
||||||
hmac,
|
hmac,
|
||||||
layout::{echo_packet_len, PacketLayout},
|
layout::{try_echo_packet_len, PacketLayout},
|
||||||
open::{check_hmac_presence, reject},
|
open::{check_hmac_presence, reject},
|
||||||
params::Params,
|
params::Params,
|
||||||
validate_header, write_header, ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE,
|
validate_header, write_header, ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE,
|
||||||
@@ -43,7 +43,7 @@ pub fn encode_echo_request(request: &EchoRequest, hmac_key: Option<&[u8]>) -> Re
|
|||||||
flags |= FLAG_HMAC;
|
flags |= FLAG_HMAC;
|
||||||
}
|
}
|
||||||
let layout = PacketLayout::echo(hmac_key.is_some(), &request.params);
|
let layout = PacketLayout::echo(hmac_key.is_some(), &request.params);
|
||||||
let len = echo_packet_len(hmac_key.is_some(), &request.params);
|
let len = try_echo_packet_len(hmac_key.is_some(), &request.params)?;
|
||||||
let payload_offset = layout.header_len();
|
let payload_offset = layout.header_len();
|
||||||
let available_payload_len = len.saturating_sub(payload_offset);
|
let available_payload_len = len.saturating_sub(payload_offset);
|
||||||
if request.payload.len() > available_payload_len {
|
if request.payload.len() > available_payload_len {
|
||||||
@@ -231,6 +231,17 @@ mod tests {
|
|||||||
assert_eq!(request, original);
|
assert_eq!(request, original);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn echo_request_rejects_negative_requested_length() {
|
||||||
|
let mut request = echo_request_with_payload(0, Vec::new());
|
||||||
|
request.params.length = -1;
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
encode_echo_request(&request, None),
|
||||||
|
Err(ProtoError::NegativePacketLength { length: -1 })
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn hmac_echo_request_places_token_and_sequence_after_hmac() {
|
fn hmac_echo_request_places_token_and_sequence_after_hmac() {
|
||||||
let packet = encode_echo_request(
|
let packet = encode_echo_request(
|
||||||
|
|||||||
@@ -28,6 +28,10 @@ pub enum ProtoError {
|
|||||||
InvalidUtf8,
|
InvalidUtf8,
|
||||||
#[error("invalid enum value {value} for {name}")]
|
#[error("invalid enum value {value} for {name}")]
|
||||||
InvalidEnum { name: &'static str, value: i64 },
|
InvalidEnum { name: &'static str, value: i64 },
|
||||||
|
#[error("packet length must be non-negative, got {length}")]
|
||||||
|
NegativePacketLength { length: i64 },
|
||||||
|
#[error("parameter {tag} length is too large: {length} bytes")]
|
||||||
|
ParameterLengthTooLarge { tag: u64, length: u64 },
|
||||||
#[error("open reply has zero token without close flag")]
|
#[error("open reply has zero token without close flag")]
|
||||||
ZeroToken,
|
ZeroToken,
|
||||||
#[error("trailing or malformed parameter payload")]
|
#[error("trailing or malformed parameter payload")]
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
use crate::{
|
use crate::{
|
||||||
params::{Params, StampAt},
|
params::{Params, StampAt},
|
||||||
HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE,
|
ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE,
|
||||||
TOKEN_SIZE,
|
TIMESTAMP_SIZE, TOKEN_SIZE,
|
||||||
};
|
};
|
||||||
|
|
||||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
@@ -117,6 +117,15 @@ pub fn echo_packet_len(hmac: bool, params: &Params) -> usize {
|
|||||||
header_len.max(requested)
|
header_len.max(requested)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub fn try_echo_packet_len(hmac: bool, params: &Params) -> Result<usize> {
|
||||||
|
let header_len = echo_header_len(hmac, params);
|
||||||
|
let requested =
|
||||||
|
usize::try_from(params.length).map_err(|_| ProtoError::NegativePacketLength {
|
||||||
|
length: params.length,
|
||||||
|
})?;
|
||||||
|
Ok(header_len.max(requested))
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
@@ -295,4 +304,25 @@ mod tests {
|
|||||||
p.length = 92;
|
p.length = 92;
|
||||||
assert_eq!(echo_packet_len(false, &p), 92);
|
assert_eq!(echo_packet_len(false, &p), 92);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn checked_packet_len_rejects_negative_requested_length() {
|
||||||
|
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
|
||||||
|
p.length = -1;
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
try_echo_packet_len(false, &p),
|
||||||
|
Err(ProtoError::NegativePacketLength { length: -1 })
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn checked_packet_len_preserves_non_negative_header_floor() {
|
||||||
|
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
|
||||||
|
p.length = 0;
|
||||||
|
assert_eq!(try_echo_packet_len(false, &p), Ok(60));
|
||||||
|
|
||||||
|
p.length = 92;
|
||||||
|
assert_eq!(try_echo_packet_len(false, &p), Ok(92));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -15,7 +15,7 @@ pub use echo::{decode_echo_reply, encode_echo_request, EchoReply, EchoRequest, T
|
|||||||
pub use error::{ProtoError, Result};
|
pub use error::{ProtoError, Result};
|
||||||
pub use flags::*;
|
pub use flags::*;
|
||||||
pub use hmac::{compute_hmac, compute_hmac_in_place, verify_hmac};
|
pub use hmac::{compute_hmac, compute_hmac_in_place, verify_hmac};
|
||||||
pub use layout::{echo_header_len, echo_packet_len, PacketLayout};
|
pub use layout::{echo_header_len, echo_packet_len, try_echo_packet_len, PacketLayout};
|
||||||
pub use open::{decode_open_reply, encode_open_request, OpenReply, OpenRequest};
|
pub use open::{decode_open_reply, encode_open_request, OpenReply, OpenRequest};
|
||||||
pub use params::{Clock, Params, ReceivedStats, ServerFill, StampAt};
|
pub use params::{Clock, Params, ReceivedStats, ServerFill, StampAt};
|
||||||
|
|
||||||
|
|||||||
@@ -57,7 +57,8 @@ impl Params {
|
|||||||
9 => {
|
9 => {
|
||||||
let (len, used) = varint::decode_uvarint(&input[pos..])?;
|
let (len, used) = varint::decode_uvarint(&input[pos..])?;
|
||||||
pos += used;
|
pos += used;
|
||||||
let len = len as usize;
|
let len = usize::try_from(len)
|
||||||
|
.map_err(|_| ProtoError::ParameterLengthTooLarge { tag, length: len })?;
|
||||||
if input.len().saturating_sub(pos) < len {
|
if input.len().saturating_sub(pos) < len {
|
||||||
return Err(ProtoError::MalformedParams);
|
return Err(ProtoError::MalformedParams);
|
||||||
}
|
}
|
||||||
@@ -406,6 +407,22 @@ mod tests {
|
|||||||
assert_eq!(Params::decode(&encoded), Err(ProtoError::InvalidUtf8));
|
assert_eq!(Params::decode(&encoded), Err(ProtoError::InvalidUtf8));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(target_pointer_width = "32")]
|
||||||
|
#[test]
|
||||||
|
fn server_fill_length_too_large_for_usize_is_rejected() {
|
||||||
|
let mut encoded = Vec::new();
|
||||||
|
varint::encode_uvarint(9, &mut encoded);
|
||||||
|
varint::encode_uvarint(u64::from(u32::MAX) + 1, &mut encoded);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
Params::decode(&encoded),
|
||||||
|
Err(ProtoError::ParameterLengthTooLarge {
|
||||||
|
tag: 9,
|
||||||
|
length: u64::from(u32::MAX) + 1,
|
||||||
|
})
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn truncated_varint_parameter_is_rejected() {
|
fn truncated_varint_parameter_is_rejected() {
|
||||||
let encoded = [1, 0x80];
|
let encoded = [1, 0x80];
|
||||||
|
|||||||
Reference in new issue
Block a user