Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 18 additions & 4 deletions .github/workflows/tls_codec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,23 @@ jobs:
- uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ matrix.rust }}
targets: ${{ matrix.target }}
targets: ${{ matrix.targets }}
- run: ${{ matrix.deps }}
- uses: RustCrypto/actions/cargo-hack-install@master
- run: cargo hack test --feature-powerset
- run: cargo hack test -p tls_codec_derive --feature-powerset --test encode\* --test decode\*
- run: cargo hack test -p tls_codec_derive --feature-powerset --doc
- run: cargo hack test --target ${{ matrix.targets }} --feature-powerset
- run: cargo hack test --target ${{ matrix.targets }} -p tls_codec_derive --feature-powerset --test encode\* --test decode\*
- run: cargo hack test --target ${{ matrix.targets }} -p tls_codec_derive --feature-powerset --doc
- run: cargo test --target ${{ matrix.targets }} --benches

fuzz:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
- uses: dtolnay/rust-toolchain@nightly
- uses: taiki-e/install-action@v2
with:
tool: cargo-fuzz
- run: |
for fuzz_target in inverse string deserialize bytes_inverse; do
cargo fuzz run --target x86_64-unknown-linux-gnu "$fuzz_target" -- -max_total_time=5
done
41 changes: 38 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions tls_codec/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## 0.4.3

