(In reply to Stefan Metzmacher from comment #0)

Hi Garming,

> After looking into the DNS server and TXT records, I found that the
> dnsserver would segfault on certain input strings. The issues in parsing
> are well known (https://bugzilla.samba.org/show_bug.cgi?id=11128 along
> with fixes by metze and ones I concocted myself), but apparently not the
> consequences.

Do you have the patches ready, so we can get a CVE number and do
security release soon? Are my fixes enough to fix the security part
of the problem?

> Attached is an input case which causes the segfault. The text string
> itself in this case does not seem that important, only that it fails to
> send anything useful onto the wire TXT={len=0 str=). After storing the
> record in the database, a subsequent query will trigger the segfault by
> the use of a talloc_asprintf call in dns_query.c. It seems to
> dereferences an array of length 0 (not checking the count of 0), using
> that pointer as a string to return to the client. If by coincidence it
> was valid, the client could possibly get some random string returned in
> the query TXT record as long as it ended in NULL.
>
> This is only the first bug I've found in the server, it looks like there
> might be another but there's more testing to be made. Our original
> intention with this test case was the try to see if it could read up to
> 255 bytes off the end, but it never actually got passed the client.

It will not happen often, but it certainly possible.
And it's at least always a denial of service attack.

metze

Created attachment 11799 [details]
Patches with tests

These are the patches to modify the IDL as well as the tests. All of them pass Windows, besides two which have been mentioned in the changes. There are probably some more tests that can be added as well as round-tripping via RPC for other record types, but I think most of the basic aspects have been covered.

I've asked secalert@redhat.com for a CVE number...

We can use CVE-2016-0771.

Comment on attachment 11799 [details]
Patches with tests

The patches look good so far, thanks! I'm trying a private autobuild with them
now and they pass

Can you write a CVE text and prepare patches with CVE and BUG markers,
similar to commit bc2d8592f4e22dd91790bcd78e7a1a99b8a83de5?
You can squash the fixme commit and add my review where required.
I think we should also remove unused commits (the last two patches for example
are not really related).

The attachments for version 1 of the patches should be:
CVE-2016-0771-description.garming01.txt
CVE-2016-0771.master.patches.garming01.txt
CVE-2016-0771.v4-3.patches.garming01.txt
CVE-2016-0771.v4-2.patches.garming01.txt
CVE-2016-0771.v4-1.patches.garming01.txt

We'll remove the garming01 part once it's the final version.

Thanks!
metze

We need
-the final patchset with complete commit messages incl. bug numbers and CVE number,
-backports for all affected versions,
-reviews and
-the CVE advisory
for this one.

Any volunteers?

(In reply to Karolin Seeger from comment #6)
ping

(In reply to Karolin Seeger from comment #7)
Sorry, been travelling. At the very least I should be able to handle the final patches and write something plausible for the CVE text. Hopefully I can get those done soon.

Created attachment 11824 [details]
Patches for master

Created attachment 11825 [details]
Patches for 4.1

Needed to remove all references to settimeout because of the old socket wrapper. Seems to pass all the dns tests. Still need to run a full autobuilds.

Created attachment 11826 [details]
Patches for 4.2

Created attachment 11827 [details]
Patches for 4.3

Created attachment 11828 [details]
Updated patches for 4.2

Created attachment 11829 [details]
CVE Description

Comment on attachment 11824 [details]
Patches for master

Reviewed-by: Andrew Bartlett <abartlet@samba.org>

Comment on attachment 11825 [details]
Patches for 4.1

Reviewed-by: Andrew Bartlett <abartlet@samba.org>

Comment on attachment 11829 [details]
CVE Description

This is OK, but even better would mention the default of:

 allow dns updates = secure only

(and that nonsecure is a much bigger risk here)

Also, investigate which class of accounts can add a problem record - presumably at least machine accounts.

Created attachment 11850 [details]
CVE-2016-0771-description.txt

Changed versions to 4.4.0rc4, 4.3.6, 4.2.9 and 4.1.23.

Added:

+By default only authenticated accounts can upload DNS records,
+as "allow dns updates = secure only" is the default.
+Any other value would allow anonymous clients to trigger this
+bug, which is a much higher risk.

Changes links to https://

Created attachment 11851 [details]
CVE-2016-0771-master.patches.txt

Added review tags based on the old attachment (and my review).

Created attachment 11852 [details]
CVE-2016-0771-v4-4.patches.txt

Created attachment 11853 [details]
CVE-2016-0771-v4-3.patches.txt

Created attachment 11854 [details]
CVE-2016-0771-v4-2.patches.txt

Added review tags based on the old attachment (and my review).

Created attachment 11855 [details]
CVE-2016-0771-v4-1.patches.txt

Added review tags based on the old attachment (and my review).

Comment on attachment 11850 [details]
CVE-2016-0771-description.txt

Change name to CVE-2016-0771-description.txt

Comment on attachment 11851 [details]
CVE-2016-0771-master.patches.txt

Change name to CVE-2016-0771-master.patches.txt

Comment on attachment 11852 [details]
CVE-2016-0771-v4-4.patches.txt

Change name to CVE-2016-0771-v4-4.patches.txt

Comment on attachment 11853 [details]
CVE-2016-0771-v4-3.patches.txt

Change name to CVE-2016-0771-v4-3.patches.txt

Comment on attachment 11854 [details]
CVE-2016-0771-v4-2.patches.txt

Change name to CVE-2016-0771-v4-2.patches.txt

Comment on attachment 11855 [details]
CVE-2016-0771-v4-1.patches.txt

Change name to CVE-2016-0771-v4-1.patches.txt

Adding samba-vendor@samba.org alias.

Scheduled public release date is Tuesday March 8th 2016.

The tarballs are available now:
https://20160225:ialeew3aeNge@download.samba.org/security/fd9a7a5c

This is public now. Pushing patches to master now.

(In reply to Stefan Metzmacher from comment #31)
Patches ended up in master and the release branches.
Closing out bug report.

Thanks!

Removing embargo from long-fixed security bug.