btw,

I can reproduce it with disabled randomization only (I can make the patch available), as it's related to crypto code, which in turn uses a source randomness.

I'll also attach my config and the repro file (which should be sent to samba port).

Created attachment 15653 [details]
Patch disabling gnutls randomization

Created attachment 15654 [details]
Samba config used

Created attachment 15655 [details]
Repro (input to be sent to smb port)

(In reply to Robert Swiecki from comment #1)

And, it requires at least debug level 1

Looking at the code, the problem can be that the signing_key might be NULL, and dereferencing decryption_key->blob.length fails

616     if (!smb2_signing_key_valid(decryption_key)) {
617         DBG_WARNING("Wrong decryption key length %zu for SMB2 signing\n",
618                 decryption_key->blob.length);
619         return NT_STATUS_ACCESS_DENIED;
620     }

while smb2_signing_key_valid() checks for whether it's NULL

 51 bool smb2_signing_key_valid(const struct smb2_signing_key *key)
 52 {
 53     if (key == NULL) {
 54         return false;
 55     }
 56 
 57     if (key->blob.length == 0 || key->blob.data == NULL) {
 58         return false;
 59     }
 60 
 61     return true;
 62 }

Jeremy,

Can you look into this and work out if this is still an issue and if it is more than a self-DOS (so we know if we need to keep it embargoed)?

OK, looking at this it seems to be a transform header added to a compound request before a user logon or connection set up. We catch the error, but then try and do a DEBUG print that wants to deref the crypto blob.length field (which doesn't exist as the blob is NULL).

Looks to me to be a harmless self-DoS (spin up a server to crash it). I can see any security implications or need for a CVE.

I'll create a patch for this, but IMHO we can unembargo this and fix in the open.

Created attachment 16336 [details]
git-am fix for master.

Here's the (minimal) fix for this. I went through all code paths using smb2_signing_key_valid() and I think these are the only places that can crash if given a NULL signing blob (and only in the debug statements).

Metze can you review ? If you're good with my analysis I'll just push as a normal MR.

Comment on attachment 16336 [details]
git-am fix for master.

+		if (signing_key == NULL) {
+			DBG_WARNING("No signing key for SMB2 signing\n");
+			return NT_STATUS_ACCESS_DENIED;
+		}
 		DBG_WARNING("Wrong session key length %zu for SMB2 signing\n",
 			    signing_key->blob.length);

I'd just change this to something like this in all places:

-		DBG_WARNING("Wrong session key length %zu for SMB2 signing\n",
-			    signing_key->blob.length);
+		DBG_WARNING("No signing key for SMB2 signing\n");

There's no need to ever print length, it will never be different from 0.

Thanks Metze, that's why we pay you the big bucks :-). Updated patch incoming. I'll also push an MR to github.

Created attachment 16337 [details]
Updated git-am fix for master.

Comment on attachment 16337 [details]
Updated git-am fix for master.

Thanks!

https://gitlab.com/samba-team/samba/-/merge_requests/1684

I think we can remove the embargo on this.

This bug was referenced in samba master:

26ba04a4d1987a859152751e6083d9b9aef770ff

Created attachment 16338 [details]
git-am fix for 4.12.next, 4.13.next.

Cherry-picked from master.

(In reply to Jeremy Allison from comment #17)
Pushed to autobuild-v4-{13,12}-test.

This bug was referenced in samba v4-13-test:

c58689c9aad8929fa6d16c2a9eb520259be9395d

This bug was referenced in samba v4-12-test:

8136ade13f8236ea10f5a2b59b5cb117f3298d76

Pushed to both branches.
Closing out bug report.

Thanks!

This bug was referenced in samba v4-13-stable (Release samba-4.13.3):

c58689c9aad8929fa6d16c2a9eb520259be9395d

This bug was referenced in samba v4-12-stable (Release samba-4.12.11):

8136ade13f8236ea10f5a2b59b5cb117f3298d76