Bug 13851 - (CVE-2019-3880) CVE-2019-3880 [SECURITY] Save registry file outside share as unprivileged user in Samba 4.x
(CVE-2019-3880)
CVE-2019-3880 [SECURITY] Save registry file outside share as unprivileged use...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes
4.10.0
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on: 13867
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-21 01:58 UTC by Andrew Bartlett
Modified: 2019-04-09 07:51 UTC (History)
6 users (show)

See Also:


Attachments
git-am patch for master, applies cleanly to 4.10.next, 4.9.next, 4.8.next. (4.36 KB, patch)
2019-03-21 21:57 UTC, Jeremy Allison
abartlet: review+
Details
draft of advisory (v1) (1.53 KB, text/plain)
2019-03-21 23:36 UTC, Andrew Bartlett
no flags Details
advisory with CVE (v2) (1.94 KB, text/plain)
2019-03-27 03:41 UTC, Andrew Bartlett
no flags Details
patch for master (v2) under test on private Gitlab CI (4.35 KB, patch)
2019-03-27 03:56 UTC, Andrew Bartlett
jra: review+
Details
patch for 4.9 (v2) (4.35 KB, patch)
2019-03-27 03:57 UTC, Andrew Bartlett
jra: review+
abartlet: review+
Details
patch for 4.8 (v2) (4.35 KB, patch)
2019-03-27 03:58 UTC, Andrew Bartlett
jra: review+
abartlet: review+
Details
patch for 4.10 (v2) (4.35 KB, patch)
2019-03-27 03:59 UTC, Andrew Bartlett
jra: review+
abartlet: review+
Details
patch for 4.5 (v2) (4.42 KB, patch)
2019-03-27 04:01 UTC, Andrew Bartlett
garming: review+
Details
v3 of the advisory. (1.95 KB, text/plain)
2019-03-27 17:56 UTC, Jeremy Allison
no flags Details
Patch for master v3. (16.44 KB, patch)
2019-03-27 20:26 UTC, Jeremy Allison
abartlet: review+
Details
Advisory v4. (2.91 KB, text/plain)
2019-03-27 20:51 UTC, Jeremy Allison
abartlet: review+
jra: review+
Details
advisory with CVE (v5) (2.65 KB, text/plain)
2019-04-01 00:44 UTC, Andrew Bartlett
garming: review+
abartlet: review+
Details
advisory with CVE (v5) (3.01 KB, text/plain)
2019-04-01 01:12 UTC, Andrew Bartlett
garming: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-03-21 01:58:11 UTC
Created attachment 14966 [details]
advisory from reporter

Michael Hanselmann found a symlink traversal issue in our winreg server.
Comment 1 Andrew Bartlett 2019-03-21 01:59:33 UTC
Assigning to Jeremy as he offered to handle these.  I'll get the CVE started however.
Comment 2 Andrew Bartlett 2019-03-21 02:33:23 UTC
Additionally the attacker can probably tell what file exist on a system by checking the errors that come out of:

