Fix SMB 3.1.1 negotiate contexts, NTLM mechListMIC, and signing-channel timing for Windows compatibility#168
Fix SMB 3.1.1 negotiate contexts, NTLM mechListMIC, and signing-channel timing for Windows compatibility#168dbsxdbsx wants to merge 10 commits intoafiffon:mainfrom
Conversation
- update MSRV to 1.89 - release patches of afiffon#162 afiffon#167
Three issues fixed:
1. Always provide SPN to SSPI regardless of kerberos feature flag
The target name (SPN) was only passed to initialize_security_context()
when the `kerberos` feature was enabled. NTLM's Negotiate SSP also
requires the target name, and omitting it causes sspi-rs to return
NoCredentials even for pure-NTLM sessions.
2. Fix negotiate context ordering and empty EncryptionCapabilities
- Reorder negotiate contexts by ascending ContextType value as
recommended by MS-SMB2 2.2.3.1. NetnameNegotiateContextId was
incorrectly placed before EncryptionCapabilities.
- Skip EncryptionCapabilities when no ciphers are available (e.g.,
when the `encrypt` feature is disabled). MS-SMB2 requires
CipherCount > 0; sending cipher_count=0 causes Windows to reject
the negotiate request with STATUS_INVALID_PARAMETER.
3. Align SSPI context requirements with Windows SMB2 client behavior
- Remove DELEGATE flag (Kerberos-specific, not used by the Windows
SMB2 client per MS-SMB2 3.2.4.2.3).
- Add REPLAY_DETECT and SEQUENCE_DETECT (used by Windows client).
Fixes afiffon#164
Made-with: Cursor
sspi-rs's Negotiate SSP produces raw NTLM tokens when Kerberos is
unavailable, but SMB2 Session Setup mandates GSSAPI/SPNEGO blobs
(MS-SMB2 §3.2.4.2.3). Without the SPNEGO wrapper, Windows 10
rejects the security blob with STATUS_INVALID_PARAMETER (0xc000000d).
This commit adds a lightweight spnego module that:
- Wraps NTLM Type-1 in a SPNEGO NegTokenInit (first round)
- Wraps NTLM Type-3 in a SPNEGO NegTokenResp (subsequent rounds)
- Unwraps server's SPNEGO NegTokenResp to extract the inner
NTLM Type-2 challenge
The Authenticator now transparently applies this wrapping/unwrapping,
making smb-rs compatible with Windows SMB2 servers that require SPNEGO.
Fixes afiffon#164.
Made-with: Cursor
Log the token sizes before/after SPNEGO wrapping to help diagnose whether the wrapping layer is actually being invoked at runtime. Made-with: Cursor
sspi-rs's Negotiate SSP produces SPNEGO NegTokenInit that includes Kerberos OIDs (MS-KRB5, KRB5) in the mechTypes list alongside NTLMSSP. When the mechToken is actually NTLM, having Kerberos OIDs first may confuse certain Windows SMB2 servers, causing STATUS_INVALID_PARAMETER. This commit modifies the Authenticator to: 1. Detect sspi-rs's SPNEGO wrapper (APPLICATION[0], tag 0x60) 2. Extract the raw NTLM mechToken from it 3. Re-wrap in a minimal NegTokenInit with only the NTLMSSP OID Also adds unwrap_init() to the spnego module for parsing NegTokenInit. Ref: afiffon#164 Made-with: Cursor
sspi-rs's Negotiate SSP wraps NTLM tokens in a Kerberos/NegoEx framed SPNEGO envelope even when configured for NTLM-only mode. Windows 10's SMB server rejects this with STATUS_INVALID_PARAMETER. By driving the Ntlm SSP directly we get clean NTLM Type-1/2/3 tokens which our spnego module wraps in the standard SPNEGO envelope that SMB2 expects. This also simplifies the code significantly by removing the Kerberos-related branching and the complex token format detection logic. Made-with: Cursor
…efore final response Two spec-compliance fixes that together let the Windows SMB 3.1.1 server accept our session setup and allow the subsequent signed SUCCESS response to be verified: 1. SPNEGO mechListMIC (MS-NLMP 3.4.4.1 / MS-SPNG 3.1.5.1) Previously the MIC checksum always ran through RC4 with the client sealing key. Per MS-NLMP 3.4.4.1 the RC4 wrap is only applied when NTLMSSP_NEGOTIATE_KEY_EXCH (0x40000000) is negotiated; otherwise the checksum is just HMAC_MD5(...)[0..8]. sspi-rs itself does not honour this distinction, so we cannot rely on its make_signature. The fix parses the actual NegotiateFlags off the wire from the Type-3 AUTHENTICATE_MESSAGE (offset 60, little-endian) and gates the RC4 step on the KEY_EXCH bit. This removes the spurious STATUS_ACCESS_DENIED returned by Windows after our Type-3+MIC. 2. Session channel timing (MS-SMB2 3.2.5.3) The final SessionSetup response is signed by the server. The transformer validates signatures against the session channel independently of the handler's skip_security_validation flag, so the channel must exist before the response is received. Previously make_channel() ran only after the response had already been accepted, which raised 'Message is required to be signed, but the channel right after send_setup_request, as soon as NTLM reports the context complete and a session exists, before invoking receive_setup_response. A defensive fallback remains further down for legacy code paths. Supporting changes: - New helpers wrap_response_with_mic and MECH_TYPE_LIST_BYTES in spnego.rs - Helper extract_ntlm_type3_negotiate_flags in authenticator.rs - is_in_progress() on PreauthHashState - Sensitive key material (MIC keys, session_key, preauth hash) is gated behind the existing __debug-dump-keys feature; other handshake diagnostics lowered to trace.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/smb/src/session/authenticator.rs`:
- Around line 76-83: Authenticator::build currently always instantiates Ntlm
causing AuthMethodsConfig.kerberos and the kerberos feature to be ignored;
modify Authenticator::build to branch on AuthMethodsConfig.kerberos: if false
(or if the kerberos feature is disabled) keep the existing Ntlm flow
(Ntlm::with_config, acquire_credentials_handle, etc.), and if true (and the
kerberos feature is enabled) use the sspi_network_client path (import/use
sspi_network_client types and call its Kerberos client construction/credential
acquisition instead); also add a compile-time guard that returns a clear error
if AuthMethodsConfig.kerberos is true but the crate is built without the
kerberos feature so callers get a deterministic failure instead of silently
falling back to NTLM.
In `@crates/smb/src/session/setup.rs`:
- Around line 187-235: The branch currently treats is_auth_done as success even
when server_needs_more is true; change the logic so that if server_needs_more is
set you do NOT finalize the session or return success: either perform an
explicit SPNEGO finalization flow (call the codepath that sends the SPNEGO
MIC/acknowledgement — e.g. implement/invoke a make_accept_complete_* or
send_final_spnego_token routine) before calling make_channel()/finalizing
preauth_hash, or return an error instead of proceeding; specifically, in the
block that checks is_auth_done and before calling self.make_channel().await? or
returning from setup, add a guard on server_needs_more to 1) invoke the explicit
SPNEGO completion routine (referencing the same preauth_hash and
session_setup_response objects) and only after a successful completion call
self.make_channel().await? and continue, or 2) return
Err(Error::InvalidMessage("Server requires further SPNEGO
processing".to_string())) so the session is not marked ready.
In `@crates/smb/src/session/spnego.rs`:
- Around line 83-89: der_skip_header currently returns the advertised value
length without verifying the buffer actually contains that many bytes, allowing
truncated DER to be silently accepted; modify der_skip_header (and the other
DER-parsing sites around the regions noted) to compute header_len = 1 +
len_bytes, then verify data.len() >= header_len + val_len and return
Err(Error::InvalidMessage(...)) if not, rather than letting callers clip with
.min(...); also update callers that slice container/OCTET STRING values to slice
exactly by the declared end (header_len..header_len+val_len) and remove the
`.min(...)` fallback behavior so malformed/truncated SPNEGO/NTLM tokens are
rejected instead of being passed through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50e913d6-2869-423c-b968-f0846bc387fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/smb/Cargo.tomlcrates/smb/src/connection.rscrates/smb/src/connection/preauth_hash.rscrates/smb/src/session.rscrates/smb/src/session/authenticator.rscrates/smb/src/session/setup.rscrates/smb/src/session/spnego.rs
Three defensive fixes, no behavioural change on the verified success path: * authenticator.rs: reject `auth_methods.kerberos = true` with `UnsupportedAuthenticationMechanism` when NTLM is also disabled, and NTLM for callers that explicitly asked for Kerberos. Full Kerberos SSP support is out of scope for this PR. * setup.rs: when `is_auth_done && server_needs_more`, return `Error::InvalidState` instead of warning + forcing the loop to exit. Per MS-SMB2 §3.2.5.3 the session is not finalized on the server at that point, so treating this status as success could leave the client talking over a half-negotiated session. * spnego.rs: `der_skip_header` now validates that the buffer actually holds `header_len + val_len` bytes before returning; truncated TLVs yield an explicit `InvalidMessage` instead of being silently clipped by the callers' `.min(...)` fallbacks, which have been removed. Verification: `cargo test -p smb --lib --no-default-features --features async,sign,std-fs-impls` -> 15 passed, 0 failed (including the 5 new spnego tests).
|
Hello @dbsxdbsx This seems to be a mostly AI-generated patch. Please explain simple in your own words, and briefly where did the issue raise first (exact server setup), and what tests have you done to make sure everything's still functioning properly. Thanks. |
|
Hi @afiffon — fair ask, let me try in plain language.(But Again, I am not confident explaining it correctly without using AI) ContextI'm a hobbyist, not a protocol person. I'm building a small cross-platform media app for personal use — basically "watch videos from the home PC on the TV / phone." To do that I need a pure-Rust SMB client, and this is the only one that exists. I pointed it at a stock Windows 10 share and it didn't work. #164 showed three other users hitting the same wall, so I dug in. The three bugs, as picturesThink of SMB's handshake as a short polite dialog.
Nothing invented. All three fixes cite What I tested
Honest caveatI'm a drive-by contributor. Not an SMB expert — I leaned heavily on an AI assistant and the specs, not on prior protocol knowledge. I opened this because my app needed it and #164 had three other blocked users, so it felt wasteful not to share. I don't have bandwidth to own this area long-term; please feel free to cherry-pick, rewrite, or close. No offense taken. The red CINot a behaviour regression. Thanks for looking. updated: Follow-up: pushed
Summary of the commit:
Looks like GitHub is waiting for your approval to run workflows on this branch — let me know if you'd like it re-kicked. |
All changes are style-only (no behaviour changes): - spnego.rs: mark the SPNEGO helpers kept for symmetry (`make_accept_complete`, `unwrap_init`) with `#[allow(dead_code)]`. - authenticator.rs: drop needless borrows on `session_key` (`Md5::update` takes `AsRef<[u8]>`, `[u8;16]` is `Copy`); reformat the MIC doc comment into a code block to silence `doc list item overindented`. - setup.rs: collapse the two nested `if` blocks in `next_preauth_hash` via let-chains. - sspi_network_client.rs: `allow(dead_code)` on the blocking `ReqwestNetworkClient` mod (unchanged upstream code that was unused already but newly flagged by stricter nightly clippy); `allow(unused_imports)` on the `pub use` re-export. - Apply `cargo fmt` across the touched files. Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/smb/src/session/authenticator.rs`:
- Around line 14-31: The rc4_crypt function is a hand-rolled RC4 implementation
(rc4_crypt) which should be replaced by a vetted crate; remove rc4_crypt and use
a maintained RC4 primitive (e.g., the rc4 crate or rust-crypto's rc4::Rc4)
instead: add the chosen dependency, import the RC4 type and key-stream trait,
construct the cipher with the sealing key (sealing_key) where rc4_crypt was
used, and stream-xor the input data to produce the same output; update all
callers to use the new cipher invocation and remove the local implementation to
avoid duplication and maintenance drift.
- Around line 129-133: The current session_key() implementation calls
try_into().unwrap() which will panic if key_info.session_key is shorter than 16
bytes; change it to validate the length and return an Err variant instead of
unwrapping: after calling self.ssp.query_context_session_key()?, check
key_info.session_key.as_ref().len() >= 16 (or pattern-match the slice), copy the
first 16 bytes into a [u8;16], and return Ok(array) or return a crate::Error (or
appropriate crate::Result error) with context if the key is too short; reference
symbols: session_key(), key_info, session_key, and
ssp.query_context_session_key().
In `@crates/smb/src/session/setup.rs`:
- Around line 219-230: The check and assignment around server_needs_more are
redundant: inside the block where you validate signing (the if that currently
tests !server_needs_more &&
!session_setup_response.session_flags.is_guest_or_null_session() &&
!message_form.signed_or_encrypted()), remove the leading !server_needs_more from
the condition and delete the trailing server_needs_more = false assignment,
leaving the intent-only checks on
session_setup_response.session_flags.is_guest_or_null_session() and
message_form.signed_or_encrypted(); this preserves the signed-message validation
(using session_setup_response.session_flags.is_guest_or_null_session() and
message_form.signed_or_encrypted()) while eliminating the no-op guard and
assignment.
In `@crates/smb/src/session/spnego.rs`:
- Around line 257-318: Duplicate SEQUENCE-walking logic in unwrap_init and
unwrap_response should be extracted into a helper to avoid copy/paste; create a
function like fn find_context_octet_string(seq_body: &[u8], ctx_tag: u8) ->
crate::Result<Option<Vec<u8>>> that iterates the DER SEQUENCE using
der_skip_header, matches elements by tag (ctx_tag, e.g. 0xa2), verifies an inner
OCTET STRING (0x04), and returns the extracted Vec<u8> when found
(Ok(Some(...))) or Ok(None) if not found, propagating der_skip_header errors;
then replace the loop in unwrap_init (currently looking for 0xa2 mechToken) and
the similar loop in unwrap_response to call this helper and handle None by
returning the existing InvalidMessage error.
- Around line 33-43: der_encode_length currently silently truncates lengths >=
0x1_000_000; change der_encode_length to validate the input against the
decoder's ceiling (the same threshold used by der_read_length) and return a
Result<Vec<u8>, _> (or an explicit error type) when len >= 0x1_000_000 instead
of encoding a truncated 3-byte length; update callers to propagate or handle the
error so oversized payloads fail fast rather than producing malformed TLVs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a19dab61-c6bb-48f0-8c11-7138734d99e6
📒 Files selected for processing (5)
crates/smb/src/connection.rscrates/smb/src/session/authenticator.rscrates/smb/src/session/setup.rscrates/smb/src/session/spnego.rscrates/smb/src/session/sspi_network_client.rs
Three small hardening fixes surfaced by the latest CodeRabbit pass: - authenticator.rs: `session_key()` no longer uses `.try_into().unwrap()`; instead it validates `len() >= 16` and returns `Error::InvalidState` if an upstream SSPI implementation ever hands back a shorter session key, so a malformed response degrades to a Result error instead of panicking. - spnego.rs: `der_encode_length()` now panics with a clear message for lengths `>= 0x0100_0000` instead of silently truncating the high byte. This matches the 3-byte ceiling already enforced by `der_read_length()`, so an encoder / decoder mismatch can no longer produce a malformed TLV on the wire. Added two unit tests covering every branch and the new overflow assertion. - setup.rs: dropped the `!server_needs_more` guard and the trailing `server_needs_more = false` assignment in the authenticated branch. The earlier `return Err(...)` on `server_needs_more` already guarantees the flag is false at this point, so both were dead code. No behaviour change on success paths; all 19 spnego / state / unc_path unit tests continue to pass. Co-Authored-By: Oz <oz-agent@warp.dev>
|
|
||
| // 0x0008 - SMB2_SIGNING_CAPABILITIES | ||
| if !signing_algorithms.is_empty() { | ||
| ctx_list.push(SigningCapabilities { signing_algorithms }.into()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Good catch. Let's add an assert to struct SigningCapabilities's field signing_algorithm_count - [brw(assert(signing_algorithm_count > 0)].
- Apply this fix to all the other relevant capability structures.
- Remove all the added comments regarding the ordering on this function (including the 0x...), since it is probably irrelevant?
|
|
||
| // Context list supported on SMB3.1.1+ | ||
| // Negotiate context list for SMB 3.1.1 (MS-SMB2 2.2.3.1). | ||
| // Contexts are ordered by ascending ContextType value as recommended by the spec. |
There was a problem hiding this comment.
"The server MUST support receiving negotiate contexts in any order."
There was a problem hiding this comment.
This is the biggest worry for me. I'm afraid this code is misplaced, and I'll explain.
The Negotiate implementation should reside in the sspi crate as it have been previously. Their implementation has some issues, which I have already raised, and they seemed to be working really hard to make things better (e.g. Devolutions/sspi-rs#600 - thx sspi-rs team!).
Did you check what of this code they have already implemented and should be tested and working properly? I know they currently might have some nasty bugs in newer versions - revert to the working version on main and test against it (probably? I might be wrong, but I did mention it in the #168)
There was a problem hiding this comment.
This comment is also VERY relevant to setup.rs
| if auth_methods.kerberos && !auth_methods.ntlm { | ||
| return Err(Error::UnsupportedAuthenticationMechanism( | ||
| "Kerberos-only authentication is not supported by this build; \ | ||
| enable NTLM (auth_methods.ntlm = true) or use a build with \ |
There was a problem hiding this comment.
please read the below comments regarding negotiate implementation.
|
@dbsxdbsx Let me begin by saying that being a contributor is not trivial, so thanks for PR-ing. Read the comments in the review, and focus on the big, great comment regarding the authentication bugfix, and let me know what you think. It's the core of my being (possibly) a bit rude regarding AI usage. This piece of code is super sensitive and has been tested against multiple environments, and breaking it is quite terrible. I rather have the fixes properly implemented and tested, and I believe most of this PR fixes in this section are not towards that direction. |
I am sorry for the late response. First of all, I completely understand that as a well-known public Rust library, you must be extremely sensitive regarding performance and the reliability of any fixes. As I mentioned in the disclaimer block at the head of this issue, most of the content was generated by AI, and I don't actually have a deep understanding of this subject. My initial encounter with this library happened because AI recommended it to me while I was looking for a way to remotely invoke Windows backend resources. Specifically, it recommended the SMB library. Throughout this process, I have had to rely on AI to handle the fixes and explanations because I am not very familiar with network security or IT networking. I have tried my best to have the AI assist me in explaining the changes, but I am truly unable to explain the technical details to you in a way that I personally understand. I think there is a compromise we can make. Although I don't have the capacity to fully understand this specific part, you can see all the other code I have submitted. You could review how this was fixed based on your own understanding, and then submit a PR yourself in the way you see fit. To be honest, I am truly unable to handle this myself for two reasons:
Thanks ,and feel free to close this PR if you think it's needed. |
2 participants
Fix SMB 3.1.1 negotiate contexts, NTLM mechListMIC, and signing-channel timing for Windows compatibility
Fixes #164. Also resolves the follow-on
STATUS_INVALID_PARAMETERandSTATUS_ACCESS_DENIEDfailures reported in the same thread.TL;DR — what was breaking, in plain words
If you asked this library to talk to a real Windows 10 / Server 2019+ SMB
box, three things went wrong:
The SMB 3.1.1 Negotiate Contexts were malformed.
The context list sent during Negotiate did not follow the ascending
ContextTypeorder that [MS-SMB2] recommends, and includedEncryptionCapabilities/SigningCapabilitieswith empty algorithmlists (violating the "count MUST be greater than zero" constraint).
Windows rejected the entire Negotiate request with
STATUS_INVALID_PARAMETER.The mechListMIC was "over-encrypted".
SPNEGO asks the client to attach a small cryptographic tag (a MIC) to
prove it really owns the session key. The recipe for that tag has a
conditional step: "wrap the 8-byte HMAC with RC4, but only if we
negotiated a key-exchange flag". We were always running the RC4
step, even when the server had explicitly told us "no key exchange
this time". Windows noticed, rejected the MIC, and sent back
STATUS_ACCESS_DENIED. (Samba is much more lenient here, which iswhy nobody hit this for a long time.)
We tried to read a signed letter before we knew how to read the
signature.
SMB 3.1.1 says: the server's final "you're in" response will be
cryptographically signed, and the client must verify that signature
with keys derived from the preauth hash. The problem is key
derivation in this library happened after we had already received
and accepted that response. The message transformer therefore
complained
Message is required to be signed, but no channel is set up!and bailed. Again, Samba tolerates this timing; Windows doesnot.
So this PR:
§2.2.3.1 so the 3.1.1 handshake is accepted;
by reading the real NegotiateFlags from the Type-3 message we just
built and gating the RC4 step on
NTLMSSP_NEGOTIATE_KEY_EXCH;make_channel()) to the momentauthentication finishes — i.e. right after we send Type-3, before
we receive the signed SUCCESS — which is exactly what [MS-SMB2] §3.2.5.3
prescribes.
Nothing new is invented. Everything is straight out of the specs.
Is this a security issue?
No. These are protocol-conformance bugs, not vulnerabilities.
The existing code was just doing the wrong thing per spec, and a strict
peer (Windows) refused to play along.
What SMB servers are actually affected?
The library itself is server-agnostic (SMB is spoken by Windows, Samba,
macOS, Synology, ksmbd, …). But these bugs mostly surface against
Windows-family SMB servers, because Windows validates Negotiate
Contexts, MIC, and signing strictly. Samba is permissive enough that
most users never noticed.
Data flow — before vs. after
sequenceDiagram participant C as Client (smb-rs) participant S as Server (Windows SMB) Note over C,S: Bug 1 — mechListMIC over-encrypted C->>S: Negotiate (SMB 3.1.1) S->>C: Negotiate response C->>S: SessionSetup #1 (NTLM Type-1 in SPNEGO) S->>C: SessionSetup #2 (NTLM Type-2, MORE_PROCESSING_REQUIRED) Note right of C: compute_mech_list_mic():<br/>checksum = RC4(sealing_key, HMAC[..8])<br/>❌ applied unconditionally C->>S: SessionSetup #3 (NTLM Type-3 + wrong MIC) S-->>C: ❌ STATUS_ACCESS_DENIED (0xC0000022)sequenceDiagram participant C as Client (smb-rs) participant S as Server (Windows SMB) Note over C,S: Bug 2 — signing channel derived too late C->>S: SessionSetup #3 (NTLM Type-3 + correct MIC) S->>C: SessionSetup #4 (SUCCESS, SIGNED) Note right of C: transformer verifies signature<br/>but channel (signing key) not yet built<br/>❌ "Message is required to be signed,<br/>but no channel is set up!"sequenceDiagram autonumber participant C as Client (smb-rs, after this PR) participant S as Server (Windows SMB) Note over C,S: AFTER — handshake completes C->>S: Negotiate (SMB 3.1.1) S->>C: Negotiate response C->>S: SessionSetup #1 (NTLM Type-1 in SPNEGO) S->>C: SessionSetup #2 (NTLM Type-2, MORE_PROCESSING_REQUIRED) Note right of C: extract_ntlm_type3_negotiate_flags()<br/>reads NegotiateFlags off the wire<br/>then compute_mech_list_mic():<br/>✅ RC4 only if KEY_EXCH bit set<br/>else checksum = HMAC[..8] C->>S: SessionSetup #3 (NTLM Type-3 + correct MIC) Note right of C: auth is now done →<br/>finalize preauth hash →<br/>✅ make_channel() derives signing key<br/>BEFORE receiving the response S->>C: SessionSetup #4 (SUCCESS, SIGNED) Note right of C: transformer verifies signature ✅ C->>S: TreeConnect, Open, Read, … S->>C: signed responses, all verified ✅Change summary
0. Fix Negotiate Context ordering and conditional emission
File:
crates/smb/src/connection.rsContextTypevalues as recommended by [MS-SMB2] §2.2.3.1:
0x0001(PreauthIntegrity) →0x0002(Encryption) →0x0003(Compression) →
0x0005(Netname) → … →0x0008(Signing).EncryptionCapabilitiesandSigningCapabilitiesare now only includedwhen their algorithm lists are non-empty. Per [MS-SMB2],
CipherCount/
SigningAlgorithmCountMUST be greater than zero; sending an emptylist caused Windows to reject the Negotiate with
STATUS_INVALID_PARAMETER.1. Correct mechListMIC computation
File:
crates/smb/src/session/authenticator.rsNew helper
extract_ntlm_type3_negotiate_flags()parses thelittle-endian
NegotiateFlagsfield at offset 60 of the AUTHENTICATEmessage (layout per [MS-NLMP] 2.2.1.3), after validating the NTLMSSP
signature and
MessageType == 3.compute_mech_list_mic()now takes anntlm_negotiate_flags: u32parameter and follows [MS-NLMP] §3.4.4.1 exactly:
The call site in
next()reads the flags from the raw Type-3 tokenthat SSPI just emitted, so we always use the exact value that will
be on the wire — no assumptions.
2. Derive the signing channel before the final SUCCESS
File:
crates/smb/src/session/setup.rsRight after
send_setup_request(), if the authenticator reportsis_authenticated()and a session object already exists (i.e. we havea
session_idfrom Type-2), the loop now:make_channel()to derive the signing key,before issuing
receive_setup_response().This matches [MS-SMB2] §3.2.5.3: the signing key is derived from
SessionKeyand the preauth hash covering every message up to andincluding the final client request, excluding the final SUCCESS
response — which is exactly the state we are in at that point.
A defensive fallback in the post-receive
is_auth_doneblock stillruns
make_channel()ifpreauth_hash.is_in_progress(), so anylegacy code path that skips the early-finalize branch remains
correct.
3. Supporting pieces
crates/smb/src/session/spnego.rs(new): minimal SPNEGO DER helpers —wrap_init(),wrap_response(),wrap_response_with_mic(),unwrap_response(),MECH_TYPE_LIST_BYTES,make_accept_complete[_with_mic]()— needed to emit NegTokenInit /NegTokenResp envelopes carrying a mechListMIC. Previously
sspi-rs'sNegotiateSSP handled SPNEGO framing, but switching to theNtlmSSP directly (to avoid NegoEx framing that Windows rejects) means we
now wrap / unwrap SPNEGO ourselves.
crates/smb/src/connection/preauth_hash.rs: tinyis_in_progress()accessor, used by the defensive fallback.crates/smb/Cargo.toml: addedmd-5dependency (required bycompute_mech_list_mic()for deriving the HMAC-MD5 signing andsealing keys).
preauth hash bytes) is gated behind the existing
__debug-dump-keysCargo feature; everyday SPNEGO / NTLM progresslines have been lowered from
debugtotraceto keep normalruns quiet.
Verification
Unit tests
All existing unit tests pass with no regressions (ran on Windows):
cargo clippyproduces only pre-existing warnings (doc indentationstyle), no errors.
Integration / end-to-end
Tested against a Windows 10 19045 SMB server (Hyper-V Default Switch,
Administratorwith empty password, SMB 3.1.1, signing required):IPC$and a real share,both succeed.
\PIPE\srvsvc, directory listings, file Open,file Close) are all signed outgoing and verified incoming, i.e.
Message #N signedandMessage #N verifiedappear in pairs.tests (
list_smb_shares,init_and_list,get_stream_url) and allthree pass.
No regressions observed against Samba (still works exactly as before —
the pre-fix code path was within Samba's tolerance window, so the new
code path still is).
Note: the repository's Docker-based integration tests (
crates/smb/tests/)require a Samba container (
ghcr.io/afiffon/smb-tests:latest) which isset up in CI. These tests exercise features unrelated to the NTLM / SPNEGO
changes in this PR, so the existing CI workflow should continue to pass
as-is.
References
extractor)
NTLMSSP_NEGOTIATE_KEY_EXCH = 0x40000000Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores