Bug 14833 (CVE-2022-32743) - CVE-2022-32743 [SECURITY] Validated dnsHostname write right needs to be implemented
Summary: CVE-2022-32743 [SECURITY] Validated dnsHostname write right needs to be imple...
Status: ASSIGNED
Alias: CVE-2022-32743
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.15.0rc7
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Joseph Sutton
QA Contact: Samba QA Contact
URL:
Keywords:
: 14885 (view as bug list)
Depends on:
Blocks: CVE-2020-25722
  Show dependency treegraph
 
Reported: 2021-09-14 05:19 UTC by Andrew Bartlett
Modified: 2022-06-14 00:10 UTC (History)
2 users (show)

See Also:


Attachments
WIP patch for master (27.95 KB, patch)
2022-06-01 04:42 UTC, Joseph Sutton
no flags Details
proposed patch for master v2 (83.14 KB, patch)
2022-06-09 08:35 UTC, Joseph Sutton
abartlet: review-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2021-09-14 05:19:45 UTC
The e Validated-DNS-Host-Name right, which should apply only apply to objectclass=computer, per

MS-ADTS 3.1.1.5.3.1.1.2 dNSHostName
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/5c578b15-d619-408d-ba17-380714b89fd1

Is not implemented in Samba.  It appears that any value might be able to be written over NETLOGON via dcesrv_netr_LogonGetDomainInfo rather than being put via this right. 

This validated update would restrict values to <samAccountName-without-$>.<dnsdomainname>.

This matters as we will be linking a trailing $ to objectclass=computer and this helps avoid creation of SPN values that collide with other, possibly privileged, hosts.
Comment 1 Andrew Bartlett 2021-10-18 16:57:26 UTC
A top level bug 14564 / CVE-2020-25722 will be used for these related issues.
Comment 2 Andrew Bartlett 2022-05-25 13:58:35 UTC
https://research.ifcr.dk/certifried-active-directory-domain-privilege-escalation-cve-2022-26923-9e098fe298f4?gi=79eaa17b1dc8 says that now:

Furthermore, the “Validated write to DNS host name” permission now only allows setting a dNSHostName attribute that matches the SAM Account Name of the account.

We should catch up and implement this before it bites us.
Comment 3 Joseph Sutton 2022-06-01 04:42:34 UTC
Created attachment 17315 [details]
WIP patch for master

A proposed patch with tests.
Comment 4 Andrew Bartlett 2022-06-01 04:53:31 UTC
Comment on attachment 17315 [details]
WIP patch for master

The next step needed is to handle the modification via NETLOGON, to ensure it operates via this validated write, not SYSTEM.
Comment 5 Andrew Bartlett 2022-06-07 09:03:08 UTC
*** Bug 14885 has been marked as a duplicate of this bug. ***
Comment 6 Andrew Bartlett 2022-06-07 09:24:46 UTC
Because the update in dcesrv_netr_LogonGetDomainInfo() sets an SPN that will conflict with the primary SPN of any legitimate service, I think we dodge a security hole here, just.

It then becomes more like a littering issue, and we have other places (DNS records) where garbage data can be written. 

Perhaps some auditing tool is using dnsHostName, but Samba doesn't use it much, other than to control SPN values. 

So I think we should, over the next day or so, think about this and potentially remove the embargo.

If do do that we should propose this as a public patch and go though the normal process (unless we come up with something worse of course).

I seek other views!
Comment 7 Joseph Sutton 2022-06-09 08:35:05 UTC
Created attachment 17333 [details]
proposed patch for master v2
Comment 8 Andrew Bartlett 2022-06-09 22:02:31 UTC
After some discussion around the office, it is hard to know what the impact of this would be on individual deployments that might rely on dnsHostName so we will do the full CVE and security release thing here.
Comment 9 Joseph Sutton 2022-06-12 23:12:42 UTC
Note that the patchset from https://bugzilla.samba.org/show_bug.cgi?id=15008 must be applied prior to this one to avoid segfaults.
Comment 10 Andrew Bartlett 2022-06-13 09:39:53 UTC
Comment on attachment 17333 [details]
proposed patch for master v2

[PATCH 02/14] tests/py_credentials: Add tests for setting dNSHostName
 with LogonGetDomainInfo()

+    # Ensure we can't use LogonGetDomainInfo to update dNSHostName to an
+    # invalid value, even with rights.

Can this test be split in two to check these rights (WP and validated write) individually?

[PATCH 03/14] dsdb: Implement validated dNSHostName write

This in the acl_check_dns_host_name() looks like a fail open, which seems bad to me:

+
+		if (oc_el == NULL) {
+			/* Not a computer or server, so no need to validate. */
+			talloc_free(tmp_ctx);
+			return LDB_SUCCESS;
+		}
+	}


I'm worried we might be in the 'add' case and have yet a further subclass of computer (eg the attacker controls search_res) or just that for some reason this right is applied on some other type of object.

 [PATCH 05/14] dsdb/modules/acl: Handle
 VALIDATED_DNS_HOSTNAME_SPN_WRITE control

Some of us don't really like boolean parameters, and I think in this case the complaint is justfied.  I think pass validated_write as a typed paramter called implicit_validated_write_control (and I think we should rename the control to be FORCE_ALLOW_VALIDATED_DNS_HOSTNAME_SPN_WRITE).

 [PATCH 13/14] s4:rpc_server/common: Add
 dcesrv_samdb_connect_session_info()

Ensure that both session_info values are overwritten, we don't want the 'transport' session info of anonymous in the logs, via audit_session_info 
+	if (session_info != NULL) {
+		ldb_session_info = session_info;
 		audit_session_info = user_session_info;
Comment 11 Andrew Bartlett 2022-06-13 09:41:54 UTC
(In reply to Andrew Bartlett from comment #8)
Given the constraints enforced in the NETLOGON server, thankfully there is less opportunity for abuse than I had feared, but the dnsHostName suffix can be changed freely.
Comment 12 Andrew Bartlett 2022-06-14 00:10:41 UTC
Removing embargo.  The constraints in comment 11 mean this isn't worth doing the full security dance for, we should just do a normal public bug and MR process for this.

We keep the CVE tag so that those concerned about the low-severity issue can track it, and in case further concerns are found later.