Implement message signing #2
Conversation
Changed initialization of "sessionGlobalId" Changed type of "sessionGlobalId" to int Changed "Global.sessionTable" to use "sessionId"
Added assertions Matched dialect check closer to spec: Now uses "connection.dialect" and accepts any 3x dialect; note that in practice the dialect can only be "3.1.1" at this point Added missing conditions/execution paths related to "Global.isMultiChannelCapable" Externalized method "gssAuthenticate" and fixed control flow to it Implemented reauthentication further
Changed the flow for supplying the singing key/algorithm to "MessageSigner" and decoupled it from "Session" Changed flags and byte order for signature generation Updated the logic for setting the security mode and server capabilities during negotiation Added logic for triggering signing and selecting signing keys
Added missing update operations for "Session.preauthIntegrityHashValue" Added missing 0-bytes in labels
This allows handling of null values
Added dependency on bouncy castle Added test case This is required for integrations tests using SMBJ because the library only supports CMAC. See: https://github.com/cryptomator/jsmb/blob/132cba90a026a6f2b40da91059a4ff683ac3c1ab/src/test/java/org/cryptomator/jsmb/ConnectIT.java
This is required for integrations tests using SMBJ because the functionality is expected by the library. See: https://github.com/cryptomator/jsmb/blob/132cba90a026a6f2b40da91059a4ff683ac3c1ab/src/test/java/org/cryptomator/jsmb/ConnectIT.java
This is required for recreating packets originally sent by Samba.
Methodology for test vectors used in MessageSignerTest#testSigning @ c2c864b:The source captures were created/analyzed with the following setup:
The captures can be made available on request. |
WalkthroughThe diff introduces SMB2 message signing end-to-end (MessageSigner, SigningCapabilities enum, SMB2Message.sign), adds LOGOFF message types and Runtime.logoff, integrates signing and key derivation into negotiation and session setup using a NIST SP800-108 HMAC-SHA256 KDF, updates NTLMv2/NtlmSession to compute signing keys and return an Authenticated record, and updates TcpConnection to sign responses. It adds BouncyCastle and Guava dependencies plus a module require for BouncyCastle, changes session/global session handling (Srvs*), several header/session helpers, and new unit tests for MessageSigner and the KDF. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 6
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/jsmb/smb2/negotiate/SigningCapabilities.java (1)
12-32: Expose an enum-friendly builder overloadWe now have a strongly-typed
Algorithm, butbuildstill forces callers back to raw chars. Adding an overload keeps the API cohesive and removes the need for every caller to sprinklegetValue()conversions.public static SigningCapabilities build(char algId) { var data = MemorySegment.ofArray(new byte[4]); data.set(Layouts.LE_UINT16, 0, (char) 1); // signing algorithm count data.set(Layouts.LE_UINT16, 2, algId); // first element in algorithm list return new SigningCapabilities(data); } + + public static SigningCapabilities build(Algorithm alg) { + return build(alg.getValue()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.idea/runConfigurations/NtlmSessionTest.xml(0 hunks)pom.xml(3 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/jsmb/TcpConnection.java(4 hunks)src/main/java/org/cryptomator/jsmb/common/NTStatus.java(1 hunks)src/main/java/org/cryptomator/jsmb/ntlmv2/Authenticator.java(1 hunks)src/main/java/org/cryptomator/jsmb/ntlmv2/Crypto.java(1 hunks)src/main/java/org/cryptomator/jsmb/ntlmv2/NtlmSession.java(2 hunks)src/main/java/org/cryptomator/jsmb/smb1/SMB1Negotiator.java(3 hunks)src/main/java/org/cryptomator/jsmb/smb2/Connection.java(2 hunks)src/main/java/org/cryptomator/jsmb/smb2/Global.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/LogoffRequest.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/LogoffResponse.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java(9 hunks)src/main/java/org/cryptomator/jsmb/smb2/PacketHeader.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/PacketHeaderBuilder.java(2 hunks)src/main/java/org/cryptomator/jsmb/smb2/Runtime.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/SMB2Message.java(3 hunks)src/main/java/org/cryptomator/jsmb/smb2/SMB2MessageParser.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/Session.java(5 hunks)src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDF.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/negotiate/SigningCapabilities.java(1 hunks)src/main/java/org/cryptomator/jsmb/srvs/SrvsGlobal.java(1 hunks)src/main/java/org/cryptomator/jsmb/srvs/SrvsSession.java(1 hunks)src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java(1 hunks)src/test/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDFTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- .idea/runConfigurations/NtlmSessionTest.xml
🧰 Additional context used
🧬 Code graph analysis (9)
src/main/java/org/cryptomator/jsmb/smb1/SMB1Negotiator.java (1)
src/main/java/org/cryptomator/jsmb/smb2/Connection.java (1)
Connection(14-45)
src/main/java/org/cryptomator/jsmb/TcpConnection.java (3)
src/main/java/org/cryptomator/jsmb/smb2/Connection.java (1)
Connection(14-45)src/main/java/org/cryptomator/jsmb/smb2/SMB2MessageParser.java (1)
SMB2MessageParser(8-33)src/main/java/org/cryptomator/jsmb/smb2/Session.java (1)
Session(20-83)
src/main/java/org/cryptomator/jsmb/smb2/Session.java (1)
src/main/java/org/cryptomator/jsmb/srvs/SrvsGlobal.java (1)
SrvsGlobal(11-22)
src/test/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDFTest.java (1)
src/main/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDF.java (1)
NistSP800108KDF(20-64)
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java (3)
src/main/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDF.java (1)
NistSP800108KDF(20-64)src/main/java/org/cryptomator/jsmb/smb2/Session.java (1)
Session(20-83)src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
src/main/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDF.java (1)
src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java (2)
src/main/java/org/cryptomator/jsmb/smb2/Connection.java (1)
Connection(14-45)src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
src/main/java/org/cryptomator/jsmb/smb2/SMB2Message.java (2)
src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java (1)
MessageSigner(34-109)src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
src/main/java/org/cryptomator/jsmb/ntlmv2/NtlmSession.java (2)
src/main/java/org/cryptomator/jsmb/ntlmv2/Crypto.java (1)
Crypto(13-64)src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
🪛 ast-grep (0.39.6)
src/main/java/org/cryptomator/jsmb/ntlmv2/Crypto.java
[warning] 28-28: Detected MD5 hash algorithm which is considered insecure. MD5 is not collision resistant and is therefore not suitable as a cryptographic signature. Use HMAC instead.
Context: "MD5"
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-md5-java)
🔇 Additional comments (8)
src/main/java/org/cryptomator/jsmb/common/NTStatus.java (1)
13-13: LGTM!The new status constant is correctly defined and appropriately placed. The value matches the NT status code specification.
src/main/java/module-info.java (1)
4-4: LGTM!The BouncyCastle provider dependency is appropriate for implementing SMB2 message signing with AES-CMAC/AES-GMAC support. Based on learnings, version 1.80 provides robust cryptographic primitives for this use case.
src/main/java/org/cryptomator/jsmb/smb2/PacketHeaderBuilder.java (2)
18-21: LGTM!The copy constructor correctly initializes a mutable builder from an existing header. This pattern enables fluent header modification for signing operations.
74-74: LGTM!The signature offset at 48 is correct per the SMB2 packet header specification (16-byte signature field begins after the 8-byte SessionId at offset 40).
src/main/java/org/cryptomator/jsmb/ntlmv2/Crypto.java (1)
27-34: LGTM!The MD5 implementation is correct and follows the established pattern. The static analysis warning about weak hashing can be safely ignored here—MD5 is required by the NTLMv2 authentication protocol for HMAC-MD5 operations, not for general cryptographic signing.
src/main/java/org/cryptomator/jsmb/srvs/SrvsSession.java (1)
8-11: LGTM!The record definition is clean and the AtomicInteger ensures thread-safe session ID generation across the full integer range.
src/test/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDFTest.java (1)
9-29: LGTM!The test implementation is thorough and uses official NIST CAVS 14.4 test vectors for HMAC-SHA256 KDF validation. The parameterized approach provides good coverage across different key lengths and output sizes.
src/main/java/org/cryptomator/jsmb/smb1/SMB1Negotiator.java (1)
48-48: LGTM!The conditional security mode computation correctly enforces signing requirements based on the global configuration. The bitwise OR operation appropriately combines the SIGNING_ENABLED and conditional SIGNING_REQUIRED flags.
src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java
Outdated
Show resolved
Hide resolved
The tests are based on packet captures obtained from communication between two Samba instances. See: #2 (comment)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java (1)
75-115: Consider adding documentation for the CSV parameters and negative test cases.The parameterized test structure is solid and covers multiple signing algorithms. However, the CSV data is dense and would benefit from inline comments explaining each field (sessionId, expUnsignedHdrHex, securityBufferHex, etc.).
Additionally, consider adding negative test cases as future enhancements, such as:
- Signing with an incorrect key length
- Verifying signature mismatch detection
- Handling null or empty inputs
These additions would improve test coverage but are not blocking for the current implementation.
Example documentation approach:
@ParameterizedTest @CsvSource(textBlock = """ # sessionId, expUnsignedHdrHex, securityBufferHex, expUnsignedMsgHex, signingKeyHex, signingAlg, expSignedMsgHex 0x00000000ef9270f6,\ fe534d42...,\ ... """) public void testSigning( long sessionId, String expUnsignedHdrHex, String securityBufferHex, // ... rest of parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java
[high] 43-43: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java (2)
25-73: Excellent use of NIST test vectors.The tests correctly validate GMAC and CMAC implementations using standard NIST test vectors, which is a best practice for cryptographic code. The nonce construction in
testGmacSignatureis correct, and the test documentation provides proper traceability to the source vectors.Regarding the static analysis hint on line 43: This is a false positive. The flagged string is a published NIST CAVP test vector, not a real API key or secret.
117-123: Clean helper implementations.The
SignedMessagerecord andassertBytesEqualshelper method are well-designed. Using hex formatting in the assertion provides clear, readable error messages when tests fail.
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
Excellent work! Just tested it on macOS with Finder and Cyberduck and the Wireshark dumps look good.
Just some minor remarks.
| } else { | ||
| throw new UnsupportedOperationException("Not yet implemented"); | ||
| } |
There was a problem hiding this comment.
else branch should be trivial:
Else
Set ChallengeFromClient to NIL
EndIf
There was a problem hiding this comment.
This can never be the case for NTLMv2. So I'd keep it as it is for now.
There was a problem hiding this comment.
Then change "not yet implemented" to "NTLMv1 not supported" and add an inline comment to this very conversation (or to spec)
| connection.serverCapabilities |= SMB2_GLOBAL_CAP_ENCRYPTION; | ||
| if (connection.cipherId != EncryptionCapabilities.NO_COMMON_CIPHER) { | ||
| connection.serverCapabilities |= SMB2_GLOBAL_CAP_ENCRYPTION; | ||
| } |
There was a problem hiding this comment.
can you reference the specs in a comment? I don't understand why not agreeing on a common cipher requires us to "lie" about our capabilities.
There was a problem hiding this comment.
This section is all about the following provision made in §3.3.5.4 Receiving an SMB2 NEGOTIATE Request:
If Connection.CipherId is nonzero, the server MUST set the SMB2_GLOBAL_CAP_ENCRYPTION flag in Connection.ServerCapabilities.
First of all I put this section in the wrong place. 😅 The assignment is supposed to happen after the response has already been sent to the client and is therefore intended solely for the server. (In other words: "Lying" to the client was actually wrong.) This was fixed by a8c39f3.
§3.3.5.4 only makes one provision about sending SMB2_GLOBAL_CAP_ENCRYPTION to the client, but that's gated behind dialects other than 3.1.1 and is not related to this section.
So as far as I can tell this is what is going on:
- If we are using one of those earlier dialects we send the capability to indicate that we support encryption with
AES-128-CCM. (But again: That's unrelated to this section.) - If we are using dialect 3.1.1 we don't send the capability, but instead tell the client our chosen encryption algorithm using the
SMB2_ENCRYPTION_CAPABILITIESnegotiate context. - ... and then set
SMB2_GLOBAL_CAP_ENCRYPTIONas a "feature flag" for future use.
Resolved by a8c39f3.
src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java
Outdated
Show resolved
Hide resolved
Prior implementation mixed javax.crypto/Bouncy Castle in ways that led to unexpected behavior such as completely ignoring the "AES128" parameter in line 101. See: #2 (comment)
Clarified nullability semantics of usages of "SigningCapabilities.Algorithm" See: #2 (comment) Improved threadsafety of "SrvsGlobal.sessionList" See: #2 (comment) Added comments regarding the Bouncy Castle dependency and moved it to the correct position See: #2 (comment) See: #2 (comment) See: #4 Reformatted code See: #2 (comment)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/jsmb/smb2/Connection.java (1)
37-38: Past issue remains unaddressed: Initialize signingAlgorithmId to prevent NPE.The previous review correctly identified that changing
signingAlgorithmIdfromchartoAlgorithmremoves the implicit0x0000default. In SMB 2.x dialects where the server omitsSIGNING_CAPABILITIES, this field staysnulland will cause NPE in downstream code that callsgetValue()or uses it in a switch. While@Nullabledocuments the nullability, it doesn't solve the runtime failure.Apply the suggested fix from the previous review:
- @Nullable - public SigningCapabilities.Algorithm signingAlgorithmId; + public SigningCapabilities.Algorithm signingAlgorithmId = SigningCapabilities.Algorithm.HMAC_SHA256;
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/jsmb/srvs/SrvsGlobal.java (2)
21-21: Thread-safety addressed, but consider ConcurrentHashMap for better concurrency.The use of
Collections.synchronizedMapsuccessfully addresses the thread-safety concern from the previous review. However,ConcurrentHashMapremains the preferred choice for high-concurrency scenarios like an SMB server handling multiple connections, as it provides fine-grained locking and better read/write performance compared to the coarse-grained synchronization ofCollections.synchronizedMap.Apply this diff for optimal performance:
- public final Map<Integer, SrvsSession> sessionList = Collections.synchronizedMap(new HashMap<>()); //Map<GlobalSessionId,SrvsSession> instead of List to make lookup easier + public final Map<Integer, SrvsSession> sessionList = new ConcurrentHashMap<>(); //Map<GlobalSessionId,SrvsSession> instead of List to make lookup easierUpdate the import:
-import java.util.Collections; -import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap;
16-16: TODO: Removing entries from sessionList.The MS-SRVS specification section on removing entries is referenced but not yet implemented.
Would you like me to help implement the session removal logic based on the MS-SRVS specification, or should I open an issue to track this task?
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java (1)
182-182: Remove redundant assertionThis assertion is redundant—if assertions are enabled, it duplicates the prior check; if disabled, it provides no value. The assertion on line 174 already validates the dialect isn't null, and line 177 computes
dialect3x.- assert dialect3x;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pom.xml(2 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/jsmb/smb2/Connection.java(2 hunks)src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java(8 hunks)src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java(1 hunks)src/main/java/org/cryptomator/jsmb/srvs/SrvsGlobal.java(1 hunks)src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/cryptomator/jsmb/smb2/crypto/MessageSigner.java
- src/test/java/org/cryptomator/jsmb/smb2/crypto/MessageSignerTest.java
- pom.xml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JaniruTEC
Repo: cryptomator/jsmb PR: 2
File: src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java:256-271
Timestamp: 2025-10-20T18:14:02.309Z
Learning: In src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java, MS-SMB2 §3.3.5.5.3 does not contain provisions about signing, key derivation, or preauthIntegrityHashValue updates.
Learnt from: JaniruTEC
Repo: cryptomator/jsmb PR: 2
File: src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java:256-271
Timestamp: 2025-10-20T18:14:02.309Z
Learning: In src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java, the NtlmSession.Initial case (which returns an unsigned response) and the NtlmSession.AwaitingAuthentication case (which returns a signed response) are distinct session setup phases and should not be conflated. The response in the NtlmSession.Initial case is never signed.
📚 Learning: 2025-10-20T18:14:02.309Z
Learnt from: JaniruTEC
Repo: cryptomator/jsmb PR: 2
File: src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java:256-271
Timestamp: 2025-10-20T18:14:02.309Z
Learning: In src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java, MS-SMB2 §3.3.5.5.3 does not contain provisions about signing, key derivation, or preauthIntegrityHashValue updates.
Applied to files:
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.javasrc/main/java/org/cryptomator/jsmb/smb2/Connection.java
📚 Learning: 2025-10-20T18:14:02.309Z
Learnt from: JaniruTEC
Repo: cryptomator/jsmb PR: 2
File: src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java:256-271
Timestamp: 2025-10-20T18:14:02.309Z
Learning: In src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java, the NtlmSession.Initial case (which returns an unsigned response) and the NtlmSession.AwaitingAuthentication case (which returns a signed response) are distinct session setup phases and should not be conflated. The response in the NtlmSession.Initial case is never signed.
Applied to files:
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.javasrc/main/java/org/cryptomator/jsmb/smb2/Connection.java
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java (3)
src/main/java/org/cryptomator/jsmb/smb2/crypto/NistSP800108KDF.java (1)
NistSP800108KDF(20-64)src/main/java/org/cryptomator/jsmb/smb2/Session.java (1)
Session(20-83)src/main/java/org/cryptomator/jsmb/util/Bytes.java (1)
Bytes(3-28)
🔇 Additional comments (6)
src/main/java/module-info.java (1)
5-8: LGTM! Temporary dependency properly documented.The BouncyCastle dependency is appropriately marked as temporary with a clear TODO referencing issue #4 for future replacement. This aligns with the prior discussion and approval from the maintainer.
src/main/java/org/cryptomator/jsmb/smb2/Negotiator.java (5)
66-66: LGTM: Configurable signing requirementThe change to use
connection.global.requireMessageSigningmakes the signing requirement configurable, which is more flexible than a hardcoded value.
96-99: Good use of type-safe enumsReplacing raw constants with
SigningCapabilities.Algorithmenum values improves type safety and code clarity.
160-165: Well-documented encryption capability handlingThe comment clearly explains the dual provisions for
SMB2_GLOBAL_CAP_ENCRYPTIONand why the flag is set onconnection.serverCapabilitiesafter the response is built. The implementation correctly distinguishes between response capabilities (3.0/3.0.2 only) and internal connection state (needed for later checks).
247-259: Correct preauth hash handling in Initial phaseThe preauth integrity hash is correctly updated with both the request and response in the Initial negotiation phase. The response remains unsigned as expected for this phase.
266-266: No actionable concerns—the credit response value is intentional and validated.The 8192 credit response at line 266 is protocol-compliant and intentional. It appears in the test suite (line 98 of MessageSignerTest.java) as an expected value for the SESSION_SETUP response, and the code directly references Microsoft SMB2 documentation for this step. Per the SMB2 specification, credit allocation is determined by a vendor-specific algorithm; the server grants a larger credit pool (8192) to the authenticated client after successful authentication, versus minimal credits (1) during the initial negotiation phase. This resource-allocation pattern is standard practice in SMB2 implementations.
This PR implements signing of SMB2 messages, including the required negotiation and (calls to) crypto primitives.