static WERROR backup_registry_key(struct registry_key_handle *krecord,
				  const char *fname)
{
...

	regfile = regfio_open(fname, (O_RDWR|O_CREAT|O_EXCL),
			      (S_IRUSR|S_IWUSR));
	if (regfile == NULL) {
		DEBUG(0,("backup_registry_key: failed to open \"%s\" (%s)\n",
			 fname, strerror(errno) ));
		return ntstatus_to_werror(map_nt_error_from_unix(errno));
	}
Comment 3 Jeremy Allison 2019-03-21 20:51:44 UTC
Hmmm. None of this winreg code seems to go through the VFS at all.

I'm wondering if we should just restrict the winreg code to be root-only access, and only work against locally attached shares.

This is very old nasty code.
Comment 4 Jeremy Allison 2019-03-21 20:54:29 UTC
Most of this code seems to be written to act on local registry files, not over an RPC connection.

I wonder if we can just restrict this to be the generic case (i.e., not allow save/restore over RPC at all).
Comment 5 Andrew Bartlett 2019-03-21 20:59:01 UTC
I think that is the correct approach, just fault on the RPC call. 

If somehow we must, then open the file with VFS hooks and either get the FD or copy the file to a known safe location. 

But looking at how this is meant to work, given it requires a full server path  (from / !) this can't be something that an automated tool or installer used, and I think we should just drop it.
Comment 6 Jeremy Allison 2019-03-21 21:35:10 UTC
OK - we already don't implement _winreg_SaveKeyEx (BaseRegSaveKeyEx in MS-RRP, opnum 31).

I just want to remove the implementation of _winreg_SaveKey/_winreg_RestoreKey in smbd.

I don't think this has ever been reliably used and is a set of security / VFS issues waiting to happen.

Patch to follow.
Comment 7 Jeremy Allison 2019-03-21 21:41:21 UTC
Instead of faulting the RPC I think first checking if the handle is valid (returning WERR_INVALID_HANDLE if not) and then always returning WERR_BAD_PATHNAME if the handle is valid might be the way to go.

It's a less invasive change and won't show RPC fault errors to clients.

From MS-RRP:

lpFile: A pointer to an RRP_UNICODE_STRING structure that contains the name of the file in which
the specified key and subkeys are saved. ****The format of the file name is implementation-specific***

(***) emphasis mine.

That way we're just saying there's no 'good' implementation-specific filename rather than pulling out support for the call.

Comments welcome though !
Comment 8 Jeremy Allison 2019-03-21 21:57:51 UTC
Created attachment 14974 [details]
git-am patch for master, applies cleanly to 4.10.next, 4.9.next, 4.8.next.

Andrew, let me know if this works for you or you'd prefer it to be an RPC fault.

Michael, please check this fixes your issue.

Once this is good I'll write an advisory and ensure everything is in place for the CVE release.
Comment 9 Jeremy Allison 2019-03-21 22:11:41 UTC
FYI, this patch passes local:

make test TESTS=samba3.blackbox.*registry
Comment 10 hansmi 2019-03-21 23:17:02 UTC
Jeremy, I can confirm that the patch breaks my proof-of-concept exploit. The "net" command fails with "rpc command function failed! (NT_STATUS_OBJECT_PATH_INVALID)".
Comment 11 Andrew Bartlett 2019-03-21 23:36:38 UTC
Created attachment 14975 [details]
draft of advisory (v1)

To save Jeremy some typing.
Comment 12 Andrew Bartlett 2019-03-25 21:19:36 UTC
Comment on attachment 14974 [details]
git-am patch for master, applies cleanly to 4.10.next, 4.9.next, 4.8.next.

The concept is good, a test (eg in python) that shows this working and then now failing would be nice.

Per our security procedures we need one patch per release even if identical, and the name needs to be $CVE-$MAJOR-$VERSION.  Also put the CVE in the bug title. 

6b.) Prepare backports for all affected and supported versions (1 file per version, even if identical).
     Files should be named $CVE-$MAJOR-$VERSION.patch
     E.g.: CVE-2018-14629-4.9-01.patch

Then put your own review tick on once you have passed a private autobuild or CI with the exact patches. 

Thanks!
Comment 13 Jeremy Allison 2019-03-25 21:22:40 UTC
Nice yes. Required ? If so, you're raising the bar significantly for this. Confirmation from the submitter should be enough here. Remember I'm also working on:

https://bugzilla.samba.org/show_bug.cgi?id=13842
Comment 14 Andrew Bartlett 2019-03-25 21:46:23 UTC
(In reply to Jeremy Allison from comment #13)
I deliberately avoided the word required :-)
Comment 15 Andrew Bartlett 2019-03-26 04:16:26 UTC
Comment on attachment 14974 [details]
git-am patch for master, applies cleanly to 4.10.next, 4.9.next, 4.8.next.

Renamed patch per guidelines.  (it should be one patch per branch, but for now at least be clear).
Comment 16 Andrew Bartlett 2019-03-27 03:41:34 UTC
Created attachment 14996 [details]
advisory with CVE (v2)

