Bug 14354 - Samba 4.12 KDC breaks with DES keys still in the database and msDS-SupportedEncryptionTypes 31 indicating support for it.
Summary: Samba 4.12 KDC breaks with DES keys still in the database and msDS-SupportedE...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.12.1
Hardware: All All
: P5 regression (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 13135
  Show dependency treegraph
 
Reported: 2020-04-22 15:58 UTC by Stefan Metzmacher
Modified: 2020-08-07 12:23 UTC (History)
7 users (show)

See Also:


Attachments
WIP patch on top of v4-12-test (1.33 KB, patch)
2020-04-23 10:17 UTC, Stefan Metzmacher
no flags Details
patch for v4-12-test branch (6.50 KB, patch)
2020-07-28 14:35 UTC, Isaac Boukris
iboukris: review+
metze: review-
Details
patch for v4-12-test branch with -x information (6.63 KB, patch)
2020-07-28 15:51 UTC, Isaac Boukris
iboukris: review+
metze: review+
Details
patch for v4-13-test branch with -x information (6.63 KB, patch)
2020-07-28 15:52 UTC, Isaac Boukris
iboukris: review+
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2020-04-22 15:58:18 UTC
There seems to be a bug in samba_kdc_message2entry_keys(),
tries to feed a DES key into smb_krb5_keyblock_init_contents() and fails.

The problem goes away if msDS-SupportedEncryptionTypes is changed from
31 to 28.
Comment 1 Isaac Boukris 2020-04-22 16:37:56 UTC
Ouch, sorry for the bug, I guess we could remove them early from supported_enctypes.
Comment 2 Stefan Metzmacher 2020-04-23 10:08:09 UTC
This only happens in Primary:Kerberos-Newer-Keys is missing for the account.

For Primary:Kerberos-Newer-Keys we already handle KRB5_PROG_ETYPE_NOSUPP.
I'll add that check also for Primary:Kerberos.

The rest should be handled as part of bug #13135
Comment 3 Stefan Metzmacher 2020-04-23 10:17:53 UTC
Created attachment 15938 [details]
WIP patch on top of v4-12-test

This should fix it.

I don't know how to add a test for this...
Would it be ok to defer tests to bug #13135?
Comment 4 Stefan Metzmacher 2020-04-23 15:07:02 UTC
Comment on attachment 15938 [details]
WIP patch on top of v4-12-test

This seems to do the job...
Comment 5 Isaac Boukris 2020-04-23 15:42:52 UTC
(In reply to Stefan Metzmacher from comment #3)

The patch looks good to me, not sure how bug #13135 would help with testing but otherwise testing seems pretty hard, so I think it would be fine to defer.
Comment 6 Isaac Boukris 2020-04-24 13:53:45 UTC
(In reply to Stefan Metzmacher from comment #3)

So I thought the problem with testing is because we always set Primary:Kerberos-Newer-Keys in >=2008 and that there was no msDS-SupportedEncryptionTypes in pre 2008, but it looks like there actually is in the fl2003dc testenv (schema option?). The following fails, and gets fixed by your patch.


make -j testenv SELFTEST_TESTENV=fl2003dc:local SCREEN=1

bin/ldbedit -e vim -H st/fl2003dc/private/sam.ldb '(samaccountname=DC6$)'

change SupportedEncryptionTypes from 28 to 31

source4/scripting/bin/machineaccountccache cc
Comment 7 Isaac Boukris 2020-04-27 12:09:51 UTC
(In reply to Isaac Boukris from comment #6)

I made a test based on the above, pipeline running:
https://gitlab.com/samba-team/devel/samba/-/commits/iboukris-metze-des-key
Comment 8 Isaac Boukris 2020-04-27 12:20:36 UTC
(In reply to Stefan Metzmacher from comment #3)

Looking at the code again, should we cleanup key.salt, in both places ?
Comment 9 Isaac Boukris 2020-04-27 12:33:55 UTC
(In reply to Isaac Boukris from comment #7)

I pushed a new version to that branch, adding cleanup of key.salt.
Comment 10 Isaac Boukris 2020-04-28 07:37:52 UTC
Reading more in ms-kile and ms-ada2, it doesn't sound exactly like the description in bug #13135 that msDS-SupportedEncryptionTypes only impacts on computer accounts, but rather only impacts when generating service ticket for an account.

Quote from ms-ada2 2.465 Attribute msDS-SupportedEncryptionTypes:
This attribute specifies the encryption algorithms supported by user, computer, or trust accounts. The Key Distribution Center (KDC) uses this information while generating a service ticket for this account.

So I'd guess it shouldn't impact on kinit even for a computer account, and should impact on a user with spn when generating a ticket to it. Need to test but if that's the case, the suggested test would be testing a wrong behaviour..
Comment 11 Andrew Bartlett 2020-04-28 08:54:15 UTC
(In reply to Isaac Boukris from comment #10)
I don't mind if the test is indirect.  The same code could be triggered by the samba-tool domain exportkeytab code if you prefer that.
Comment 12 Isaac Boukris 2020-04-28 09:00:30 UTC
(In reply to Andrew Bartlett from comment #11)

I thought I'd change it to test a service ticket but "samba-tool domain exportkeytab" sounds better, I'll try that.
Comment 13 Isaac Boukris 2020-04-28 10:27:22 UTC
(In reply to Isaac Boukris from comment #12)

Changed to test with "samba-tool domain exportkeytab", thanks.
Comment 14 Isaac Boukris 2020-07-28 14:35:29 UTC
Created attachment 16145 [details]
patch for v4-12-test branch

Pipeline for 4.12 branch running at:
https://gitlab.com/samba-team/devel/samba/-/commits/iboukris-v4-12-test
Comment 15 Stefan Metzmacher 2020-07-28 15:30:51 UTC
Comment on attachment 16145 [details]
patch for v4-12-test branch

This should also be added to 4.13
Comment 16 Stefan Metzmacher 2020-07-28 15:31:54 UTC
Comment on attachment 16145 [details]
patch for v4-12-test branch

Please upload attachements with cherry-pick -x information
for v4-12-test and v4-13-test, thanks!
Comment 17 Isaac Boukris 2020-07-28 15:51:04 UTC
Created attachment 16146 [details]
patch for v4-12-test branch with -x information
Comment 18 Isaac Boukris 2020-07-28 15:52:12 UTC
Created attachment 16147 [details]
patch for v4-13-test branch with -x information
Comment 19 Karolin Seeger 2020-08-06 11:15:54 UTC
Pushed to autobuild-v4-{13,12}-test.
Comment 20 Stefan Metzmacher 2020-08-07 12:23:21 UTC
Pushed to v4-{12,13}-test