Bug 14187 (CVE-2019-14870) - CVE-2019-14870 | DelegationNotAllowed not being enforced
Summary: CVE-2019-14870 | DelegationNotAllowed not being enforced
Status: RESOLVED FIXED
Alias: CVE-2019-14870
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Isaac Boukris
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14185
  Show dependency tree / graph
 
Reported: 2019-11-06 15:57 UTC by Isaac Boukris
Modified: 2021-02-09 22:20 UTC (History)
8 users (show)

See Also:

Attachments
v2 patches for master (15.42 KB, patch)
2019-11-07 11:24 UTC, Isaac Boukris 		no flags Details
v2 patches for master (updated) (15.33 KB, patch)
2019-11-07 15:26 UTC, Isaac Boukris 		abartlet: review+
Details
v2.1 patches for master (mit updated) (13.46 KB, patch)
2019-11-21 21:58 UTC, Isaac Boukris 		abartlet: review+
iboukris: review+
iboukris: ci-passed+
Details
v2.1 patches for v4.9 branch (13.60 KB, patch)
2019-11-24 08:44 UTC, Isaac Boukris 		abartlet: review+
iboukris: review+
iboukris: ci-passed+
Details
v2.1 patches for v4.10 and v4.11 branches (13.55 KB, patch)
2019-11-24 08:45 UTC, Isaac Boukris 		abartlet: review+
iboukris: ci-passed+
Details
first draft security advisory (2.20 KB, text/plain)
2019-11-27 04:30 UTC, Andrew Bartlett 		no flags Details
v2.1 patches for v4.10 branche (13.55 KB, patch)
2019-11-27 08:09 UTC, Isaac Boukris 		iboukris: review+
abartlet: review+
iboukris: ci-passed+
Details
v2.1 patches for v4.11 branch (13.55 KB, patch)
2019-11-27 08:13 UTC, Isaac Boukris 		iboukris: review+
abartlet: review+
iboukris: ci-passed+
Details
security advisory v2 (2.35 KB, patch)
2019-11-27 09:14 UTC, Isaac Boukris 		iboukris: review+
Details
Advisory v3 (2.44 KB, text/plain)
2019-11-27 17:21 UTC, Andrew Bartlett 		iboukris: review+
Details
Advisory v4 (2.44 KB, text/plain)
2019-11-29 10:35 UTC, Karolin Seeger 		abartlet: review+
Details
backport of patch 2.1 to Samba 4.5 (14.04 KB, patch)
2019-12-03 03:02 UTC, Andrew Bartlett 		iboukris: review+
abartlet: ci-passed+
Details
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Isaac Boukris 2019-11-06 15:57:26 UTC 
The S4U delegation model includes a feature allowing for a subset of clients to be opted out of constrained delegation in any way, either S4U2Self or regular Kerberos authentication, by forcing all tickets for that client to be non-forwardable.
In AD this is implemented by a user attribute delegation_not_allowed (aka not-delegated), which translates to disallow-forwardable and disallow-proxiable.

