Bug 8509 - Read-only handles on SAMR allow SAMR_DOMAIN_ACCESS_CREATE_USER.
Read-only handles on SAMR allow SAMR_DOMAIN_ACCESS_CREATE_USER.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: DCE-RPCs and pipes
unspecified
All All
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-06 21:13 UTC by Jeremy Allison
Modified: 2011-10-08 18:10 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
jra: review? (asn)
metze: review+
Details

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!