Bug 14189 - Encryption Session Id 0xFFFFFFFFFFFFFFFF Change Notify
Summary: Encryption Session Id 0xFFFFFFFFFFFFFFFF Change Notify
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.9.1
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-06 21:53 UTC by Jordan
Modified: 2019-12-17 12:02 UTC (History)
2 users (show)

See Also:


Attachments
Samba debug log and Wireshark packet capture (53.74 KB, application/x-xz)
2019-11-06 21:53 UTC, Jordan
no flags Details
raw patch for master. (573 bytes, patch)
2019-11-07 01:01 UTC, Jeremy Allison
no flags Details
Updated raw patch for master. (1.01 KB, patch)
2019-11-07 01:05 UTC, Jeremy Allison
no flags Details
3rd time is the charm :-). (2.81 KB, patch)
2019-11-07 01:23 UTC, Jeremy Allison
no flags Details
git-am fix for master I have in CI. (6.10 KB, patch)
2019-11-07 17:33 UTC, Jeremy Allison
no flags Details
Minimal fix metze requested. (1.03 KB, patch)
2019-11-07 20:42 UTC, Jeremy Allison
no flags Details
git-am fix for 4.11.next, 4.10.next (1.12 KB, patch)
2019-11-18 17:54 UTC, Jeremy Allison
jra: review? (metze)
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan 2019-11-06 21:53:29 UTC
Created attachment 15601 [details]
Samba debug log and Wireshark packet capture

Hi

When sending an encrypted compound message that contains a Create and Change Notify request to a Samba server the response back is in 2 Transform headers. The Create response has the correct Session Id set but the pending response for the Change Notify has a Session id of 0xFFFFFFFFFFFFFFFF. Because of this the client is unable to decrypt the response as it does not know what session key's to use for decryption.

This is not an issue for Windows servers as they set the correct Session Id on the Change Notify pending response. I originally thought Windows was also affected but the Transform Header response for this exchange contains the correct Session Id on that OS. I've got a slightly related thread on MSDN about that https://social.msdn.microsoft.com/Forums/en-US/a580f7bc-6746-4876-83db-6ac209b202c4/mssmb2-change-notify-response-sessionid?forum=os_fileservices.

The version I am running on is the latest in Centos 7, the rpm -q output is 'samba-4.9.1-6.el7.x86_64'. You can see my setup scripts for this server at https://github.com/jborean93/smbprotocol/tree/easy-api/build-scripts.

I've attached a file that contains 3 files:

1. change_notify_encryption.pcapng - The Wireshark capture of the process. 
2. change_notify_plaintext.pcapng - The Wireshark capture of a plaintext version of 1 to show off the actual requests that client is sending without encryption (not in the logs and works)
3. log.smbd - The debug log when associated with the encryption network capture ('2019/11/06 20:39:56.994865' is when Samba receives the compound request)

The Wireshark encrypted capture is mostly useless as the packets are encrypted from the Session Setup forward but here are the important frames and you can see the plaintext messages for each frame by looking at 'change_notify_plaintext.pcapng'.

* 24: The compound request of the Create and Change Notify messages from the client
* 25: The Create response from the request above
* 26: The Change Notify response from the request above but the SessionId is 0xFFFFFFFFFFFFFFFF on the encrypted header (this is the issue)

You may notice in the plaintext capture that the pending Notify Response sent after the compound request has a Session id of '0xFFFFFFFFFFFFFFFF' in the header. This is the same behaviour as Windows SMB but because we have access to the Message Id we can get the Session Id from the corresponding request (see the MSDN link for more info there).

Please let me know if you need any more information about this, I'm happy to supply whatever is needed to try and solve this.

Thanks
Comment 1 Jeremy Allison 2019-11-07 00:26:56 UTC
We're just copying the incoming Session id here, instead of copying the session id from the previous request.
Comment 2 Jeremy Allison 2019-11-07 01:01:18 UTC
Created attachment 15603 [details]
raw patch for master.

Can you check this (completely untested but hey it compiles :-) patch ?

I think it might fix the incorrect sessionid.

Thanks !

Jeremy.
Comment 3 Jeremy Allison 2019-11-07 01:05:31 UTC
Created attachment 15604 [details]
Updated raw patch for master.

Sorry, I think that previous patch was looking at the wrong flags (outhdr not inhdr flags).

Can you try this one instead ?
Comment 4 Jeremy Allison 2019-11-07 01:23:33 UTC
Created attachment 15605 [details]
3rd time is the charm :-).

OK, so here is a patch for master I'm happier with, as it makes the transform header sending code in:

smbd_smb2_request_pending_timer()

match the transform header sending code in:

smbd_smb2_request_reply().

Can you check this one ? I think it should make us match Windows now.

Cheers,

Jeremy.
Comment 5 Jordan 2019-11-07 02:49:50 UTC
Awesome thanks for the quick turnaround. I've applied that to master and can confirm that the SMB2 Transform header of the reply contains the correct Session Id and my client is able to decrypt it.
Comment 6 Jeremy Allison 2019-11-07 17:33:03 UTC
Created attachment 15612 [details]
git-am fix for master I have in CI.

Adding it here so I can keep track.
Comment 7 Jeremy Allison 2019-11-07 20:42:02 UTC
Created attachment 15614 [details]
Minimal fix metze requested.
Comment 8 Jeremy Allison 2019-11-18 17:54:01 UTC
Created attachment 15625 [details]
git-am fix for 4.11.next, 4.10.next

Cherry-pick from master. Applies cleanly to 4.11.next, 4.10.next.
Comment 9 Jeremy Allison 2019-12-11 19:24:47 UTC
Ralph ping !
Comment 10 Ralph Böhme 2019-12-11 19:46:46 UTC
Reassigning to Karolin for inclusion in 4.10 and 4.11.
Comment 11 Karolin Seeger 2019-12-13 10:03:47 UTC
(In reply to Ralph Böhme from comment #10)
Pushed to autobuild-v4-{11,10}-test.
Comment 12 Karolin Seeger 2019-12-17 12:02:01 UTC
Pushed to both branches.
Closing out bug report.

Thanks!