acl_read: Rework Samba code to reflect Windows logic

This patch should not alter functionality. It is just updating the Samba
code to better match the Windows specification docs.

When fixing Samba BUG #13434, the Microsoft behaviour wasn't clearly
documented, so we made a best guess based on observed behaviour.
The problem was an exception was made to allow "objectClass=*" searches
to return objects, even if you didn't have Read Property rights for the
object's objectClass attribute. However, the logic behind what
attributes were and weren't covered by this exception wasn't clear.

I made a guess that it was attributes belonging to the Public Info
property-set that also have the systemOnly flag set.

Microsoft have confirmed the object visibility behaviour. It turns out
that an optimization is made for the 4 attributes that are always
present for every object (i.e. objectClass, distinguishedName,
name, objectGUID). They're updating their Docs to reflect this.

Now that we know the Windows logic, we can update the Samba code.
This simplifies the code somewhat.

Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c
index 280845a47a5013bd83d35bd283aac901a5a416f5..6ab4780a19649dbabed0a9c55284139d60ab5956 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/acl_read.c
+++ b/source4/dsdb/samdb/ldb_modules/acl_read.c
@@ -38,9 +38,6 @@
 #include "param/param.h"
 #include "dsdb/samdb/ldb_modules/util.h"
 
-/* The attributeSecurityGuid for the Public Information Property-Set */
-#define PUBLIC_INFO_PROPERTY_SET "e48d0154-bcf8-11d1-8702-00c04fb96050"
-
 struct aclread_context {
        struct ldb_module *module;
        struct ldb_request *req;
@@ -282,16 +279,13 @@ static int check_attr_access_rights(TALLOC_CTX *mem_ctx, const char *attr_name,
                                    struct aclread_context *ac,
                                    struct security_descriptor *sd,
                                    const struct dsdb_class *objectclass,
-                                   struct dom_sid *sid, struct ldb_dn *dn,
-                                   bool *is_public_info)
+                                   struct dom_sid *sid, struct ldb_dn *dn)
 {
        int ret;
        const struct dsdb_attribute *attr = NULL;
        uint32_t access_mask;
        struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
 
-       *is_public_info = false;
-
        attr = dsdb_attribute_by_lDAPDisplayName(ac->schema, attr_name);
        if (!attr) {
                ldb_debug_set(ldb,
@@ -302,35 +296,6 @@ static int check_attr_access_rights(TALLOC_CTX *mem_ctx, const char *attr_name,
                return LDB_SUCCESS;
        }
 
-       /*
-        * If we have no Read Property (RP) rights for a child object, it should
-        * still appear as a visible object in 'objectClass=*' searches,
-        * as long as we have List Contents (LC) rights for it.
-        * This is needed for the acl.py tests (e.g. test_search1()).
-        * I couldn't find the Windows behaviour documented in the specs, so
-        * this is a guess, but it seems to only apply to attributes in the
-        * Public Information Property Set that have the systemOnly flag set to
-        * TRUE. (This makes sense in a way, as it's not disclosive to find out
-        * that a child object has a 'objectClass' or 'name' attribute, as every
-        * object has these attributes).
-        */
-       if (attr->systemOnly) {
-               struct GUID public_info_guid;
-               NTSTATUS status;
-
-               status = GUID_from_string(PUBLIC_INFO_PROPERTY_SET,
-                                         &public_info_guid);
-               if (!NT_STATUS_IS_OK(status)) {
-                       ldb_set_errstring(ldb, "Public Info GUID parse error");
-                       return LDB_ERR_OPERATIONS_ERROR;
-               }
-
-               if (GUID_compare(&attr->attributeSecurityGUID,
-                                &public_info_guid) == 0) {
-                       *is_public_info = true;
-               }
-       }
-
        access_mask = get_attr_access_mask(attr, ac->sd_flags);
 
        /* the access-mask should be non-zero. Skip attribute otherwise */
@@ -399,8 +364,14 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
 {
        struct parse_tree_aclread_ctx *ctx = NULL;
        const char *attr_name = NULL;
-       bool is_public_info = false;
        int ret;
+       static const char * const attrs_always_present[] = {
+               "objectClass",
+               "distinguishedName",
+               "name",
+               "objectGUID",
+               NULL
+       };
 
        ctx = (struct parse_tree_aclread_ctx *)private_context;
 
@@ -418,9 +389,24 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
                return LDB_SUCCESS;
        }
 
+       /*
+        * If the search filter is checking for an attribute's presence, and the
+        * attribute is always present, we can skip access rights checks. Every
+        * object has these attributes, and so there's no security reason to
+        * hide their presence.
+        * Note: the acl.py tests (e.g. test_search1()) rely on this exception.
+        * I.e. even if we lack Read Property (RP) rights for a child object, it
+        * should still appear as a visible object in 'objectClass=*' searches,
+        * so long as we have List Contents (LC) rights for the object.
+        */
+       if (tree->operation == LDB_OP_PRESENT &&
+           is_attr_in_list(attrs_always_present, attr_name)) {
+               return LDB_SUCCESS;
+       }
+
        ret = check_attr_access_rights(ctx->mem_ctx, attr_name, ctx->ac,
                                       ctx->sd, ctx->objectclass, ctx->sid,
-                                      ctx->dn, &is_public_info);
+                                      ctx->dn);
 
        /*
         * if the user does not have the rights to view this attribute, then we
@@ -428,17 +414,6 @@ static int parse_tree_check_attr_access(struct ldb_parse_tree *tree,
         * object doesn't exist (for this particular user, at least)
         */
        if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) {
-
-               /*
-                * We make an exception for attribute=* searches involving the
-                * Public Information property-set. This allows searches like
-                * objectClass=* to return visible objects, even if the user
-                * doesn't have Read Property rights on the attribute
-                */
-               if (tree->operation == LDB_OP_PRESENT && is_public_info) {
-                       return LDB_SUCCESS;
-               }
-
                ctx->suppress_result = true;
                return LDB_SUCCESS;
        }