fix(smb2): correct SMB 3.1.1 cipher key labels and harden response decoding - #80
Merged
Conversation
…coding Fixes found during an MS-SMB2 / MS-FSCC conformance review of the SMB2/3 client (org.codelibs.jcifs.smb). Correctness: - Smb3KeyDerivation: SMB 3.1.1 encryption key labels were "SMB2C2SCipherKey" / "SMB2S2CCipherKey". The SP800-108 KDF requires "SMBC2SCipherKey" / "SMBS2CCipherKey"; the stray "2" derives encryption keys that never match a real server. - Smb2ReadResponse: DataOffset was read as a signed byte, so values >= 0x80 became negative. Read it as an unsigned byte. - FileBothDirectoryInfo.decode: returned a negative byte count (start - bufferIndex), violating the Decodable contract; return bufferIndex - start. Robustness (validate server-controlled offsets/lengths before allocating or indexing, to prevent a malicious/malformed server from triggering AIOOBE/OOM on the client): - Smb2CreateResponse: validate create-context name/data offset+length. - Smb2ReadResponse: validate data offset+length against the packet bounds. - Smb2QueryDirectoryResponse: reject a negative/out-of-range OutputBufferLength instead of decoding one bogus entry. - FileBothDirectoryInfo: bound-check the file name length. Reliability: - SmbFileOutputStream / SmbRandomAccessFile: throw instead of looping forever when the server reports a zero-length write with bytes still remaining. - SmbFileOutputStream.close(): null-guard the handle (matching the input stream). - SmbRandomAccessFile: fix the RemainingBytes hint (report bytes left in the request, not offset-adjusted). - FileRenameInformation2.encode: explicitly zero Reserved / RootDirectory rather than relying on buffer pre-zeroing. - SmbTransportImpl: use a dedicated final monitor for the reassigned preauth-integrity hash, apply topup credits to the current chain head, and null-guard getResponse() on the zero-credit path. - Transport: pass the current request (not the chain head) to handleIntermediate on the async wait path. Tests updated for the corrected decode contract and the new decode-time validation.
Follow-up to the SMB2/3 conformance-review fixes: closes gaps where a malformed/malicious server could still trigger a raw exception or an infinite loop in code the initial pass only partially hardened. Robustness (server-controlled offset/length validation): - FileBothDirectoryInfo.decode: bound the fixed 94-byte entry portion and reject a ShortNameLength > 24 (fixed 24-byte field), preventing a raw StringIndexOutOfBoundsException / over-read; the previous change guarded only the adjacent FileNameLength. - Smb2CreateResponse: validate the 16-byte create-context entry header before reading it, so a crafted CreateContextsOffset / next offset yields an SMBProtocolDecodingException instead of ArrayIndexOutOfBoundsException. - Smb2QueryDirectoryResponse: add a long-safe bound before advancing by nextEntryOffset, so a large value can no longer overflow bufferIndex to a negative index. Reliability: - SmbFileOutputStream.writeDirect: hoist the zero-length-write guard and the fp/len/off updates out of the protocol branches so the SMB1 (NT and legacy) paths can no longer loop forever, matching SmbRandomAccessFile.write. - SmbTransportImpl: mark preauthIntegrityHash volatile so cross-thread readers observe the value published under preauthLock. - SmbRandomAccessFile: fix the SMB1 write RemainingBytes hint (len - w, not len - w - off), matching the SMB2 path. Tests: add decode-time rejection tests for the ShortNameLength / truncated entry, the out-of-range create-context offset, and the overflowing nextEntryOffset.
… SMB3 cipher-key labels Follow-up hardening in the same low-risk, high-confidence spirit as the rest of this PR: apply the server-controlled offset/length validation to the response decoders that still lacked it, correct an inverted decode return, read a UCHAR field unsigned, and lock in the SMB 3.1.1 cipher-key labels with a known-answer test. Robustness (server-controlled offset/length validation): - Smb2IoctlResponse: validate input/output offset+length against the packet bounds before decode/arraycopy. The fields are INT4 and the existing output-capacity guard covered neither a bad source offset nor a negative count, so a malicious server could trigger a raw AIOOBE. - Smb2QueryInfoResponse: reject a negative / out-of-range buffer offset+length before decoding (same shape as the Smb2QueryDirectory- Response fix). - Smb2SessionSetupResponse: validate the security-buffer offset+length before allocating/copying; this path is reachable pre-authentication. Correctness: - SmbComLockingAndX: the four wire methods returned a negative consumed- byte count (start - index); return the positive count (index - start) to match the Decodable/encode contract and every sibling SMB1 command. - ServerMessageBlock2: read ErrorContextCount (a UCHAR) as unsigned. Testing: - Smb3KeyDerivation: add a known-answer test that reproduces the SP800-108 KDF with an independent in-test oracle (cross-checked against the never-buggy SMBSigningKey label) and pins the SMB 3.1.1 cipher-key labels SMBC2SCipherKey / SMBS2CCipherKey, so reintroducing the stray "2" is caught. - mvn test: 8448 tests, 0 failures, 0 errors, 0 skipped.
Follow-up to the SMB2/3 conformance-review hardening: extend the same server-controlled offset/length validation to the response decoders the earlier passes left unguarded, so a malformed/malicious server cannot trigger ArrayIndexOutOfBoundsException / OutOfMemoryError / NegativeArraySizeException on the client. Robustness (server-controlled offset/length validation): - ServerMessageBlock2.readErrorResponse: bound the error-data ByteCount against the packet bounds before new byte[] / arraycopy. This path is reached on every response's error branch (structureSize == 9), including pre-authentication. - Smb2NegotiateResponse: bound the SMB 3.1.1 negotiate-context loop (context header and per-context data length) against the packet bounds. Reachable pre-authentication. - Smb2ChangeNotifyResponse: validate the output buffer offset/length and make the per-entry advance long-safe, mirroring the Smb2QueryDirectoryResponse fix. - FileNotifyInformationImpl.decode: bound the file name length before decoding; this also hardens the SMB1 NT_TRANSACT change-notify path that shares this decoder. - SrvPipePeekResponse.decode: reject a length below the 16-byte fixed header instead of throwing NegativeArraySizeException. Testing: - Add decode-time rejection tests for each new guard, plus a valid-packet test per decoder to confirm no legitimate response is rejected. - Smb2ReadResponse: add an unsigned DataOffset (>= 0x80) decode test that pins the & 0xFF read. - FileRenameInformation2: add a test that pins the explicit Reserved / RootDirectory zeroing. - mvn test (touched classes): 335 tests, 0 failures, 0 errors, 0 skipped.
Complete the previous decode-hardening pass, which validated the outer offsets but left two reachable over-reads: - PreauthIntegrityNegotiateContext.decode / EncryptionNegotiateContext.decode: these read server-controlled element counts (HashAlgorithmCount / SaltLength / CipherCount, each a UINT16 up to 65535) and then allocate and read that many entries without validating them against the packet bounds. The outer Smb2NegotiateResponse guard only bounds the context offset/length, not these inner counts, so a malicious SMB 3.1.1 server could still overrun the negotiate buffer (ArrayIndexOutOfBoundsException) pre-authentication. Bound the cumulative reads against the buffer length. - FileNotifyInformationImpl.decode: the previous change bounded only the variable file name; the fixed 12-byte entry header (nextEntryOffset, action, fileNameLength) was still read unchecked. Bound it up front, mirroring the fixed-header check in FileBothDirectoryInfo. This also hardens the SMB1 NT_TRANSACT change-notify path that shares the decoder. Testing: - Add decode-time rejection tests (oversized count / truncated header) and paired valid-input tests for each guard. - mvn test (touched classes): 283 tests, 0 failures, 0 errors, 0 skipped.
Extend the decode hardening to two more response-reachable decoders that allocated/indexed from unvalidated server-controlled fields: - SID(byte[], int): the binary constructor read the fixed 8-byte header and sub_authority_count*4 bytes without checking them against the source length, and a sub_authority_count with the high bit set passed the `> 100` check and produced a negative array size. Bound the fixed header and the sub-authority region, and reject a negative count. This also hardens the ACE and Kerberos PAC decode paths that share this constructor. - SecurityDescriptor.decode: validate the fixed header length, the owner / group / DACL offsets and the ACL header against the buffer bounds, reject a negative ACE count (previously only `> 4096` was checked, so a negative count reached new ACE[count]), and bound each ACE entry read. - Referral.decode / readString: validate the fixed (and v3) header and the string offsets, and cap the UNICODE-termination scan to the buffer so a crafted path/alt-path/node offset can no longer over-read. All checks are exclusive-bound over-approximations against the receive buffer length, so a well-formed response is never rejected. Testing: - Add reject tests (truncated buffer / out-of-range offset / negative count) and paired valid-input tests for each decoder. - mvn test (SID, SecurityDescriptor, Referral, ACE, dtyp, dfs and the full pac/kerberos suites): 547 tests, 0 failures, 0 errors, 0 skipped.
…ffset Make the SecurityDescriptor header bound symmetric with the SID and DFS referral guards by also rejecting a negative start index. Not reachable from the current callers (which pass a non-negative offset), but keeps the bounds check defensive and consistent.
marevol
marked this pull request as ready for review
July 4, 2026 05:23
…ed contract The decode-hardening in this branch replaced the raw ArrayIndexOutOfBoundsException that SecurityDescriptor.decode used to throw on a malformed / truncated buffer with a controlled SMBProtocolDecodingException. Five pre-existing error-path tests still asserted the old raw AIOOBE and therefore failed: - MsrpcShareGetInfoTest (empty / mismatched-size / invalid-format security descriptor): getSecurity() now surfaces SMBProtocolDecodingException. - NtTransQuerySecurityDescResponseTest (too-small / zero-length buffer): readDataWireFormat catches the IOException (SMBProtocolDecodingException) and rethrows it as RuntimeCIFSException, so these cases now expect RuntimeCIFSException. Only the expected exception type and the now-stale comments / display names changed; the paired valid-input tests are untouched and still pass. Testing: - mvn test (full suite): 8566 tests, 0 failures, 0 errors, 0 skipped.
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
1 participant
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
Bug fixes surfaced by an MS-SMB2 / MS-FSCC conformance review of the SMB2/3 client (
org.codelibs.jcifs.smb). Scope is limited to low-risk, high-confidence correctness / robustness fixes; larger architectural gaps found in the review (e.g. SMB3 transport encryption wiring, cancel/echo, multi-credit / large-MTU) are intentionally out of scope and left for follow-up.Correctness
Smb3KeyDerivation— SMB 3.1.1 encryption key labels wereSMB2C2SCipherKey/SMB2S2CCipherKey. The SP800-108 KDF requiresSMBC2SCipherKey/SMBS2CCipherKey; the stray2derives encryption keys that never match a real server. (SigningSMBSigningKeyand appSMBAppKeylabels were already correct.)Smb2ReadResponse—DataOffsetwas read as a signedbyte, so values ≥0x80became negative. Now read as an unsigned byte.FileBothDirectoryInfo.decode— returned a negative byte count (start - bufferIndex), violating theDecodablecontract. Now returnsbufferIndex - start. (Both call sites ignore the value, so no behavioral impact beyond the contract.)Robustness (server-controlled offset/length validation)
Validate offsets/lengths from the server before allocating or indexing, so a malicious/malformed server cannot trigger
ArrayIndexOutOfBoundsException/OutOfMemoryErroron the client:Smb2CreateResponse— validate create-context name/data offset+length.Smb2ReadResponse— validate data offset+length against packet bounds.Smb2QueryDirectoryResponse— reject a negative / out-of-rangeOutputBufferLengthinstead of decoding one bogus entry.FileBothDirectoryInfo— bound-check the file name length.Reliability
SmbFileOutputStream/SmbRandomAccessFile— throw instead of looping forever when the server reports a zero-length write with bytes still remaining.SmbFileOutputStream.close()— null-guard the handle (matching the input stream).SmbRandomAccessFile— fix theRemainingByteshint (bytes left in the request, not offset-adjusted).FileRenameInformation2.encode— explicitly zeroReserved/RootDirectoryinstead of relying on buffer pre-zeroing.SmbTransportImpl— dedicatedfinalmonitor for the reassigned preauth-integrity hash; apply topup credits to the current chain head; null-guardgetResponse()on the zero-credit path.Transport— pass the current request (not the chain head) tohandleIntermediateon the async wait path.Testing
mvn test— 8434 tests, 0 failures, 0 errors, 0 skipped. Tests updated for the corrected decode contract and the new decode-time validation.