Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-encrypted case should be negotiated consistently when using dialect 3.1.1 #747

Closed
turcsanyip opened this issue Jan 17, 2023 · 7 comments · Fixed by #752
Closed

Non-encrypted case should be negotiated consistently when using dialect 3.1.1 #747

turcsanyip opened this issue Jan 17, 2023 · 7 comments · Fixed by #752

Comments

@turcsanyip
Copy link

SMBJ 0.11.x introduced SMB 3.x support with (optionally enabled) encryption feature, configurable via SmbConfig.Builder.withEncryptData(boolean).

When using 3.0 dialect, the encryption is negotiated properly with the server:

  • if withEncryptData is set to true, the library sends SMB2_GLOBAL_CAP_ENCRYPTION capability in the negotiate request to the server, and if the server also supports encryption (answers with the same capability), the client will use encryption
  • if withEncryptData is set to false (default), the library does not send the encryption capability and will not use encryption (regardless of the server's response)

So the library uses encryption only if both sides claimed that they support encryption during the protocol negotiation, which I believe the proper behaviour.

In case of 3.1.1 dialect, the library decides based on the servers response only and does not take into account whether the encryption was set via withEncryptData(true) and the capability was sent to the server, or not.

It works for the withEncryptData(true) case but leads to inconsistent behaviour with some servers when withEncryptData(false) (and therefore no encryption capability was sent from the client side):

  • Samba running on Ubuntu 20.04 does not send back encryption info in this case and smbj will not use encryption
  • Windows Server 2002 sends back the encryption info, and based on this smbj will turn on encryption (and the communication will be successful)
  • NetApp Files sends back the encryption info, and based on this smbj will turn on encryption but the communication will fail (it seems NetApp Files does not tolerate that the client does not claim encryption capability but then tries to use encryption, which sounds reasonable from the server's perspective)

The first 2 cases are just inconsistent but the 3rd one is a concrete failure in the communication between the client and the server.

For these reasons, I would suggest taking into account also the client side encryption setting (not just the server's response) when encryption is determined in case of 3.1.1 (similar to 3.0):
ConnectionContext. supportsEncryption()

    public boolean supportsEncryption() {
        SMB2Dialect dialect = negotiatedProtocol.getDialect();
        if (dialect == SMB2Dialect.SMB_3_1_1) {
            return cipherId != null; // clientCapabilities.contains(SMB2GlobalCapability.SMB2_GLOBAL_CAP_ENCRYPTION) should be checked here too
        } else {
            return clientCapabilities.contains(SMB2GlobalCapability.SMB2_GLOBAL_CAP_ENCRYPTION)
                && supports(SMB2GlobalCapability.SMB2_GLOBAL_CAP_ENCRYPTION);
        }
    }

Error stack trace (no useful info related to the root cause though):

12:02:09.704 [main] DEBUG com.hierynomus.smbj.connection.Connection - Granted 1 (out of 128) credits to Encrypted[SMB2_CREATE with message id << 4 >>]
12:02:09.704 [main] DEBUG com.hierynomus.smbj.transport.tcp.direct.DirectTcpTransport - Writing packet Encrypted[SMB2_CREATE with message id << 4 >>]
12:02:09.705 [main] DEBUG com.hierynomus.protocol.commons.concurrent.Promise - Awaiting << 4 >>
12:02:09.813 [Packet Reader for localhost] DEBUG com.hierynomus.smbj.transport.tcp.direct.DirectTcpPacketReader - Received packet Encrypted for session id << -7976719365003134876 >>
12:02:09.813 [Packet Reader for localhost] DEBUG com.hierynomus.smbj.connection.packet.SMB3DecryptingPacketHandler - Decrypting packet Encrypted for session id << -7976719365003134876 >>
12:02:09.814 [Packet Reader for localhost] DEBUG com.hierynomus.smbj.connection.packet.SMB3DecryptingPacketHandler - Decrypted packet Encrypted for session id << -7976719365003134876 >> is packet SMB2_CREATE with message id << 4 >>.
12:02:09.814 [Packet Reader for localhost] DEBUG com.hierynomus.smbj.connection.packet.SMB2SignatureVerificationPacketHandler - Passthrough Signature Verification as packet is decrypted
12:02:09.814 [Packet Reader for localhost] DEBUG com.hierynomus.smbj.connection.packet.SMB2CreditGrantingPacketHandler - Server granted us 1 credits for SMB2_CREATE with message id << 4 >>, now available: 128 credits
12:02:09.814 [Packet Reader for localhost] DEBUG com.hierynomus.protocol.commons.concurrent.Promise - Setting << 4 >> to `SMB2_CREATE with message id << 4 >>`
Exception in thread "main" com.hierynomus.mssmb2.SMBApiException: STATUS_ACCESS_DENIED (0xc0000022): Create failed for \\localhost\netapp-vol2
	at com.hierynomus.smbj.share.Share.receive(Share.java:380)
	at com.hierynomus.smbj.share.Share.sendReceive(Share.java:359)
	at com.hierynomus.smbj.share.Share.createFile(Share.java:156)
	at com.hierynomus.smbj.share.DiskShare.createFileAndResolve(DiskShare.java:75)
	at com.hierynomus.smbj.share.DiskShare.access$100(DiskShare.java:55)
	at com.hierynomus.smbj.share.DiskShare$2.apply(DiskShare.java:109)
	at com.hierynomus.smbj.share.DiskShare$2.apply(DiskShare.java:105)
	at com.hierynomus.smbj.paths.PathResolver$1.resolve(PathResolver.java:32)
	at com.hierynomus.smbj.paths.SymlinkPathResolver.resolve(SymlinkPathResolver.java:62)
	at com.hierynomus.smbj.share.DiskShare.resolveAndCreateFile(DiskShare.java:105)
	at com.hierynomus.smbj.share.DiskShare.open(DiskShare.java:65)
	at com.hierynomus.smbj.share.DiskShare.openDirectory(DiskShare.java:151)
	at ...
@hierynomus
Copy link
Owner

Unfortunately it seems that NetApp is inconsistent in the implementation of the MS-SMB2 spec. The reason it's implemented as it is now stems from these lines in the specification for the 3.1.1 dialect:

Processing the SMB2_ENCRYPTION_CAPABILITIES negotiate context

The client MUST return an error to the calling application in the following cases:

The DataLength of the negotiate context is less than the size of SMB2_ENCRYPTION_CAPABILITIES structure.

CipherCount is not 1.

Ciphers[0] is not 0 and not one of the ciphers that the client specified in its negotiate request.

The client MUST set Connection.CipherId to Ciphers[0] and if Server.CipherId is empty, set Server.CipherId to Ciphers[0].

If Connection.CipherId is nonzero, the client MUST set Connection.SupportsEncryption to TRUE. Otherwise, it MUST be set to FALSE.

Source: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/3b29f3af-86f9-4962-8cf3-43471cb59363

So, though I understand the confusion, this is actually pretty well specced out, and is in line with how it should behave.

I think the error here is on the server side, as that specifies the following behaviour:

If the server received an SMB2_ENCRYPTION_CAPABILITIES negotiate context in the client's negotiate request, the server MUST add an SMB2_ENCRYPTION_CAPABILITIES negotiate context to the response's NegotiateContextList. Note that the server MUST send an SMB2_ENCRYPTION_CAPABILITIES context even if the client and server have no common cipher. This is done so that the client can differentiate between a server that does not support encryption (no SMB2_ENCRYPTION_CAPABILITIES context in the response's NegotiateContextList) and a server that supports encryption but does not share a cipher with the client (an SMB2_ENCRYPTION_CAPABILITIES context in the response's NegotiateContextList that indicates a cipher of 0).

SMBJ will correctly interprete a 0 CipherId, however it seems that NetApp does not send a 0, but rather either 1 or 2. Can you check which Cipher IDs are sent? Samba also seems to misbehave in this sense, as the spec says they should, but luckily we just don't need to distinguish, so their implementation works.

@turcsanyip
Copy link
Author

@hierynomus Thanks for your detailed answer and also for the pointers to the related sections of the SMB specification (I read it carefully).
Now I understand that in case of 3.1.1, the encryption negotiation is based on the SMB2_ENCRYPTION_CAPABILITIES negotiate context solely. Earlier it was not clear for me if SMB2_GLOBAL_CAP_ENCRYPTION capability also matters or not when 3.1.1 is used (so it should not). Maybe Samba and NetApp implement the protocol improperly due to the same misunderstanding.
So it is fine, thanks for the clarification.

However, I still have a question regarding smbj: Why does the library always send the SMB2_ENCRYPTION_CAPABILITIES negotiate context and this way turns encryption on (supposed the server also supports it), even when withEncryptData = false?
I think the proper behaviour would be to send SMB2_ENCRYPTION_CAPABILITIES negotiate context only if withEncryptData has been set to true by the app using the library (that is the app is requesting encryption by setting the flag on the api).

@hierynomus
Copy link
Owner

Let me check, I don't think we should indeed if that's the case.

@hierynomus
Copy link
Owner

@turcsanyip Finally had some time to do some work on the lib again! Could you check whether the version from branch #752 works correctly for you?

@hannosgit
Copy link
Contributor

@hierynomus The fix from your branch seems to be working. I tested it with an on-premise NetApp.
I am using following SMB config:

private static final SmbConfig cfg = SmbConfig
        .builder()
        .withDialects(SMB2Dialect.SMB_3_1_1)
        .withEncryptData(false)
        .build();

With the current main branch I get:

Exception in thread "main" com.hierynomus.mssmb2.SMBApiException: STATUS_ACCESS_DENIED (0xc0000022): Create failed for \\10.180.1.10\shr\.
	at com.hierynomus.smbj.share.Share.receive(Share.java:380)
	at com.hierynomus.smbj.share.Share.sendReceive(Share.java:359)
	at com.hierynomus.smbj.share.Share.createFile(Share.java:156)
	at com.hierynomus.smbj.share.DiskShare.createFileAndResolve(DiskShare.java:75)
	at com.hierynomus.smbj.share.DiskShare.access$100(DiskShare.java:55)
	at com.hierynomus.smbj.share.DiskShare$2.apply(DiskShare.java:109)

But on the feature branch I do not get any exceptions and connecting is successful.
The log shows that a null cipher is used for encryption.

08:26:47.783 [main] INFO com.hierynomus.smbj.connection.PacketEncryptor - Initialized PacketEncryptor with Cipher << null >>
08:26:47.791 [main] INFO com.hierynomus.smbj.connection.Connection - Successfully connected to: 10.180.1.10
```

hierynomus added a commit that referenced this issue Mar 17, 2023
#752)

* Do not send SMB2EncryptionCapabilities NegotiationContext is !isEncryptionSupported (Fixes #747)

* Add test for SMB2EncryptionCapabilities
@turcsanyip
Copy link
Author

Thanks @hierynomus !

hierynomus added a commit that referenced this issue May 8, 2023
#752)

* Do not send SMB2EncryptionCapabilities NegotiationContext is !isEncryptionSupported (Fixes #747)

* Add test for SMB2EncryptionCapabilities
hierynomus added a commit that referenced this issue Jul 3, 2023
* Ignore VSCode

* Add NtStatus.STATUS_UNSUCCESSFUL

* Add NtStatus.STATUS_INSUFF_SERVER_RESOURCES (#611)

* Add NtStatus.STATUS_IO_REPARSE_TAG_NOT_HANDLED (#514)

* Update gradle build

* Update release plugin

* Release version: 0.11.0

* Fix signing task dependency

* Release version: 0.11.1

* Use BCSecurityProvider by default for SMB3 compatibility (Fixes #638)

* Ensure DFS Path Referral times out after transactTimeout (Fixes #578)

* Only add DFSPathResolver if both client and server support DFS (#640)

* Only add DFSPathResolver if both client and server support DFS

* Fix indentation problems

* Fix incorrectly reformatted javadoc

* Format using java formatter

* One more indentation fix

* Upgrade Bouncy Castle to 1.68 to fix vulnerability report (#641)

* address issue #604 - stop closing the dfs share connection immediately. (#609)

* stop closing the dfs share connection immediately.

* Add explanatory comment

Co-authored-by: Jeroen van Erp <[email protected]>

* Add support for unregistering server from serverlist (Fixes #644) (#647)

* Add support for unregistering server from serverlist (Fixes #644)

* Fix indentation

* Reducing logging for smb3 (#650)

For each smb3 packet there's an info log message which produces a tremendous amount of output.
I would suggest to reduce log level to debug (or trace; similarly as in one of the other packet reciever classes).

* Consolidate SMBv1 error messages

* Upgrade BouncyCastle to 1.69

* Release version: 0.11.2

* Ensure artifact is signed

* Release version: 0.11.3

* Fix #665: Allow JCE KDF to work (#666)

* Fix #665: Allow JCE KDF to work

* Add header

* Add KDF unit test

* Use correct maxPayloadSize for encrypted packets (Fixes #668) (#683)

* Read fileId as long (#693)

* Read fileId of FileIdBothDirectoryInformation into a long
* Read fileId of FileIdFullDirectoryInformation into a long

* File the issue that nested folder creation throw NAME EXIST error. (#685)

* File the issue that nested folder creation throw NAME EXIST error.

* formatting

* Updated build status badges (#684)

- Added GitHub badge for Build SMBJ
- Corrected Codacy badge link
- Removed bintray badge link
- Removed Travis CI badge and configuration
- Removed Java profiler link
- Removed CircleCI configuration
- Removed unused github-ci configuration

* Use AceSize field when reading ACEs (#696)

Fixes issue seen in the wild where unnecessary padding at the end of an ACE
confused Smbj (but not Windows).

* Ensure that enough bytes are cached from InputStream to get a correct bytesLeft count for SMB2Write (fixes #669)

* GzipOutputStream integration test

* Ensure that enough bytes are cached from InputStream to get a correct bytesLeft count for SMB2Write

Co-authored-by: Stanislav Kardashov <[email protected]>
Co-authored-by: Jeroen van Erp <[email protected]>

* Add GH workflow for publishing

* Update dependencies and build file

* Rename test class to *Spec

* Release version: 0.11.5

* Use the hostname part of the TargetHint for DFS step 9 (fixes \#419) (#722)

* Slightly reduce the locking in Connection.send and DirectTcpTransport (fixes \#732)

* Fixed indentation

* Converting bytes written to long (Fixes #740)

Signed-off-by: Jeroen van Erp <[email protected]>

* Upgrading gradle to 8.0.2

Signed-off-by: Jeroen van Erp <[email protected]>

* Add Implementation manifest attributes (Fixes #743)

* Revert accidental comment of integration docker tasks

* Do not send SMB2EncryptionCapabilities NegotiationContext is !isEncry… (#752)

* Do not send SMB2EncryptionCapabilities NegotiationContext is !isEncryptionSupported (Fixes #747)

* Add test for SMB2EncryptionCapabilities

* Add preliminary changelog for new release

* Ensure we call flip() on Buffer to avoid Java8 problems (Fixes #705)

Signed-off-by: Jeroen van Erp <[email protected]>

* Ensure path is set for rmdir to prevent accidents (Fixes #756)

Signed-off-by: Jeroen van Erp <[email protected]>

* Add support for reading / writing NIO ByteBuffers (#759)

* Add support for reading / writing NIO ByteBuffers

Currently one can transfer data using streams or array, but it would be
great to have the opportunity to use NIO buffers.

This adds two new method to the File class that accept a NIO ByteBuffer.

* Implemented ByteBuffer write using ByteChunkProvider

Signed-off-by: Jeroen van Erp <[email protected]>

---------

Signed-off-by: Jeroen van Erp <[email protected]>
Co-authored-by: Christoph Läubrich <[email protected]>
Co-authored-by: Jeroen van Erp <[email protected]>

* Fix some sonatype warnings

* resolve conflict with master

* Ignore non-semver tags for release workflow

* Small warning cleanup

Signed-off-by: Jeroen van Erp <[email protected]>

* Setup ConnectionContext and AuthenticationContext for NTLM improvements

Signed-off-by: Jeroen van Erp <[email protected]>

* Refactor TargetInfo/AvPairs

* Added null check and rename field

* Refactor NtlmFunctions

* Change hierarchy of Ntlm messages

Signed-off-by: Jeroen van Erp <[email protected]>

* Next step of NTLM refactor

* NtlmNegotiate sends Domain/Workstation/Version fields

* Filter negotiateflags and use clientTargetInfo

* Rework keys in NtlmAuthenticator

Signed-off-by: Jeroen van Erp <[email protected]>

* Change to structure of NtlmAuthenticate

Signed-off-by: Jeroen van Erp <[email protected]>

* Added last changes

Required to put withIntegrity = false still, due to missing mechListMIC

Signed-off-by: Jeroen van Erp <[email protected]>

---------

Signed-off-by: Jeroen van Erp <[email protected]>
Co-authored-by: Nicholas DiPiazza <[email protected]>
Co-authored-by: ndimitry <[email protected]>
Co-authored-by: Patrick Boyd <[email protected]>
Co-authored-by: Hannes <[email protected]>
Co-authored-by: pyzhou <[email protected]>
Co-authored-by: exceptionfactory <[email protected]>
Co-authored-by: Chris Pacejo <[email protected]>
Co-authored-by: Stanislav Kardashov <[email protected]>
Co-authored-by: Stanislav Kardashov <[email protected]>
Co-authored-by: Christoph Läubrich <[email protected]>
Co-authored-by: Christoph Läubrich <[email protected]>
hellivan added a commit to solunio/smbj that referenced this issue Mar 21, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 22, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 22, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 25, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 26, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 26, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 26, 2024
hellivan added a commit to solunio/smbj that referenced this issue Mar 26, 2024
@SaurabhNITK1
Copy link

@hierynomus @turcsanyip

If we enable encryption on the SMBJ client side by setting .withEncryptData(true). At the same time on the server side executing "Get-SmbServerConfiguration" tells that EncryptData : False means server has not enabled encryption.
In this scenario, will the packets be encrypted? If not will the transfer happen without encryption or will it fail ?

I tried this configuration and while debugging I found out that during client - server negotiation, even if the server side encryption is off it was sending the encryption key and during the negotiation client -server were successfully able to negotiate encryption. I want to know will this be true for all use cases if the server supports SMB3.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants