search

hierynomus / smbj

Server Message Block (SMB2, SMB3) implementation in Java
Other
705 stars 179 forks source link

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

Open manish-pai opened 3 months ago

manish-pai commented 3 months ago

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 https://github.com/hierynomus/smbj/issues/817

hierynomus commented 3 months ago

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 commented 3 months ago

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.

manish-pai commented 3 months ago

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

vairavlavy commented 2 months ago

Is this PR not complete?

vairavlavy commented 2 months ago

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

manish-pai commented 2 months ago

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 commented 1 month ago

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

tooptoop4 commented 5 days ago

💥