However in the heimdal build we don't seem to do that for S4U2Self and we set forwardbale flag even if the impersonated client has the not-delegated flag set, while in MIT where we don't support S4U we don't seem to enforce it at all so it may also be a problem (we do translate the UAC ND flag to forwardable in db-glue.c, but then we don't use it).
Comment 1 Andrew Bartlett 2019-11-06 17:38:12 UTC 
(In reply to Isaac Boukris from comment #0)

G'Day Isaac,

Thanks so much for raising this bug and starting down the Samba security process.  Let me know if there is anything I can do to help.  It can be quite daunting!
Comment 2 Isaac Boukris 2019-11-07 11:24:07 UTC 
Created attachment 15606 [details]
v2 patches for master
Comment 3 Isaac Boukris 2019-11-07 11:38:54 UTC 
(In reply to Andrew Bartlett from comment #1)

Thanks Andrew, i've attached v2 patches for review and I need to run a private autobuild, then we'll be ready to coordinate with upstream I think.
Comment 4 Isaac Boukris 2019-11-07 15:26:55 UTC 
Created attachment 15611 [details]
v2 patches for master (updated)
Comment 5 Andrew Bartlett 2019-11-07 17:41:28 UTC 
Comment on attachment 15611 [details]
v2 patches for master (updated)

Thanks.  The patches look reasonable to me.

You will need to backport to each release branch and then CI will need to be done on each exact patch set on the -stable branch it is targeted to.

However, to make things a bit more difficult, when I've been doing CI work recently Samba 4.9 consistently failed CI for me until I backported 4ae0f9ce0f5ada99cf1d236377e5a1234c879ae3.  Therefore I needed to include that with the patches before they would pass.
Comment 6 Isaac Boukris 2019-11-07 18:13:48 UTC 
(In reply to Andrew Bartlett from comment #5)

Thanks for the initial review and CI hints.
Any idea about the state of https://gitlab.com/samba-team/samba-security/ ?
This would be much simpler than private autobuids.

I also need to start writing the advisory.
Comment 7 Andrew Bartlett 2019-11-07 18:49:21 UTC 
(In reply to Isaac Boukris from comment #6)
The CI isn't working on that repo at the moment and we prefer not to have CVE-worthy patches on that infrastructure.

There is an alternate GitLab CI which I'll get you details for privately.
Comment 8 Isaac Boukris 2019-11-07 19:05:02 UTC 
(In reply to Andrew Bartlett from comment #7)

Thanks!

Meanwhile private autobuild succeeded for the heimdal commits, while it failed with the MIT patch, I'll need to look into that.
Comment 9 Isaac Boukris 2019-11-08 10:15:43 UTC 
(In reply to Isaac Boukris from comment #8)

As I suspected, the CI passed on gitlab with the MIT patch.
It is only on sn-devel, using apparently the same krb5 version, where the old MIT bug seem to be fixed, so it fails on unexpected success. I need to find a way to distinguish.
Comment 10 Andrew Bartlett 2019-11-20 21:29:05 UTC 
Is this bug related to this public MR in Heimdal https://github.com/heimdal/heimdal/pull/653 ?

If so, we might need to hurry up here.
Comment 11 Isaac Boukris 2019-11-20 21:46:31 UTC 
(In reply to Andrew Bartlett from comment #10)

No worries, it is not related, that is about tgt delegation, not S4U.
Comment 12 Isaac Boukris 2019-11-21 21:58:37 UTC 
Created attachment 15630 [details]
v2.1 patches for master (mit updated)

I have updated the MIT commit to only apply disallow-forwardable in AS request, since we do not support S4U2Self it should be sufficient. Please look.
Comment 13 Isaac Boukris 2019-11-24 08:44:59 UTC 
Created attachment 15631 [details]
v2.1 patches for v4.9 branch
Comment 14 Isaac Boukris 2019-11-24 08:45:58 UTC 
Created attachment 15632 [details]
v2.1 patches for v4.10 and v4.11 branches
Comment 15 Isaac Boukris 2019-11-24 08:59:26 UTC 
Attached v2.1 patches for all branches, all passed CI on gitlab (in the v4.9 i added the commit from comment 5 when pushing to gitlab).
Also, it finally passed a private build on sn-devel (master).
Comment 16 Andrew Bartlett 2019-11-24 09:10:11 UTC 
Thanks for all your hard work here.  I'll check the patches again when I get a chance, but I really appreciate you stepping up to address this!
Comment 17 Andrew Bartlett 2019-11-27 04:30:00 UTC 
Created attachment 15635 [details]
first draft security advisory

Thanks for the patches!

I think the next step is to write an advisory, so I've made a first go at it and attached it to keep things moving.  Please review, correct and improve. 

Thanks!

Andrew Bartlett
Comment 18 Andrew Bartlett 2019-11-27 04:32:45 UTC 
Only one process note.  Our process requests that you add your own + to patches are you are happy with, and we need exactly one patch per branch, so that the CI-passed tags can be per-branch.  

Sorry for the extra admin, it just helps ensure no mistakes in this stressful process (we have had wrong patches uploaded, and incorrect assertions that they apply to all branches etc).
Comment 19 Isaac Boukris 2019-11-27 08:09:41 UTC 
Created attachment 15636 [details]
v2.1 patches for v4.10 branche
Comment 20 Isaac Boukris 2019-11-27 08:13:09 UTC 
Created attachment 15637 [details]
v2.1 patches for v4.11 branch
Comment 21 Isaac Boukris 2019-11-27 09:14:15 UTC 
Created attachment 15638 [details]
security advisory v2
Comment 22 Isaac Boukris 2019-11-27 09:31:24 UTC 
(In reply to Andrew Bartlett from comment #17)

Thank you for all the help on this, I've updated the advisory to make it clear the DelegationNotAllowed is not enforced only in S4U2Self (not for tgt delegation for instance to which it also applies i think), and I added a note about the MIT build.
Comment 23 Andrew Bartlett 2019-11-27 17:21:41 UTC 
Created attachment 15639 [details]
Advisory v3

The advisory looks good, just adding credit to us both for writing it (need to put that in the template).
Comment 24 Karolin Seeger 2019-11-29 10:12:28 UTC 
Opening bug report for vendors.
Planned release date: December 10
Comment 25 Karolin Seeger 2019-11-29 10:35:26 UTC 
Created attachment 15648 [details]
Advisory v4

"4.11.4" -> "4.11.3"
Comment 26 Andrew Bartlett 2019-12-03 02:37:44 UTC 
Comment on attachment 15648 [details]
Advisory v4

Sorry for the delay, the new advisory looks fine.
Comment 27 Andrew Bartlett 2019-12-03 03:02:32 UTC 
Created attachment 15662 [details]
backport of patch 2.1 to Samba 4.5

I've back-ported the Samba 4.9 patch series to Samba 4.5.  They pass the new test but I've not run a full CI yet.
Comment 28 Andrew Bartlett 2019-12-09 22:09:30 UTC 
Comment on attachment 15662 [details]
backport of patch 2.1 to Samba 4.5

I'm pleased to say that the Samba 4.5 backport passed. 

If someone would like to review the patch to ensure I've not missed anything in the backport that would be most appreciated.
Comment 29 Karolin Seeger 2019-12-10 09:11:45 UTC 
Samba 4.11.3, 4.10.11 and 4.9.17 have been shipped to address this defect.
Comment 30 Karolin Seeger 2019-12-10 09:18:18 UTC 
Pushed to autobuild-master.
Comment 31 Karolin Seeger 2019-12-10 09:27:54 UTC 
Pushed to v4-{11,10,9}-test.
Comment 32 Isaac Boukris 2019-12-10 09:41:35 UTC 
I've also submitted PR #663 and #664 upstream heimdal, thank you all.
Comment 33 Karolin Seeger 2019-12-13 08:55:20 UTC 
(In reply to Karolin Seeger from comment #30)
Pushed to master.
Closing out bug report.

Thanks!