Bug 8509 - Read-only handles on SAMR allow SAMR_DOMAIN_ACCESS_CREATE_USER.
Summary: Read-only handles on SAMR allow SAMR_DOMAIN_ACCESS_CREATE_USER.
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: unspecified
Hardware: All All
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-06 21:13 UTC by Jeremy Allison
Modified: 2020-12-15 13:38 UTC (History)
2 users (show)

See Also:

Attachments
Patch #1 (1.76 KB, patch)
2011-10-06 21:23 UTC, Jeremy Allison 		no flags Details
Possible patch #2 (516 bytes, patch)
2011-10-06 21:25 UTC, Jeremy Allison 		no flags Details
git-am fix for 3.6.1. (2.43 KB, patch)
2011-10-07 22:05 UTC, Jeremy Allison 		metze: review+
Details
Show Obsolete (2) 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 Jeremy Allison 2011-10-06 21:13:55 UTC 
Discovered by Andreas Schneider <asn@samba.org>.

Caused by commit c0e7f7ff, by Jerry Carter, which explicitly changed :

access_check_samr_object()

-       *acc_granted |= saved_mask;
+       *acc_granted |= rights_mask;

This means that *any* NT_STATUS_OK return from access_check_samr_object() will add in the "extra" bits, even if not asked for. This has the effect of changing a read-only handle into a writable one giving SAMR_DOMAIN_ACCESS_CREATE_USER on any open.

Note - this bug goes all the way back to 3.2.0 (although not in 3.0.x). Once we have decided on a fix I'll create back-ported patches for all versions.

Two possible patches for master will follow.

Jeremy.
Comment 1 Jeremy Allison 2011-10-06 21:23:48 UTC 
Created attachment 6975 [details]
Patch #1

This is the patch for master if we must always return the SAMR_DOMAIN_ACCESS_CREATE_USER for any handle with SEC_PRIV_MACHINE_ACCOUNT, SEC_PRIV_ADD_USERS privilege.
Comment 2 Jeremy Allison 2011-10-06 21:25:55 UTC 
Created attachment 6976 [details]
Possible patch #2

This is the patch to use if we don't add the SAMR_DOMAIN_ACCESS_CREATE_USER to open handle requests for users with SEC_PRIV_MACHINE_ACCOUNT,
SEC_PRIV_ADD_USERS privilege.
Comment 3 Stefan Metzmacher 2011-10-07 13:50:09 UTC 
Comment on attachment 6976 [details]
Possible patch #2

With this one, root would not get rights_mask
Comment 4 Stefan Metzmacher 2011-10-07 13:50:30 UTC 
Comment on attachment 6975 [details]
Patch #1

I think I'd preferr this one
Comment 5 Jeremy Allison 2011-10-07 18:17:23 UTC 
Phew - not a security issue (so unlocking from Samba Team only).

We have protections inside _samr_CreateUser2() that also check for the privileges, so even though the handle returned has that permission, we check again.

Still going to fix this in 3.6.1 though.

Jeremy.
Comment 6 Jeremy Allison 2011-10-07 21:33:06 UTC 
Lowering importance and severity.
Comment 7 Jeremy Allison 2011-10-07 22:05:06 UTC 
Created attachment 6981 [details]
git-am fix for 3.6.1.

Patch that went into master.
Comment 8 Stefan Metzmacher 2011-10-07 22:44:51 UTC 
Comment on attachment 6981 [details]
git-am fix for 3.6.1.

Looks good
Comment 9 Stefan Metzmacher 2011-10-07 22:45:17 UTC 
Karolin, please pick for the release
Comment 10 Karolin Seeger 2011-10-08 18:10:56 UTC 
Pushed to v3-6-test.
Closing out bug report.

Thanks!