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.

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

Domain name compression is broken in 0.10.0 #321

Merged
merged 2 commits into from
May 30, 2024

Conversation

hunts
Copy link
Contributor

@hunts hunts commented May 29, 2024

There is a bug in SliceLabelsIter's next() implement that is causing it not able to return the full/right labels in the octets slice.

StaticCompressor leverages SliceLabelsIter to search known domain names in the underlying octets, is not able to find existing names because of this. Thus, leading to broken domain name compression in 0.10.0.

@hunts
Copy link
Contributor Author

hunts commented May 29, 2024

I see Label has a compose_len() function, but it is trying to return a u16, meanwhile there are label.len() + 1 in several places in the codebase. It may worth a new issue to update all of these, but I'm going for label.len() + 1 for now.

hunts added 2 commits May 29, 2024 15:25
The `Iterator` implementation on SliceLabelsIter was not advancing offset
with the a leading length octet.
Add a test case where owner name in the A record is compressed.
@hunts hunts force-pushed the fix-name-compression branch from 7b64a96 to 44f4218 Compare May 29, 2024 22:25
@partim
Copy link
Member

partim commented May 30, 2024

Thank you for the PR!

This actually used to use compose_len – I probably broke it when changing that function to return a u16.

@partim partim merged commit 7ebfcf3 into NLnetLabs:main May 30, 2024
12 checks passed
@hunts hunts deleted the fix-name-compression branch May 30, 2024 16:42
partim added a commit that referenced this pull request Jun 3, 2024
New

* Allow AllRecordData’s parsing impls to accept an unsized [u8] as the
  source octets. ([#310] by [@xofyarg])
* Made `sign::records::FamilyName` public. ([#312] by [@achow101])
* Added an impl of `FromStr` for `Question`. ([#317])

Bug fixes

* Accept an empty record type bitmap when scanning NSEC/NSEC3 data.
  ([#310] by [@xofyarg])
* Fix serialization of ProtoRrsig to conform with RFC 4034. ([#313 by
  [@achow101])
* Add `?Sized` bounds to `Message::is_answer` and `ParsedRecord::to_record`.
  ([#318] by [@xofyarg], [#325] by [@hunts])
* Bring back `MessageBuilder::as_target`. ([#318] by [@xofyarg])
* Bring back `impl FreezeBuilder for StaticCompressor`. ([#318] by [@xofyarg])
* `sign::records::RecordsIter::skip_before` now stops at the first name in
  zone even if the apex itself doesn’t appear. ([#314] by [@achow101])
* Fix a counting error in `SliceLabelsIter::next` that broke compression
  via `StaticCompressor`. ([#321] by [@hunts])

Unstable features

* New unstable feature `unstable-stelline` for the Stelline testing
  framework as a “normal” module of _domain._ ([#315])
* Renamed the domain name types in `zonetree` from `Dname` to `Name`.
  ([#308])

Other changes

* The minimum Rust version is now 1.78. ([#320])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants