smbj icon indicating copy to clipboard operation
smbj copied to clipboard

Performance improvement for unsigned packets (#817 fix)

Open manish-pai opened this issue 1 year ago • 9 comments
trafficstars

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

manish-pai avatar May 03 '24 08:05 manish-pai

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 avatar May 06 '24 12:05 hierynomus

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.

hierynomus avatar May 06 '24 12:05 hierynomus

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

manish-pai avatar May 08 '24 05:05 manish-pai

Is this PR not complete?

vairavlavy avatar Jun 17 '24 22:06 vairavlavy

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

vairavlavy avatar Jun 18 '24 04:06 vairavlavy

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.

manish-pai avatar Jun 27 '24 10:06 manish-pai

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

vairavlavy avatar Jul 09 '24 20:07 vairavlavy

💨

tooptoop4 avatar Sep 01 '24 11:09 tooptoop4

@hierynomus Can we assist somehow in getting this released?

dkocher avatar Sep 10 '24 12:09 dkocher