Bug 14785 - [SECURITY] PKINIT login should not do $ extension
Summary: [SECURITY] PKINIT login should not do $ extension
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.15.0rc1
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL: https://msrc.microsoft.com/update-gui...
Keywords:
Depends on:
Blocks: 14781
  Show dependency treegraph
 
Reported: 2021-08-06 11:36 UTC by Andrew Bartlett
Modified: 2022-06-26 22:41 UTC (History)
2 users (show)

See Also:


Attachments
patch (againt old master) for this issue (7.03 KB, patch)
2022-04-13 01:49 UTC, Andrew Bartlett
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2021-08-06 11:36:05 UTC
The principal lookup code in Heimdal and MIT should pass in a flag to say that the user principal is being asserted from a PKINIT X.509 certificate and therefore not do the $ alias step in the client lookup.

We can reasonably assume (I hope) that certificates will be issued to the right name.
Comment 1 Andrew Bartlett 2021-09-23 03:25:05 UTC
Robbie,

Please make sure that MIT Kerberos provides a flag to Samba to say the ticket came from PKINIT, so we can distinguish this case.

Don't say why (publicly) for now, but if you could add that flag it would mitigate name confusion attacks here.
Comment 2 Alexander Bokovoy 2021-09-23 07:43:04 UTC
According to MS-PAC 2.6, if PKINIT is used in AD environment with MS-PKCA, MS-KILE-compatible KDC should add PAC_CREDENTIAL_INFO structure. This is one of indicators that PKINIT was used and should cover AD case for TGS, I think.

Judging by the mit_samba.c, we have support to add the PAC_CREDENTIAL_INFO already but it is switched off as we don't know what to use to trigger its addition in MIT.

In Heimdal case wdc_samba.c expects that Heimdal KDC will send a pk_reply_key to the PAC generation code in the case this was a PKINIT.

In MIT, perhaps we can get access to the chosen PA data to see its type? In handle_authdata() this could probably be achieved but I am lost a bit in deciphering how to reach list of PA data returned from AS REQ.
Comment 3 Andrew Bartlett 2021-10-18 16:43:20 UTC
Moving to being a task under bug 14561.  Patches will be tagged with CVE-2020-25719 as this is essentially that race - a PKINIT login has by definition only a name, so it just like a PAC-less race to the target server.
Comment 4 Andrew Bartlett 2021-10-19 23:29:02 UTC
We should also not allow the lookup to succeed when there is ambiguity between samAccountName and userPrincipalName.
Comment 5 Andrew Bartlett 2021-10-27 00:28:42 UTC
This bug remains under indefinite embargo.  Do not push or disclose this without checking with Andrew Bartlett.
Comment 6 Andrew Bartlett 2022-04-13 01:49:20 UTC
Created attachment 17267 [details]
patch (againt old master) for this issue

This patch implements a new distinct check to confirm that the certificate should match, without doing the $ extension.
Comment 7 Andrew Bartlett 2022-04-13 01:57:57 UTC
Additionally, we should inspect the certificate to find any other clues (such as an extension to indicate the SID for which the certificate was issued for) that might aid us making a safe match.

Disclosure for Samba is on or after May 10 to allow coordinated disclosure with Microsoft.
Comment 8 Andrew Bartlett 2022-04-13 02:39:58 UTC
There are further issues than this patch so far attempts to address, but this helps because it means the names have to match.

The SID match mentioned would be a much stronger fix, but we don't have an example certificate for this yet. 

Fundamentally the issue is that, just as with the cname in a Kerberos ticket, if the string name in a certificate is allowed to match to a different account than originally intended, we can be broken.

The security comes from an intelligent person issuing (or not issuing) the certificates to 'suspicious' names like DC or DC$.  

If certificate auto-entrollment is used, then this could be under the control of an attacker (who also has some rights over creating or renaming AD objects). 

The use of Samba as an AD DC and certificate auto-enrolment is not something I've seen or heard of being deployed, which reduces the risk here significantly.
Comment 9 David Mulder 2022-04-13 07:46:37 UTC
(In reply to Andrew Bartlett from comment #8)
FYI, setting up certificate auto-enrollment via samba ad-dc would not be trivial. It involves a complicated mix of settings on the sysvol and entries added in ldap. On top of that, it would require a SCEP compatible CA, which IIRC there is only one opensource project (now dead) attempting to create one (openscep). Although there do appear to be proprietary solutions out there.

While not impossible, this setup seems pretty unlikely.
Comment 10 Andreas Schneider 2022-04-19 07:08:48 UTC
In order to check if a certificate is allowed for authentication we would need to implement a certauth plugin for MIT Kerberos:

https://git.samba.org/?p=asn/samba.git;a=commitdiff;h=0291232a8948d5d26f2365ce15bb311db01b045c
Comment 11 Andrew Bartlett 2022-05-10 21:16:51 UTC
The matching Microsoft update has been released as https://msrc.microsoft.com/update-guide/vulnerability/CVE-2022-26931
Comment 12 Andrew Bartlett 2022-05-10 21:45:41 UTC
Given the low risk to Samba I suggest we don't publish this until more details are public, but we should start preparing the fix.  I've asked Microsoft to publish more details on the protocol changes.
Comment 13 Andrew Bartlett 2022-05-13 06:10:05 UTC
Specification changes (describes how to build a X.509 certificate that locks in the SID the certificate is for).

https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-winerrata/6898053e-8726-4209-ade2-37f8b0474c99

https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-winerrata/c39fd72a-da21-4b13-b329-c35d61f74a60
Comment 15 Andrew Bartlett 2022-06-26 22:41:10 UTC
The use of PKINIT with X.509 in Samba is rare, so we are not doing a security release for this, but we should get a CVE and test/push the patches.