Created attachment 14460 [details]
CVE for cname loop bug

Created attachment 14476 [details]
CVE for cname loop bug

Created attachment 14478 [details]
Completed CVE for cname loop bug

Created attachment 14537 [details]
Work in progress patch.

Work in progress patch.

Created attachment 14572 [details]
patch for 4.7 with CVE

Created attachment 14573 [details]
patch for master with CVE

Created attachment 14574 [details]
patch for 4.8 with CVE

Created attachment 14575 [details]
patch for 4.9 with CVE

Created attachment 14594 [details]
updated advisory with release versions

Sadly the patches don't currently pass their own tests.

Further investigation needed:

[3(104)/18 at 1m27s, 1 errors] samba.tests.dns(vampire_dc:local)
Won't reply to replies.
dns_process returned WERR_INVALID_PARAMETER
UNEXPECTED(failure): samba.tests.dns.__main__.TestComplexQueries.test_cname_loop(vampire_dc:local)
REASON: Exception: Exception: Traceback (most recent call last):
  File "/data/samba/SECURITY/samba-security-wip/python/samba/tests/dns.py", line 853, in test_cname_loop
    self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME)
  File "/data/samba/SECURITY/samba-security-wip/python/samba/tests/dns.py", line 700, in make_dns_update
    self.assert_dns_rcode_equals(response, dns.DNS_RCODE_OK)
  File "/data/samba/SECURITY/samba-security-wip/bin/python/samba/tests/dns_base.py", line 73, in assert_dns_rcode_equals
    (self.errstr(rcode), self.errstr(p_errcode)))
AssertionError: Expected RCODE OK, got REFUSED
dsdb_match_for_dns_to_tombstone_time: unauthorised access
envlog: SAMBA LOG of: LOCALVAMPIREDC pid 16022
Won't reply to replies.
dns_process returned WERR_INVALID_PARAMETER
dsdb_match_for_dns_to_tombstone_time: unauthorised access

command: python /data/samba/SECURITY/samba-security-wip/python/samba/tests/dns.py $SERVER $SERVER_IP --machine-pass -U"$USERNAME%$PASSWORD" --workgroup=$DOMAIN $LOADLIST  2>&1  | python /data/samba/SECURITY/samba-security-wip/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.dns." --suffix="(vampire_dc:local)"
expanded command: python /data/samba/SECURITY/samba-security-wip/python/samba/tests/dns.py localvampiredc 127.0.0.22 --machine-pass -U"Administrator%locDCpass1" --workgroup=SAMBADOMAIN $LOADLIST  2>&1  | python /data/samba/SECURITY/samba-security-wip/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.dns." --suffix="(vampire_dc:local)"
ERROR: Testsuite[samba.tests.dns(vampire_dc:local)]
REASON: Exit code was 1

Created attachment 14631 [details]
revised patch for master using size_t

strlen and strncmp deal with size_t rather than int. It probably makes no difference here but we might as well get it right.

