Bug 14210 - smbd NULL pointer de-reference in smb2_signing_decrypt_pdu
Summary: smbd NULL pointer de-reference in smb2_signing_decrypt_pdu
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.11.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-29 17:48 UTC by Andrew Bartlett
Modified: 2020-11-24 11:17 UTC (History)
4 users (show)

See Also:


Attachments
ASAN Log (3.70 KB, text/plain)
2019-11-29 17:48 UTC, Andrew Bartlett
no flags Details
Patch disabling gnutls randomization (2.50 KB, patch)
2019-11-29 21:00 UTC, Robert Swiecki
no flags Details
Samba config used (717 bytes, text/plain)
2019-11-29 21:00 UTC, Robert Swiecki
no flags Details
Repro (input to be sent to smb port) (1.87 KB, application/octet-stream)
2019-11-29 21:02 UTC, Robert Swiecki
no flags Details
git-am fix for master. (1.91 KB, patch)
2020-11-13 22:29 UTC, Jeremy Allison
metze: review-
Details
Updated git-am fix for master. (1.72 KB, patch)
2020-11-15 00:14 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 4.12.next, 4.13.next. (1.95 KB, patch)
2020-11-16 17:06 UTC, Jeremy Allison
vl: review+
jra: review? (metze)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-11-29 17:48:01 UTC
Created attachment 15652 [details]
ASAN Log

Reported by Robert Święcki on another bug, re-published here so we can track separately.
Comment 1 Robert Swiecki 2019-11-29 20:59:34 UTC
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).
Comment 2 Robert Swiecki 2019-11-29 21:00:10 UTC
Created attachment 15653 [details]
Patch disabling gnutls randomization
Comment 3 Robert Swiecki 2019-11-29 21:00:54 UTC
Created attachment 15654 [details]
Samba config used
Comment 4 Robert Swiecki 2019-11-29 21:02:59 UTC
Created attachment 15655 [details]
Repro (input to be sent to smb port)
Comment 5 Robert Swiecki 2019-11-29 21:05:13 UTC
(In reply to Robert Swiecki from comment #1)

And, it requires at least debug level 1
Comment 6 Robert Swiecki 2019-11-29 22:36:31 UTC
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 }
Comment 7 Andrew Bartlett 2020-11-11 17:30:12 UTC
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)?
Comment 8 Jeremy Allison 2020-11-13 22:04:55 UTC
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.
Comment 9 Jeremy Allison 2020-11-13 22:29:53 UTC
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 10 Stefan Metzmacher 2020-11-14 17:05:02 UTC
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.
Comment 11 Jeremy Allison 2020-11-15 00:13:12 UTC
Thanks Metze, that's why we pay you the big bucks :-). Updated patch incoming. I'll also push an MR to github.
Comment 12 Jeremy Allison 2020-11-15 00:14:20 UTC
Created attachment 16337 [details]
Updated git-am fix for master.
Comment 13 Stefan Metzmacher 2020-11-15 17:00:53 UTC
Comment on attachment 16337 [details]
Updated git-am fix for master.

Thanks!
Comment 15 Jeremy Allison 2020-11-16 04:42:03 UTC
I think we can remove the embargo on this.
Comment 16 Samba QA Contact 2020-11-16 09:48:06 UTC
This bug was referenced in samba master:

26ba04a4d1987a859152751e6083d9b9aef770ff
Comment 17 Jeremy Allison 2020-11-16 17:06:41 UTC
Created attachment 16338 [details]
git-am fix for 4.12.next, 4.13.next.

Cherry-picked from master.
Comment 18 Karolin Seeger 2020-11-19 08:52:17 UTC
(In reply to Jeremy Allison from comment #17)
Pushed to autobuild-v4-{13,12}-test.
Comment 19 Samba QA Contact 2020-11-19 11:25:06 UTC
This bug was referenced in samba v4-13-test:

c58689c9aad8929fa6d16c2a9eb520259be9395d
Comment 20 Samba QA Contact 2020-11-19 14:10:06 UTC
This bug was referenced in samba v4-12-test:

8136ade13f8236ea10f5a2b59b5cb117f3298d76
Comment 21 Karolin Seeger 2020-11-24 11:17:59 UTC
Pushed to both branches.
Closing out bug report.

Thanks!