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

Performance improvement for unsigned packets (#817 fix) #824

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manish-pai
Copy link

The Session.send() will check if the server has set RequireSecuritySignature in the Negotiate Protocol Response. If server and client do not wish to sign packets, an unsigned packet will be sent.

This should address #817

The Session.send() will check if the server has set RequireSecuritySignature in the Negotiate Protocol Response. If server and client do not wish to sign packets, an unsigned packet will be sent. This should address hierynomus#817
@manish-pai manish-pai mentioned this pull request May 3, 2024
@hierynomus
Copy link
Owner

hierynomus commented May 6, 2024

It's always nice that the guys at MS write a spec (MS-SMB2) and then a tech-note, which to some extend contradict eachother...

From section 3.2.4.1.1 Signing the Message

The client MUST sign the message if one of the following conditions is TRUE:
- If Connection.Dialect is equal to "2.0.2" or "2.1", the message being sent contains a nonzero
value in the SessionId field and the session identified by the SessionId has
Session.SigningRequired equal to TRUE.
- If Connection.Dialect belongs to 3.x dialect family, the message being sent contains a nonzero
value in the SessionId field and one of the following conditions is TRUE:
- The session identified by SessionId has Session.EncryptData equal to FALSE.
- The tree connection identified by the TreeId field has TreeConnect.EncryptData equal to
FALSE.

If Session.SigningRequired is FALSE, the client MAY<99> sign the request.

@hierynomus
Copy link
Owner

That means that your implementation is too simple unfortunately. It doesn't take into account the SMB3 requirements which are now implicitly covered because we just "always sign if we can"...

Also it feels like "sane" behaviour to sign when we can, and make it explicit in the client to not sign. Implicitly offering up security over performance is rarely a good idea.

The signing can be only avoided for smb2 protocol when the smb server
has required signing false.
@manish-pai
Copy link
Author

Thanks for the feedback. It basically means for SMB 3.x, either you encrypt or sign. I've incorporated the changes.

@vairavlavy
Copy link

Is this PR not complete?

@vairavlavy
Copy link

Please complete this PR ASAP as I need the exact same feature

@manish-pai
Copy link
Author

I believe that If the server does not expect client packets to be signed, it would not validate the integrity even if we sign it. Based on this assumption, I've made these changes.

@vairavlavy
Copy link

Please complete this PR ASAP as I need the exact same feature

@tooptoop4
Copy link

💨

@dkocher
Copy link

dkocher commented Sep 10, 2024

@hierynomus Can we assist somehow in getting this released?

@gpsmit
Copy link

gpsmit commented Nov 6, 2024

@hierynomus what is the state of this PR by @manish-pai ?
we have a corporate environment where windows also doesn't sign the packets, we are getting nowhere near the speeds regular windows clients are getting because this library always signs regardless of our requests not to sign

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 this pull request may close these issues.

6 participants