(In reply to Douglas Bagnall from comment #11)

The 4.[789] patches are not affected as they don't have this bit.

Created attachment 14632 [details]
revised patch for master using size_t

The patch I meant to attach, using size_t instead of int, because that is what the functions use.

Created attachment 14634 [details]
updated advisory with minor grammatical and formatting changes.

Created attachment 14635 [details]
patch for 4.7 with CVE (v2)

Created attachment 14636 [details]
patch for master with CVE (v2)

I need a second "review +" here, please.

Why is the patch with cname_self_reference() not in the backport?

(In reply to Andreas Schneider from comment #18)
The DCE/RPC dnsserver set-time protections are a user-visible tool change that do not provide protection (due to direct LDAP access being available), but instead just make things nicer to the administrator.

Windows does not have these protections, and even if it did it just checks for a trivial loop back to itself, not a more complex loop.

Therefore this additional comfort feature is just introduced in the next major release.

(In reply to Karolin Seeger from comment #17)
Just on the master patch, or am I confused about the rules here?

(In reply to Andrew Bartlett from comment #20)
No, for each release series, please.
The patches might be different...
It also happened before that a patch for master applied cleanly on the release branches and was wrong there.

(In reply to Karolin Seeger from comment #21)
I'm still confused.  Douglas added the additional review today.

What more do you need?

The patches for 4.7, 4.8 and 4.9 are identical (I compared the patch files before setting that title).

Created attachment 14649 [details]
patch for master (v3 asn01)

For master the additional patch has long lines and doesn't use helper variables. See the attached patch.

(In reply to Andrew Bartlett from comment #22)
Nothing more now, thanks. :-)
Just wanted to state that 2 "review +" from team members are needed for each patch. We can discuss that, if you like.

(In reply to Andreas Schneider from comment #23)

To do that you will need to do all the required re-testing of your modified patch, and re-spin or obsolete the series patch on the master bug.

Sadly I'm right out of time to spend on this.  

Given the nature of the change, can I suggest this be done as an additional patch in master once this is out of embargo?

If you are still keen to do this under the embargo, I can however get you access to the private gitlab server when I'm in the office tomorrow. 

Sorry,

Andrew Bartlett

(In reply to Karolin Seeger from comment #24)
Uploader and reviewer is the rule, right?

(In reply to Andrew Bartlett from comment #26)
Works for me, but I prefer the explicit "review +".

Comment on attachment 14649 [details]
patch for master (v3 asn01)

Andreas, I think you meant to squash this one:

From: Andreas Schneider <asn@samba.org>
Date: Tue, 13 Nov 2018 09:01:16 +0100
Subject: [PATCH 2/3] SQ

The changes there do improve the coding style, but I certainly understand Andrew's desire to avoid another cascade of pre-security testing, and also think this would work as a post-release patch.

Aaron: note Andreas' changes.

(In reply to Douglas Bagnall from comment #28)

> this would work as a post-release patch.

Of course this is true of the whole extra master patch.

Opening bug report for vendors.
Planned release date is Tuesday, November 27 2018.

Patch doesn't apply cleanly on v4-9 (in python/samba/tests/dns.py). Easy to fix, but strange. On which baseline did you test?

(In reply to Mathieu Parent from comment #31)
Testing was against the v4-9-test branch.

Created attachment 14667 [details]
patch for 4.9 with CVE (v2)

Created attachment 14668 [details]
patch for 4.8 with CVE (v2)

Comment on attachment 14635 [details]
patch for 4.7 with CVE (v2)

I've split out the patches to be distinct for 4.7, 4.8 and 4.9. 

These are the patches that were tested, I was not correct when I said that 4.9 was identical to the the 4.8 / 4.7 patches. 

Sorry,

Andrew Bartlett

Created attachment 14670 [details]
possible patch for 4.5

This is a backport to 4.5.  It is unclear to me why the tevent changes are needed, but they seem to be required.   This applies and works on samba 4.5.0rc1 but not v4-5-test currently (or the tests flap, which is just as likely).

(In reply to Andrew Bartlett from comment #36)
I'm taking a break from this for a day to try and attack it again with a clear head tomorrow.

Things to check:
 - Confirm the stability or otherwise of the tests (make test TESTS=samba.tests.dns) without any changes
 - Confirm the stability or otherwise of the tests (make test TESTS=samba.tests.dns) with just the tevent changes
 - Confirm the behaviour with only the new test (no tevent changes, no fix)
 - Confirm the behaviour with only the fix
 - Confirm the behaviour with only the fix and new test
 - Confirm the behaviour with the fix, new test and tevent changes

The existing CNAME tests (regarding CNAME lookups for forwarded names) are where the failures are being seen, not the test for the actual fix.

(In reply to Andrew Bartlett from comment #37)
Thanks for this backport.

However, it seems that the [PATCH 4/4] header is missing, and it tries to patch selftest/knownfail.d/dns which doesn't exists.

(In reply to Mathieu Parent from comment #38)
Yeah, very much a WIP.  I was bisecting back from where it fully passed to samba-4.5.0rc1.

Because we introduced the knownfail.d concept during development of Samba 4.6 so some had that file and some did not (so I patched selftest/knownfail instead). 

Hopefully I get to a more acceptable situation tomorrow.

Created attachment 14685 [details]
Tested patch for 4.5

This should be the correct patch for Samba 4.5, currently undergoing a full autobuild.

The confusion with the 4.5 backport came from:
 - changes in the API used by the test
 - flappy tests during 4.5 for other, unrelated CNAME behaviour

Comment on attachment 14685 [details]
Tested patch for 4.5

The fix is exactly the same, and I trust the tests.

This bit will have no effect:

>--- /dev/null
>+++ b/selftest/knownfail.d/dns
>@@ -0,0 +1,5 @@
>+#
>+# rodc and vampire_dc require signed dns updates, so the test setup
>+# fails, but the test does run on fl2003dc
>+^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(rodc:local\)
>+^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(vampire_dc:local\)

Comment on attachment 14685 [details]
Tested patch for 4.5

The patch passed a full autobuild on the Catalyst Cloud

Samba 4.9.3, 4.8.7 and 4.7.12 have been shipped to address these defects.

Pushed to autobuild-master.

The patches introduce a regression, when there're more than 20 records on
a non CNAME record.

This happens in domains with more than 20 DCs.

Created attachment 14691 [details]
Possible regression fix for master

Created attachment 14692 [details]
Possible regression fix for v4-9-test

Created attachment 14693 [details]
Possible regression fix for v4-8-test

Created attachment 14694 [details]
Possible regression fix for v4-7-test

(In reply to Stefan Metzmacher from comment #46)
Thanks for that.

However we are going to need a test to make sure we don't stumble on it again.

Drat.

I'll see if someone on my team can look into that this afternoon.

(In reply to Andrew Bartlett from comment #50)
Test cases needed:

 - 21 A records
 - 21 AAAA records
 - CNAME to name with 21 A records
 - 21 SRV records
 - SRV record to name with 21 A records 
 - SRV record to CNAME loop

Created attachment 14696 [details]
WIP patch counting CNAMEs not answers

We are working on tests.

Through not reading the bug properly I created another fix, which I don't prefer over metze's "possible regression fix" if metze's one works.

Comment on attachment 14696 [details]
WIP patch counting CNAMEs not answers

I like the idea and also thought about that, but I needed a quick solution
in order to fix customer problems.

Please use size_t instead of 'uint'
and don't pass *cname_count to add_response_rr(), we can increment
it in the caller.

Should we rename cname_count to cname_depth?
And we always need to pass down the value and not a pointer.
Maybe we don't even need to store it on state.

Created attachment 14697 [details]
Possible regression fix for master (v2)

(In reply to Karolin Seeger from comment #44)
Pushed initial fix to master (not the regression fix).

Comment on attachment 14697 [details]
Possible regression fix for master (v2)

That is much simpler and better.

(In reply to Douglas Bagnall from comment #56)

Though it will need this:

-	if (cname_depth >= MAX_Q_RECURSION_DEPTH) {
-		tevent_req_done(req);
-		return tevent_req_post(req, ev);
-	}

Created attachment 14704 [details]
tests exposing regression

Thanks for all the work guys, sorry for my newbie mistake.  Here's 4 out of 6 of the tests described by Andrew above.  The tests fail on the original patch, pass with Stefan's quick fix, and also pass with the more thorough patch (attachment 14697 [details]).

The last two suggested SRV tests don't help because it turns out in samba we don't do server-side following of SRV records even if the target is something in our database.  I will figure out whether this is intended behaviour on Monday.  Either way, not relevant to this discussion.

Thanks again.

(In reply to Douglas Bagnall from comment #57)

> Though it will need this:
>
> -	if (cname_depth >= MAX_Q_RECURSION_DEPTH) {
> -		tevent_req_done(req);
> -		return tevent_req_post(req, ev);
> -	}

I don't understand that.

We don't need to move that, because we only increment cname_depth below
and for the non CNAME cname_depth will always be 0.

(In reply to Stefan Metzmacher from comment #59)

I don't understand it either now.

Clearly it was too early in the morning.

Created attachment 14711 [details]
tests exposing regression v2

Here's a new test patch version with a new test to make sure queries with qtype=ANY don't break any assumptions.  As it turns out, ANY queries don't follow CNAME records.

Created attachment 14712 [details]
tests exposing regression v3

Adding knownfails for test environments that don't allow unsigned DNS updates.

Comment on attachment 14712 [details]
tests exposing regression v3

The tests look correct and work as expected.

Both of metze's patches pass; master fails.

Comment on attachment 14712 [details]
tests exposing regression v3

Douglas can you push my 2nd version and these tests to master, so that we
can backport them from there?

Thanks!
metze

Created attachment 14717 [details]
combined fix, regression fix and test patch for 4.5

Comment on attachment 14717 [details]
combined fix, regression fix and test patch for 4.5

This passed a full autobuild on the Catalyst Cloud

Regression fix and test patches for 4.7, 4.8 and 4.9 are under test here:

https://gitlab.com/samba-team/samba/merge_requests/134
https://gitlab.com/samba-team/samba/merge_requests/135
https://gitlab.com/samba-team/samba/merge_requests/136

Feel free to upload and approve those once the CI passes.

Created attachment 14719 [details]
combined fix, regression fix and test patch for 4.9

Created attachment 14720 [details]
combined fix, regression fix and test patch for 4.7

This starts at 3/4 because it was tested on top of attachment 14683 [details] of bug 13689 (needed for testing).

(In reply to Andrew Bartlett from comment #67)
4.7 and 4.9 passed, 4.8 flapped and is being re-tested.

Created attachment 14724 [details]
combined fix, regression fix and test patch for 4.8

Note, at the end of my work week I've been unable to get this patch to pass the full pipeline on GitLab CI due to unrelated flapping tests.

https://gitlab.com/samba-team/devel/samba/pipelines/38990590

Rather than persist at this, please push via a normal autobuild on sn-devel.

(In reply to Andrew Bartlett from comment #71)
Thanks, Andrew!
Pushed to v4-{9,8,7}-autobuild.

Pushed to all branches.
Closing out bug report.

Thanks!

Created attachment 17140 [details]
advisory text (v03)

Removing samba-vendor CC.  Please subscribe individually if you wish to follow this old and not terribly exciting security bug.

I do apologise for the noise.