tls_codec: 0.4.3 prep#2365
Open
franziskuskiefer wants to merge 3 commits into
Open
Conversation
franziskuskiefer
force-pushed
the
franziskus/tls_codec-0.4.3-prep
branch
from
2e91fee to
9349b57
Compare
franziskuskiefer
force-pushed
the
franziskus/tls_codec-0.4.3-prep
branch
from
9349b57 to
b31a9e9
Compare
wysiwys
reviewed
Jul 3, 2026
| let length = ContentLength::from_usize(content_length)?; | ||
| let len_len = length.0.bytes_len(); | ||
|
|
||
| let mut out = Vec::with_capacity(content_length + len_len); |
Contributor
There was a problem hiding this comment.
If the total serialized content length is isize::MAX, this vector allocation could panic. I think this could use a check first that content_length + len_len <= isize::MAX.
| /// Returns [`Error::EndOfStream`] if the reader is exhausted before `len` bytes | ||
| /// are read. | ||
| #[cfg(feature = "std")] | ||
| fn read_bytes_bounded<R: std::io::Read>(reader: &mut R, len: usize) -> Result<Vec<u8>, Error> { |
Contributor
There was a problem hiding this comment.
A nit on the parameter name: I think len could be renamed to expected_len or similar, to be more descriptive
| #[cfg(feature = "std")] | ||
| #[inline(always)] | ||
| fn deserialize_bytes<R: Read>(bytes: &mut R) -> Result<Self, Error> { | ||
| let len = <$size>::tls_deserialize(bytes)?.try_into().unwrap(); |
Contributor
There was a problem hiding this comment.
It looks like this may panic on 16-bit platforms where $size is u32
|
|
||
| // `read_to_end` reads directly into the vector's spare capacity and retries | ||
| // `ErrorKind::Interrupted` internally, unlike a bare `read` loop. | ||
| reader.take(len as u64).read_to_end(&mut result)?; |
Contributor
There was a problem hiding this comment.
Since this method is used for types that are internally represented as byte vectors, and the bytes are read here into a byte vector, the size of the serialized data cannot exceed isize::MAX. I think that before reaching this line, an initial check that len does not exceed isize::MAX would be useful in order to rule out invalid lengths early on
|
|
||
| ### 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. |
Contributor
There was a problem hiding this comment.
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. |
| - [#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. | ||
| - 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. |
Contributor
There was a problem hiding this comment.
checked_add() is not yet used in the Deserialize impls for VLBytes and VLByteVec, right?
Comment on lines
+770
to
+773
| /// 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). |
Contributor
There was a problem hiding this comment.
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release prep for the next tls_codec release.
This also fixes a couple more issues (see changelog) and adds some more fuzzers.