Jeremy, 

Can you please check the mitigation section and confirm this is true.
Comment 17 Andrew Bartlett 2019-03-27 03:56:17 UTC
Created attachment 14997 [details]
patch for master (v2) under test on private Gitlab CI

This has my review tag on it.  I'll add my own review once the CI is back.
Comment 18 Andrew Bartlett 2019-03-27 03:57:23 UTC
Created attachment 14998 [details]
patch for 4.9 (v2)

This has my review tag in the commit.  I'll add my own review here once the CI is back.
Comment 19 Andrew Bartlett 2019-03-27 03:58:23 UTC
Created attachment 14999 [details]
patch for 4.8 (v2)

This has my review tag in the commit.  I'll add my own review here once the CI is back.
Comment 20 Andrew Bartlett 2019-03-27 03:59:44 UTC
Created attachment 15000 [details]
patch for 4.10 (v2)

This has my review tag in the commit.  I'll add my own review here once the CI is back.
Comment 21 Andrew Bartlett 2019-03-27 04:01:30 UTC
Created attachment 15001 [details]
patch for 4.5 (v2)

Samba 4.5 is not supported by the Samba Team any more but here is a backport that may be helpful.
Comment 22 Jeremy Allison 2019-03-27 17:56:37 UTC
Created attachment 15010 [details]
v3 of the advisory.

Andrew, slightly updated advisory. Explicitly calls out vulnerable versions (Samba 3.2.0 and onwards) and slightly clarifies mitigation text.

Let me know if this is OK.
Comment 23 Andrew Bartlett 2019-03-27 19:13:15 UTC
I think we will get questions as to if 'read only = yes', write list or share ACLs etc provides protection.  

The way I read the code and understand smbd, it would prevent a symlink from being created, but not prevent saving a file because the check is only that it is a valid share path, not the permissions on that share. 

Likewise VFS objects that normally provide protections would not do so. 

Without making this really complex, we should prove this with testing and explain this somehow.
Comment 24 Jeremy Allison 2019-03-27 19:32:36 UTC
I thought the modification of the advisory made this clear. A symlink can be followed to write outside the share as there are no protections being applied here.

Any writable share can be used so long as a symlink inside it pointing outside.

Do you want me to do another pass over the advisory to try and clarify this ?

I won't have time before the release on 4/8/2019 to add a test. The code allowing the upload has been removed completely, so a test would be be confirming what has already been done here.
Comment 25 Jeremy Allison 2019-03-27 19:37:41 UTC
One more thing. The 'restore function' reg_restorekey() was only called from the removed code inside _winreg_RestoreKey().

This makes several functions inside source3/registry/reg_api_regf.c dead code.

