Fix three implementation bugs found during codebase audit#3
Merged
tmthecoder merged 1 commit intomainfrom Feb 8, 2026
Merged
Conversation
B1: Fix u64 bit shift 54 -> 56 in byte_helper.rs B2: Fix SMBServerError::Display wrong prefix B3: Add V3_0_2 to signing key KDF derivation branch Includes regression tests for all three fixes.
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
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.
Summary
Fixes three correctness bugs discovered during a deep codebase audit against the Rust Book and MS-SMB2 spec.
Bug Fixes
B1:
byte_helper.rs— u64 bit shift uses 54 instead of 56 (data corruption)File:
smb/src/byte_helper.rsProblem: Both
bytes_to_u64andu64_to_bytesuse bit shift54for the high byte (index 7) instead of the correct56(7 × 8 = 56). This follows the pattern0, 8, 16, 24, 32, 40, 48, **54**where the last value should be56.This is a data corruption bug — any
u64value with significant bits above position 53 will be silently mangled during serialization or deserialization. It has not manifested in integration tests because the only call sites (FileTimeandSMBExtra) currently operate on values small enough that the top byte is zero or trivially small. However, it will break for large file sizes, far-future timestamps, or any u64 field where the top byte is non-trivial.Fix: Changed
<< 54to<< 56and>> 54to>> 56in both functions.Tests: The existing
u64_max_value_round_tripandu64_high_bits_correctnesstests (which were previously documenting the bug) now pass.B2:
SMBServerError::Displaysays "Parse failed" instead of "Server operation failed"File:
smb-core/src/error.rsProblem: The
Displayimpl forSMBServerErrorprinted"Parse failed with error: ..."which is copy-pasted fromSMBParseError. This is misleading — a server error is not a parse error.Fix: Changed the prefix to
"Server operation failed with error: ...".Tests: Added 5 new tests covering the
Displayoutput of all error variants (SMBServerError,SMBParseError,SMBCryptoError,SMBPayloadSizeError,SMBResponseError) to prevent future regressions.B3:
SMBDialect::V3_0_2missing from signing key KDF derivation branchFile:
smb/src/server/session.rsProblem: In
generate_keys(), the match arm for KDF-derived signing keys only coveredV3_0_0 | V3_1_1.V3_0_2fell through to the_wildcard, which returns the raw session key without KDF derivation. Per [MS-SMB2] Section 3.3.5.5.3, all SMB 3.x dialects (3.0, 3.0.2, 3.1.1) must derive signing keys using SP800-108 KDF.Fix: Added
SMBDialect::V3_0_2to the KDF match arm.Tests: Added 3 new tests verifying that
generate_keyproduces correct-length output, that KDF output differs from the raw session key, and that V3_0_2 and V3_0_0 produce identical signing keys (since they use the same label/context).Test Results
All 13 tests pass:
smb-core: 5 new error Display tests ✅smb_reader::byte_helper: 5 tests (3 existing + 2 previously-failing now fixed) ✅smb_reader::server::session: 3 new KDF tests ✅