Skip to content

Fix name compressors emitting wrong pointers past 14-bit offset limit #707

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

Open
SebastiaanYN wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix name compressors emitting wrong pointers past 14-bit offset limit #707

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
59 changes: 52 additions & 7 deletions src/base/message_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1963,6 +1963,10 @@ where
}
}

/// Compression pointers encode the offset in 14 bits, so positions at or
/// above this limit cannot be referenced.
const MAX_COMPRESSION_OFFSET: usize = 1 << 14;

//------------ StaticCompressor ----------------------------------------------

/// A domain name compressor that doesn’t require an allocator.
Expand Down Expand Up @@ -2050,7 +2054,7 @@ impl<Target> StaticCompressor<Target> {

/// Inserts the position of a new domain name if possible.
fn insert(&mut self, pos: usize) -> bool {
if pos < 0xc000 && self.len < self.entries.len() {
if pos < MAX_COMPRESSION_OFFSET && self.len < self.entries.len() {
self.entries[self.len] = pos as u16;
self.len += 1;
true
Expand Down Expand Up @@ -2133,7 +2137,7 @@ impl<Target: Composer> Composer for StaticCompressor<Target> {
impl<Target: Truncate> Truncate for StaticCompressor<Target> {
fn truncate(&mut self, len: usize) {
self.target.truncate(len);
if len < 0xC000 {
if len < MAX_COMPRESSION_OFFSET {
let len = len as u16;
for i in 0..self.len {
if self.entries[i] >= len {
Expand Down Expand Up @@ -2264,7 +2268,7 @@ impl<Target> TreeCompressor<Target> {
name: N,
pos: usize,
) -> bool {
if pos >= 0xC000 {
if pos >= MAX_COMPRESSION_OFFSET {
return false;
}
let pos = pos as u16;
Expand Down Expand Up @@ -2361,7 +2365,7 @@ impl<Target: Composer> Composer for TreeCompressor<Target> {
impl<Target: Composer> Truncate for TreeCompressor<Target> {
fn truncate(&mut self, len: usize) {
self.target.truncate(len);
if len < 0xC000 {
if len < MAX_COMPRESSION_OFFSET {
self.start.drop_above(len as u16)
}
}
Expand Down Expand Up @@ -2457,7 +2461,7 @@ struct HashEntry {
impl HashEntry {
/// Try constructing a [`HashEntry`].
fn new(head: usize, tail: usize) -> Option<Self> {
if head < 0xC000 {
if head < MAX_COMPRESSION_OFFSET {
Some(Self {
head: head as u16,
tail: tail as u16,
Expand Down Expand Up @@ -2600,7 +2604,7 @@ impl<Target: Composer> Composer for HashCompressor<Target> {

// Remember this label for future compression, if possible.
//
// If some labels in this name pass the 0xC000 boundary point, then
// If some labels in this name pass the 14-bit boundary, then
// none of its remembered labels can be used (since they are looked
// up from right to left, and the rightmost ones will fail first).
// We could check more thoroughly for this, but it's not worth it.
Expand Down Expand Up @@ -2636,7 +2640,7 @@ impl<Target: Composer> Composer for HashCompressor<Target> {
impl<Target: Composer> Truncate for HashCompressor<Target> {
fn truncate(&mut self, len: usize) {
self.target.truncate(len);
if len < 0xC000 {
if len < MAX_COMPRESSION_OFFSET {
self.names.retain(|name| name.head < len as u16);
}
}
Expand Down Expand Up @@ -2694,6 +2698,8 @@ impl core::error::Error for PushError {}
#[cfg(test)]
#[cfg(feature = "alloc")]
mod test {
use rstest::rstest;

use super::*;
use crate::base::iana::Rtype;
use crate::base::opt;
Expand Down Expand Up @@ -2955,6 +2961,45 @@ mod test {
assert_eq!(expect[..], actual, "unexpected response data");
}

#[rstest]
#[case::static_compressor(StaticCompressor::new(Vec::new()))]
#[case::tree_compressor(TreeCompressor::new(Vec::new()))]
#[cfg_attr(
feature = "std",
case::hash_compressor(HashCompressor::new(Vec::new()))
)]
fn compress_past_14bit_limit<T>(#[case] target: T)
where
T: Composer + FreezeBuilder,
T::AppendError: fmt::Debug,
T::Octets: Octets,
{
use crate::rdata::{AllRecordData, Txt};

let pad = "pad.example.com.".parse::<Name<Vec<u8>>>().unwrap();
let txt = Txt::<Vec<u8>>::build_from_slice(&[0u8; 16_400]).unwrap();
let name = "a.b.c.".parse::<Name<Vec<u8>>>().unwrap();

let mut msg = MessageBuilder::from_target(target).unwrap().answer();
msg.push((&pad, 0, &txt)).unwrap();
msg.push((&name, 0, A::from_octets(1, 2, 3, 4))).unwrap();
msg.push((&name, 0, A::from_octets(5, 6, 7, 8))).unwrap();
let msg = msg.into_message();

let records = msg
.answer()
.unwrap()
.into_records::<AllRecordData<_, _>>()
.collect::<Result<Vec<_>, _>>()
.unwrap();
assert!(
records[1].owner().name_eq(records[2].owner()),
"compression pointer corrupted: record[2] owner {:?} != {:?}",
records[2].owner(),
records[1].owner(),
);
}

#[cfg(feature = "std")]
#[test]
fn hash_compress_positive_response() {
Expand Down