- [#2351](https://github.com/RustCrypto/formats/pull/2351): Implement `Size`, `SerializeBytes`, and `DeserializeBytes` for `String` (and `Size`/`SerializeBytes` for `str` / `&str`), encoding the UTF-8 bytes as a `VLByteVec`. Also implement `SerializeBytes` for `ContentLength` and `VLByteSlice`.
- [#2322](https://github.com/RustCrypto/formats/pull/2322): Add `VLByteVec` and `SecretVLByteVec`, which are `#[serde(transparent)]` wrappers serializing via `serde_bytes`. They produce a much more compact representation in `serde` formats that distinguish byte arrays from sequences of `u8` (e.g. CBOR, MessagePack, bincode). Their `serde` output is not compatible with `VLBytes` / `SecretVLBytes`, but their `Deserialize` impls are backwards-compatible: in self-describing `serde` formats they also accept the legacy `VLBytes` / `SecretVLBytes` encoding (a struct with a `vec` field containing a sequence of `u8`). Deprecate `VLBytes` and `SecretVLBytes` in favour of `VLByteVec` and `SecretVLByteVec`.
- [#1656](https://github.com/RustCrypto/formats/pull/1656) Add `TlsVarInt` type for variable-length integers.

### Fixed
- [#2348](https://github.com/RustCrypto/formats/pull/2348) Use `write_all` everywhere instead of write to prevent partial writes from going undetected. The `Error::InvalidWriteLength` variant is deprecated as it is no longer returned.
- [#XXXX](https://github.com/RustCrypto/formats/pull/XXXX) Element-vector deserialization (`Vec<T>`, `TlsVecU*<T>`, `SecretTlsVecU*<T>`), for both the `Deserialize` (`std::io::Read`) and `DeserializeBytes` implementations, now measures actual byte consumption instead of relying on `tls_serialized_len()` and enforces the declared length exactly. This makes the two implementations agree for non-canonical inner encodings (e.g. non-minimal varint lengths) and rejects input whose elements overshoot the declared vector boundary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [#XXXX](https://github.com/RustCrypto/formats/pull/XXXX) Element-vector deserialization (`Vec<T>`, `TlsVecU*<T>`, `SecretTlsVecU*<T>`), for both the `Deserialize` (`std::io::Read`) and `DeserializeBytes` implementations, now measures actual byte consumption instead of relying on `tls_serialized_len()` and enforces the declared length exactly. This makes the two implementations agree for non-canonical inner encodings (e.g. non-minimal varint lengths) and rejects input whose elements overshoot the declared vector boundary.
- [#2365](https://github.com/RustCrypto/formats/pull/2365) Element-vector deserialization (`Vec<T>`, `TlsVecU*<T>`, `SecretTlsVecU*<T>`), for both the `Deserialize` (`std::io::Read`) and `DeserializeBytes` implementations, now measures actual byte consumption instead of relying on `tls_serialized_len()` and enforces the declared length exactly. This makes the two implementations agree for non-canonical inner encodings (e.g. non-minimal varint lengths) and rejects input whose elements overshoot the declared vector boundary.

- Element vectors now reject zero-length elements that would otherwise never advance the read cursor, preventing an infinite loop and unbounded allocation on malicious input.
- Byte-vector deserialization (`TlsByteVecU*`, `VLBytes`, `VLByteVec`) now uses `checked_add` when computing the content range, returning `Error::InvalidVectorLength` instead of overflowing `usize` on targets where the length field is as wide as the pointer width.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked_add() is not yet used in the Deserialize impls for VLBytes and VLByteVec, right?

- Read-based byte-vector deserialization (`TlsByteVecU*`, `VLBytes`, `VLByteVec`) no longer eagerly allocates a buffer sized by the untrusted length field, avoiding large allocations from bogus length prefixes.
- Fixed swapped doc comments on the `Deserializable*` / `Undeserializable*` type aliases generated by `#[conditionally_deserializable]`.
- Avoid potential overflows on summing up lengths.

## 0.4.2

Expand Down
10 changes: 5 additions & 5 deletions tls_codec/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "tls_codec"
version = "0.4.2"
version = "0.4.3-pre.1"
authors = ["RustCrypto Developers"]
license = "Apache-2.0 OR MIT"
documentation = "https://docs.rs/tls_codec/"
Expand All @@ -12,13 +12,13 @@ edition = "2024"
rust-version = "1.85"

[dependencies]
zeroize = { version = "1.8", default-features = false, features = ["alloc"] }
zeroize = { version = "1.9", default-features = false, features = ["alloc"] }

# optional dependencies
arbitrary = { version = "1.4", features = ["derive"], optional = true }
tls_codec_derive = { version = "=0.4.2", path = "./derive", optional = true }
serde = { version = "1.0.184", features = ["derive"], optional = true }
serde_bytes = { version = "0.11.17", optional = true }
tls_codec_derive = { version = "=0.4.3-pre.1", path = "./derive", optional = true }
serde = { version = "1.0.228", features = ["derive"], optional = true }
serde_bytes = { version = "0.11.19", optional = true }

[dev-dependencies]
criterion = { version = "0.6", default-features = false }
Expand Down
37 changes: 36 additions & 1 deletion tls_codec/benches/tls_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,41 @@ fn byte_vector(c: &mut Criterion) {
TlsByteSliceU32(&long_vec).tls_serialize_detached().unwrap()
},
|serialized_long_vec| {
TlsVecU32::<u8>::tls_deserialize(&mut serialized_long_vec.as_slice()).unwrap()
// Decode into the byte-vector type so this exercises the
// bulk `read_bytes_bounded` path, not the generic per-element
// loop (`TlsVecU32`).
TlsByteVecU32::tls_deserialize(&mut serialized_long_vec.as_slice()).unwrap()
},
BatchSize::SmallInput,
)
});
}

/// Benchmarks the generic per-element deserialize loop with a multi-byte
/// element type, so the loop's per-element bookkeeping isn't hidden behind
/// trivial `u8` decoding.
fn typed_vector(c: &mut Criterion) {
use tls_codec::*;
c.bench_function("TLS Serialize Typed Vector", |b| {
b.iter_batched_ref(
|| {
(
TlsVecU32::from(vec![0x7777u16; N]),
Vec::with_capacity(8 + 2 * N),
)
},
|(long_vec, buf)| long_vec.tls_serialize(buf).unwrap(),
BatchSize::SmallInput,
)
});
c.bench_function("TLS Deserialize Typed Vector", |b| {
b.iter_batched_ref(
|| {
let long_vec = vec![0x7777u16; N];
TlsSliceU32(&long_vec).tls_serialize_detached().unwrap()
},
|serialized_long_vec| {
TlsVecU32::<u16>::tls_deserialize(&mut serialized_long_vec.as_slice()).unwrap()
},
BatchSize::SmallInput,
)
Expand Down Expand Up @@ -78,6 +112,7 @@ fn slice(c: &mut Criterion) {
}
fn benchmark(c: &mut Criterion) {
vector(c);
typed_vector(c);
slice(c);
byte_vector(c);
byte_slice(c);
Expand Down
2 changes: 1 addition & 1 deletion tls_codec/derive/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "tls_codec_derive"
version = "0.4.2"
version = "0.4.3-pre.1"
authors = ["RustCrypto Developers"]
license = "Apache-2.0 OR MIT"
documentation = "https://docs.rs/tls_codec_derive/"
Expand Down
64 changes: 56 additions & 8 deletions tls_codec/derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
//! function returns an `Error::UnknownValue` with a `u64` value of the unknown
//! type.
//!
//! ```
//! ```no_run
//! # #[cfg(feature = "std")]
//! # {
//! use tls_codec_derive::{TlsDeserialize, TlsSerialize, TlsSize};
Expand Down Expand Up @@ -761,6 +761,35 @@ fn define_discriminant_constants(
Ok(quote! { #(#discriminant_constants)* })
}

/// Builds an expression that sums `base` and all `terms` into a `usize`,
/// guarding against overflow only on targets where it can actually occur.
///
/// On 64-bit targets every length is bounded by addressable memory
/// (`isize::MAX`), so the sum can't overflow `usize`; a plain, branch-free
/// addition is emitted to keep the serialization hot path free of checks. On
/// narrower targets the serialized form of a large structure can carry enough
/// length-prefix / discriminant overhead to exceed `usize::MAX`, so we saturate
/// rather than silently wrapping to a small value (a wrapped length would be
/// written to the wire as a truncated, mismatched length prefix).
Comment on lines +770 to +773

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to panic in the derive macro instead, since saturating could cause an incorrectly short length to be written. This would be caught by a debug assertion in impl_serialize(), but otherwise the length does not appear to be checked

///
/// This mirrors `tls_codec::len_add` but is emitted inline so the helper does
/// not need to be part of `tls_codec`'s public API.
fn sum_lengths(terms: &[TokenStream2], base: TokenStream2) -> TokenStream2 {
quote! {
{
#[cfg(target_pointer_width = "64")]
let __tls_len: usize = #base #(+ #terms)*;
#[cfg(not(target_pointer_width = "64"))]
let __tls_len: usize = {
let mut __tls_len: usize = #base;
#(__tls_len = __tls_len.saturating_add(#terms);)*
__tls_len
};
__tls_len
}
}
}

#[allow(unused_variables)]
fn impl_tls_size(parsed_ast: TlsStruct) -> TokenStream2 {
match parsed_ast {
Expand All @@ -779,13 +808,18 @@ fn impl_tls_size(parsed_ast: TlsStruct) -> TokenStream2 {
.iter()
.map(|p| p.for_trait("Size"))
.collect::<Vec<_>>();
let field_lengths = prefixes
.iter()
.zip(members.iter())
.map(|(prefix, member)| quote! { #prefix::tls_serialized_len(&self.#member) })
.collect::<Vec<_>>();
let serialized_len = sum_lengths(&field_lengths, quote! { 0usize });
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();
quote! {
impl #impl_generics tls_codec::Size for #ident #ty_generics #where_clause {
#[inline]
fn tls_serialized_len(&self) -> usize {
#(#prefixes::tls_serialized_len(&self.#members) + )*
0
#serialized_len
}
}

Expand All @@ -811,21 +845,35 @@ fn impl_tls_size(parsed_ast: TlsStruct) -> TokenStream2 {
let variant_id = &variant.ident;
let members = &variant.members;
let bindings = make_n_ids(members.len());
let prefixes = variant.member_prefixes.iter().map(|p| p.for_trait("Size")).collect::<Vec<_>>();
let prefixes = variant
.member_prefixes
.iter()
.map(|p| p.for_trait("Size"))
.collect::<Vec<_>>();
let field_lengths = prefixes
.iter()
.zip(bindings.iter())
.map(|(prefix, binding)| quote! { #prefix::tls_serialized_len(#binding) })
.collect::<Vec<_>>();
let variant_len = sum_lengths(&field_lengths, quote! { 0usize });
quote! {
#ident::#variant_id { #(#members: #bindings,)* } => 0 #(+ #prefixes::tls_serialized_len(#bindings))*,
#ident::#variant_id { #(#members: #bindings,)* } => #variant_len,
}
})
.collect::<Vec<_>>();
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();
let total_len = sum_lengths(
&[quote! { field_len }],
quote! { core::mem::size_of::<#repr>() },
);
quote! {
impl #impl_generics tls_codec::Size for #ident #ty_generics #where_clause {
#[inline]
fn tls_serialized_len(&self) -> usize {
let field_len = match self {
#(#field_arms)*
};
core::mem::size_of::<#repr>() + field_len
#total_len
}
}

Expand Down Expand Up @@ -1414,9 +1462,9 @@ fn impl_conditionally_deserializable(mut annotated_item: ItemStruct) -> TokenStr
quote! {
#annotated_item

#[doc = #doc_string_deserializable]
#annotated_item_visibility type #undeserializable_ident #original_ty_generics = #annotated_item_ident #undeserializable_ty_generics;
#[doc = #doc_string_undeserializable]
#annotated_item_visibility type #undeserializable_ident #original_ty_generics = #annotated_item_ident #undeserializable_ty_generics;
#[doc = #doc_string_deserializable]
#annotated_item_visibility type #deserializable_ident #original_ty_generics = #annotated_item_ident #deserializable_ty_generics;

#deserialize_implementation
Expand Down
4 changes: 2 additions & 2 deletions tls_codec/fuzz/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions tls_codec/fuzz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,15 @@ name = "string"
path = "fuzz_targets/string.rs"
test = false
doc = false

[[bin]]
name = "deserialize"
path = "fuzz_targets/deserialize.rs"
test = false
doc = false

[[bin]]
name = "bytes_inverse"
path = "fuzz_targets/bytes_inverse.rs"
test = false
doc = false
29 changes: 29 additions & 0 deletions tls_codec/fuzz/fuzz_targets/bytes_inverse.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#![no_main]
#![allow(deprecated)]

//! Round-trip fuzzing of the slice-based [`DeserializeBytes`] path.
//!
//! The existing `inverse` target only exercises the `Read`-based
//! [`Deserialize`] implementation. This one covers `tls_deserialize_bytes`
//! and additionally checks that both paths agree on a valid serialization.

use libfuzzer_sys::fuzz_target;
use tls_codec::{Deserialize, DeserializeBytes, Serialize, Size, VLBytes};

fuzz_target!(|expected: VLBytes| {
let serialized = expected.tls_serialize_detached().unwrap();

// Assert that the serialized length matches the predicted length.
assert_eq!(expected.tls_serialized_len(), serialized.len());

// Slice-based deserialization round-trips and consumes all bytes.
let (got, remainder) = VLBytes::tls_deserialize_bytes(&serialized).unwrap();
assert!(remainder.is_empty());
assert_eq!(expected, got);

// The `Read`-based path must agree with the slice-based path.
let mut read_slice = serialized.as_slice();
let got_read = VLBytes::tls_deserialize(&mut read_slice).unwrap();
assert!(read_slice.is_empty());
assert_eq!(expected, got_read);
});
Loading
Loading