Do you want me to update the patchset to prune these also ?
Comment 26 Andrew Bartlett 2019-03-27 19:58:22 UTC
(In reply to Jeremy Allison from comment #25)
For master, certainly.  It seems we are on deadline now so I'd rather not need to re-test, even for 'obvious' stuff like this.

I guess my point on the read only thing is that the symlink game is not the only attack, you could also create files in a read-only share with an arbitrary name, potentially (after some effort) filling the disk.
Comment 27 Andrew Bartlett 2019-03-27 20:00:04 UTC
(In reply to Jeremy Allison from comment #24)
Regarding a test, that is fine.  I spoke with Garming and we agreed that a test for the return code is rather pointless on it's own and a comprehensive test is not good use of time between now and the deadline if this is to ship with the other security issue.
Comment 28 Jeremy Allison 2019-03-27 20:03:41 UTC
Got it. I'll update the advisory. I have the updated patch for master that removes the now unused files:

source3/registry/reg_api_regf.c
source3/registry/reg_api_regf.h

and the build scripts for them. I'll upload as an updated fix for master.
Comment 29 Jeremy Allison 2019-03-27 20:26:52 UTC
Created attachment 15011 [details]
Patch for master v3.

Additional patch on top of the previous fix. Removes unused files from build. Removes:

source3/registry/reg_api_regf.c
source3/registry/reg_api_regf.h

and the REG_API_REGF subsystem from source3/wscript_build.
Comment 30 Jeremy Allison 2019-03-27 20:42:47 UTC
OK, going through this carefully - read-only shares don't protect against this. Files may be written even on read-only shares.

The only mitigating factor is that we use:

O_CREAT|O_EXCL

as open flags, which means that only *new* files can be created, not overwriting existing files.
Comment 31 Jeremy Allison 2019-03-27 20:51:34 UTC
Created attachment 15012 [details]
Advisory v4.

Updated to explain the attack and possible mitigations better.
Comment 32 Andrew Bartlett 2019-03-29 04:55:01 UTC
Jeremy,

I've tried to help out with running some CI for you, but I've only had some of them pass due generally to unrelated issues. 

If you want this released in the next security release you will need to get a autobuild or CI pass and then edit the patch titles, then follow the steps from the security procedure doc.

To provide the 10 days notice, you would need to do that before COB tomorrow. 

If any of the jobs I've got running pass (I'm testing two different GitLab CI systems, and sn-devel) I'll update the bugzilla of course. 

I had hoped to take care of this for you,

Sorry,
Comment 33 Andrew Bartlett 2019-03-29 05:17:06 UTC
Comment on attachment 15000 [details]
patch for 4.10 (v2)

I've been having various flapping tests fail on this.  The closet I got to a pass was this:

samba: [check-clean-tree] Running script/clean-source-tree.sh in '/tmp/samba-testbase/b11/samba'
The tree has 1 new uncommitted files!!! see stderr

==> /builds/samba/samba.org-security-patches/samba.stderr <==
The tree has 1 new uncommitted files!!!
git clean -n
Would remove core
samba: [check-clean-tree] failed 'script/clean-source-tree.sh' with status 1

I would write this off as environmental (eg the Rackspace runner used for the private CI) however I can't reproduce the failure on clean master or 4.10 in the same setup.  I'm testing this again via multiple systems to try and get a pass, or we could call this a pass (but I won't make that call).
Comment 34 Andrew Bartlett 2019-03-29 08:06:42 UTC
Comment on attachment 15000 [details]
patch for 4.10 (v2)

This passed GitLab CI.
Comment 35 Andrew Bartlett 2019-03-29 08:31:35 UTC
Comment on attachment 14999 [details]
patch for 4.8 (v2)

To break the deadlock here I've filed bug 13867 to backport a fix for the most problematic flapping test.
Comment 36 Jeremy Allison 2019-03-29 16:15:48 UTC
Yes, getting the flapping tests out of the way will help making the security-CI runs reliable. Don't think there's anything more I need to do here (lots of other tasks right now). Let me know if so.
Comment 37 Andrew Bartlett 2019-03-29 17:22:54 UTC
Comment on attachment 14999 [details]
patch for 4.8 (v2)

Passed on sn-devel when combined with fix for bug 13867.
Comment 38 Andrew Bartlett 2019-03-29 17:24:25 UTC
Proposed release date: Monday, April 8 2019.
Comment 39 Andrew Bartlett 2019-04-01 00:44:13 UTC
Created attachment 15028 [details]
advisory with CVE (v5)
Comment 40 Andrew Bartlett 2019-04-01 01:12:19 UTC
Created attachment 15030 [details]
advisory with CVE (v5)

Sorry, uploaded wrong file
Comment 41 Andrew Bartlett 2019-04-08 08:42:47 UTC
Samba 4.10.2, 4.9.6 and 4.8.11 Security Releases were made to address this issue.

Leaving open until patches land in master, removing embargo.
Comment 42 Karolin Seeger 2019-04-08 10:32:05 UTC
Pushed to v4-8-test, v4-9-test, v4-10-test and autobuild-master.
Comment 43 Karolin Seeger 2019-04-09 07:51:31 UTC
(In reply to Karolin Seeger from comment #42)
Pushed to master.
Closing out bug report.

Thanks!