diff options
Diffstat (limited to 'main/samba/0010-CVE-2018-10919-acl_read-Fix-unauthorized-attribute-a.patch')
| -rw-r--r-- | main/samba/0010-CVE-2018-10919-acl_read-Fix-unauthorized-attribute-a.patch | 351 |
1 files changed, 351 insertions, 0 deletions
diff --git a/main/samba/0010-CVE-2018-10919-acl_read-Fix-unauthorized-attribute-a.patch b/main/samba/0010-CVE-2018-10919-acl_read-Fix-unauthorized-attribute-a.patch new file mode 100644 index 0000000000..2a647fa024 --- /dev/null +++ b/main/samba/0010-CVE-2018-10919-acl_read-Fix-unauthorized-attribute-a.patch @@ -0,0 +1,351 @@ +From 9b17ce9a1f46e8519302eb6ec72f1104560bf953 Mon Sep 17 00:00:00 2001 +From: Tim Beale <timbeale@catalyst.net.nz> +Date: Fri, 20 Jul 2018 15:42:36 +1200 +Subject: [PATCH] CVE-2018-10919 acl_read: Fix unauthorized attribute access + via searches + +A user that doesn't have access to view an attribute can still guess the +attribute's value via repeated LDAP searches. This affects confidential +attributes, as well as ACLs applied to an object/attribute to deny +access. + +Currently the code will hide objects if the attribute filter contains an +attribute they are not authorized to see. However, the code still +returns objects as results if confidential attribute is in the search +expression itself, but not in the attribute filter. + +To fix this problem we have to check the access rights on the attributes +in the search-tree, as well as the attributes returned in the message. + +Points of note: +- I've preserved the existing dirsync logic (the dirsync module code + suppresses the result as long as the replPropertyMetaData attribute is + removed). However, there doesn't appear to be any test that highlights + that this functionality is required for dirsync. +- To avoid this fix breaking the acl.py tests, we need to still permit + searches like 'objectClass=*', even though we don't have Read Property + access rights for the objectClass attribute. The logic that Windows + uses does not appear to be clearly documented, so I've made a best + guess that seems to mirror Windows behaviour. + +BUG: https://bugzilla.samba.org/show_bug.cgi?id=13434 + +Signed-off-by: Tim Beale <timbeale@catalyst.net.nz> +--- + selftest/knownfail.d/acl | 1 - + selftest/knownfail.d/confidential_attr | 15 -- + source4/dsdb/samdb/ldb_modules/acl_read.c | 247 ++++++++++++++++++++++ + 3 files changed, 247 insertions(+), 16 deletions(-) + delete mode 100644 selftest/knownfail.d/acl + delete mode 100644 selftest/knownfail.d/confidential_attr + +diff --git a/selftest/knownfail.d/acl b/selftest/knownfail.d/acl +deleted file mode 100644 +index 6772ea1f943..00000000000 +--- a/selftest/knownfail.d/acl ++++ /dev/null +@@ -1 +0,0 @@ +-^samba4.ldap.acl.python.*test_search7 +diff --git a/selftest/knownfail.d/confidential_attr b/selftest/knownfail.d/confidential_attr +deleted file mode 100644 +index 7a2f2aada57..00000000000 +--- a/selftest/knownfail.d/confidential_attr ++++ /dev/null +@@ -1,15 +0,0 @@ +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_basic_search\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_acl_override\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_attr_acl_override\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_blanket_oa_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_neutral_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_neutral_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_neutral_attr_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_neutral_cr_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTest.test_search_with_propset_acl_override\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTestDenyAcl.test_search_with_blanket_oa_deny_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTestDenyAcl.test_search_with_deny_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTestDenyAcl.test_search_with_deny_attr_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTestDenyAcl.test_search_with_deny_propset_acl\(ad_dc_ntvfs\) +-samba4.ldap.confidential_attr.python\(ad_dc_ntvfs\).__main__.ConfidentialAttrTestDirsync.test_search_with_dirsync\(ad_dc_ntvfs\) +- +diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c +index 9607ed05ee7..280845a47a5 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl_read.c ++++ b/source4/dsdb/samdb/ldb_modules/acl_read.c +@@ -38,6 +38,8 @@ + #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; +@@ -262,6 +264,219 @@ static uint32_t get_attr_access_mask(const struct dsdb_attribute *attr, + return access_mask; + } + ++/* helper struct for traversing the attributes in the search-tree */ ++struct parse_tree_aclread_ctx { ++ struct aclread_context *ac; ++ TALLOC_CTX *mem_ctx; ++ struct dom_sid *sid; ++ struct ldb_dn *dn; ++ struct security_descriptor *sd; ++ const struct dsdb_class *objectclass; ++ bool suppress_result; ++}; ++ ++/* ++ * Checks that the user has sufficient access rights to view an attribute ++ */ ++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) ++{ ++ 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, ++ LDB_DEBUG_TRACE, ++ "acl_read: %s cannot find attr[%s] in schema," ++ "ignoring\n", ++ ldb_dn_get_linearized(dn), 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 */ ++ if (access_mask == 0) { ++ DBG_ERR("Could not determine access mask for attribute %s\n", ++ attr_name); ++ return LDB_SUCCESS; ++ } ++ ++ ret = acl_check_access_on_attribute(ac->module, mem_ctx, sd, sid, ++ access_mask, attr, objectclass); ++ ++ if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { ++ return ret; ++ } ++ ++ if (ret != LDB_SUCCESS) { ++ ldb_debug_set(ldb, LDB_DEBUG_FATAL, ++ "acl_read: %s check attr[%s] gives %s - %s\n", ++ ldb_dn_get_linearized(dn), attr_name, ++ ldb_strerror(ret), ldb_errstring(ldb)); ++ return ret; ++ } ++ ++ return LDB_SUCCESS; ++} ++ ++/* ++ * Returns the attribute name for this particular level of a search operation ++ * parse-tree. ++ */ ++static const char * parse_tree_get_attr(struct ldb_parse_tree *tree) ++{ ++ const char *attr = NULL; ++ ++ switch (tree->operation) { ++ case LDB_OP_EQUALITY: ++ case LDB_OP_GREATER: ++ case LDB_OP_LESS: ++ case LDB_OP_APPROX: ++ attr = tree->u.equality.attr; ++ break; ++ case LDB_OP_SUBSTRING: ++ attr = tree->u.substring.attr; ++ break; ++ case LDB_OP_PRESENT: ++ attr = tree->u.present.attr; ++ break; ++ case LDB_OP_EXTENDED: ++ attr = tree->u.extended.attr; ++ break; ++ ++ /* we'll check LDB_OP_AND/_OR/_NOT children later on in the walk */ ++ default: ++ break; ++ } ++ return attr; ++} ++ ++/* ++ * Checks a single attribute in the search parse-tree to make sure the user has ++ * sufficient rights to view it. ++ */ ++static int parse_tree_check_attr_access(struct ldb_parse_tree *tree, ++ void *private_context) ++{ ++ struct parse_tree_aclread_ctx *ctx = NULL; ++ const char *attr_name = NULL; ++ bool is_public_info = false; ++ int ret; ++ ++ ctx = (struct parse_tree_aclread_ctx *)private_context; ++ ++ /* ++ * we can skip any further checking if we already know that this object ++ * shouldn't be visible in this user's search ++ */ ++ if (ctx->suppress_result) { ++ return LDB_SUCCESS; ++ } ++ ++ /* skip this level of the search-tree if it has no attribute to check */ ++ attr_name = parse_tree_get_attr(tree); ++ if (attr_name == NULL) { ++ 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); ++ ++ /* ++ * if the user does not have the rights to view this attribute, then we ++ * should not return the object as a search result, i.e. act as if the ++ * 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; ++ } ++ ++ return ret; ++} ++ ++/* ++ * Traverse the search-tree to check that the user has sufficient access rights ++ * to view all the attributes. ++ */ ++static int check_search_ops_access(struct aclread_context *ac, ++ TALLOC_CTX *mem_ctx, ++ struct security_descriptor *sd, ++ const struct dsdb_class *objectclass, ++ struct dom_sid *sid, struct ldb_dn *dn, ++ bool *suppress_result) ++{ ++ int ret; ++ struct parse_tree_aclread_ctx ctx = { 0 }; ++ struct ldb_parse_tree *tree = ac->req->op.search.tree; ++ ++ ctx.ac = ac; ++ ctx.mem_ctx = mem_ctx; ++ ctx.suppress_result = false; ++ ctx.sid = sid; ++ ctx.dn = dn; ++ ctx.sd = sd; ++ ctx.objectclass = objectclass; ++ ++ /* walk the search tree, checking each attribute as we go */ ++ ret = ldb_parse_tree_walk(tree, parse_tree_check_attr_access, &ctx); ++ ++ /* return whether this search result should be hidden to this user */ ++ *suppress_result = ctx.suppress_result; ++ return ret; ++} ++ + static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares) + { + struct ldb_context *ldb; +@@ -275,6 +490,7 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares) + TALLOC_CTX *tmp_ctx; + uint32_t instanceType; + const struct dsdb_class *objectclass; ++ bool suppress_result = false; + + ac = talloc_get_type(req->context, struct aclread_context); + ldb = ldb_module_get_ctx(ac->module); +@@ -436,6 +652,37 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares) + goto fail; + } + } ++ ++ /* ++ * check access rights for the search attributes, as well as the ++ * attribute values actually being returned ++ */ ++ ret = check_search_ops_access(ac, tmp_ctx, sd, objectclass, sid, ++ msg->dn, &suppress_result); ++ if (ret != LDB_SUCCESS) { ++ ldb_debug_set(ldb, LDB_DEBUG_FATAL, ++ "acl_read: %s check search ops %s - %s\n", ++ ldb_dn_get_linearized(msg->dn), ++ ldb_strerror(ret), ldb_errstring(ldb)); ++ goto fail; ++ } ++ ++ if (suppress_result) { ++ ++ /* ++ * As per the above logic, we strip replPropertyMetaData ++ * out of the msg so that the dirysync module will do ++ * what is needed (return just the objectGUID if it's, ++ * deleted, or remove the object if it is not). ++ */ ++ if (ac->indirsync) { ++ ldb_msg_remove_attr(msg, "replPropertyMetaData"); ++ } else { ++ talloc_free(tmp_ctx); ++ return LDB_SUCCESS; ++ } ++ } ++ + for (i=0; i < msg->num_elements; i++) { + if (!aclread_is_inaccessible(&msg->elements[i])) { + num_of_attrs++; +-- +2.18.0 + |