Assigning to Jeremy as he offered to handle these.  I'll get the CVE started however.

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));
	}

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.

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).

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.

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.

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 !

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.

FYI, this patch passes local:

make test TESTS=samba3.blackbox.*registry

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)".

Created attachment 14975 [details]
draft of advisory (v1)

To save Jeremy some typing.

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!

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

(In reply to Jeremy Allison from comment #13)
I deliberately avoided the word required :-)

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).

Created attachment 14996 [details]
advisory with CVE (v2)

Jeremy, 

Can you please check the mitigation section and confirm this is true.

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.

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.

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.

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.

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.

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.

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.

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.

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 ?

(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.

(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.

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.

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.

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.

Created attachment 15012 [details]
Advisory v4.

Updated to explain the attack and possible mitigations better.

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 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 on attachment 15000 [details]
patch for 4.10 (v2)

This passed GitLab CI.

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.

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 on attachment 14999 [details]
patch for 4.8 (v2)

Passed on sn-devel when combined with fix for bug 13867.

Proposed release date: Monday, April 8 2019.

Created attachment 15028 [details]
advisory with CVE (v5)

Created attachment 15030 [details]
advisory with CVE (v5)

Sorry, uploaded wrong file

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.

Pushed to v4-8-test, v4-9-test, v4-10-test and autobuild-master.

(In reply to Karolin Seeger from comment #42)
Pushed to master.
Closing out bug report.

Thanks!