debian/changelog

+samba (2:4.5.12+dfsg-2+deb9u3) stretch-security; urgency=high
+
+  * Non-maintainer upload by the Security Team.
+  * Confidential attribute disclosure from the AD LDAP server (CVE-2018-10919)
+  * Insufficient input validation on client directory listing in libsmbclient
+    (CVE-2018-10858)
+
+ -- Salvatore Bonaccorso <carnil@debian.org>  Mon, 13 Aug 2018 22:49:33 +0200
+
 samba (2:4.5.12+dfsg-2+deb9u2) stretch-security; urgency=high
 
   * This is a security release in order to address the following defects:

debian/patches/CVE-2018-10858-4.6.patch

+From c6a7d5a2afaddc82038bdda1cd2717237c1301c0 Mon Sep 17 00:00:00 2001
+From: Jeremy Allison <jra@samba.org>
+Date: Fri, 15 Jun 2018 15:07:17 -0700
+Subject: [PATCH 1/2] libsmb: Ensure smbc_urlencode() can't overwrite passed in
+ buffer.
+
+BUG: https://bugzilla.samba.org/show_bug.cgi?id=13453
+
+CVE-2018-10858: Insufficient input validation on client directory
+		listing in libsmbclient.
+
+Signed-off-by: Jeremy Allison <jra@samba.org>
+Reviewed-by: Ralph Boehme <slow@samba.org>
+---
+ source3/libsmb/libsmb_path.c | 9 +++++++--
+ 1 file changed, 7 insertions(+), 2 deletions(-)
+
+diff --git a/source3/libsmb/libsmb_path.c b/source3/libsmb/libsmb_path.c
+index 01b0a61e483..ed70ab37550 100644
+--- a/source3/libsmb/libsmb_path.c
++++ b/source3/libsmb/libsmb_path.c
+@@ -173,8 +173,13 @@ smbc_urlencode(char *dest,
+                 }
+         }
+ 
+-        *dest++ = '\0';
+-        max_dest_len--;
++	if (max_dest_len == 0) {
++		/* Ensure we return -1 if no null termination. */
++		return -1;
++	}
++
++	*dest++ = '\0';
++	max_dest_len--;
+ 
+         return max_dest_len;
+ }
+-- 
+2.11.0
+
+
+From c712faa47b0bf693a8dda408a7fd8a2938ce0024 Mon Sep 17 00:00:00 2001
+From: Jeremy Allison <jra@samba.org>
+Date: Fri, 15 Jun 2018 15:08:17 -0700
+Subject: [PATCH 2/2] libsmb: Harden smbc_readdir_internal() against returns
+ from malicious servers.
+
+BUG: https://bugzilla.samba.org/show_bug.cgi?id=13453
+
+CVE-2018-10858: Insufficient input validation on client directory
+		listing in libsmbclient.
+
+Signed-off-by: Jeremy Allison <jra@samba.org>
+Reviewed-by: Ralph Boehme <slow@samba.org>
+---
+ source3/libsmb/libsmb_dir.c  | 57 ++++++++++++++++++++++++++++++++++++++------
+ source3/libsmb/libsmb_path.c |  2 +-
+ 2 files changed, 51 insertions(+), 8 deletions(-)
+
+diff --git a/source3/libsmb/libsmb_dir.c b/source3/libsmb/libsmb_dir.c
+index 631459156d5..96eb1631542 100644
+--- a/source3/libsmb/libsmb_dir.c
++++ b/source3/libsmb/libsmb_dir.c
+@@ -930,27 +930,47 @@ SMBC_closedir_ctx(SMBCCTX *context,
+ 
+ }
+ 
+-static void
++static int
+ smbc_readdir_internal(SMBCCTX * context,
+                       struct smbc_dirent *dest,
+                       struct smbc_dirent *src,
+                       int max_namebuf_len)
+ {
+         if (smbc_getOptionUrlEncodeReaddirEntries(context)) {
++		int remaining_len;
+ 
+                 /* url-encode the name.  get back remaining buffer space */
+-                max_namebuf_len =
++                remaining_len =
+                         smbc_urlencode(dest->name, src->name, max_namebuf_len);
+ 
++		/* -1 means no null termination. */
++		if (remaining_len < 0) {
++			return -1;
++		}
++
+                 /* We now know the name length */
+                 dest->namelen = strlen(dest->name);
+ 
++		if (dest->namelen + 1 < 1) {
++			/* Integer wrap. */
++			return -1;
++		}
++
++		if (dest->namelen + 1 >= max_namebuf_len) {
++			/* Out of space for comment. */
++			return -1;
++		}
++
+                 /* Save the pointer to the beginning of the comment */
+                 dest->comment = dest->name + dest->namelen + 1;
+ 
++		if (remaining_len < 1) {
++			/* No room for comment null termination. */
++			return -1;
++		}
++
+                 /* Copy the comment */
+-                strncpy(dest->comment, src->comment, max_namebuf_len - 1);
+-                dest->comment[max_namebuf_len - 1] = '\0';
++                strlcpy(dest->comment, src->comment, remaining_len);
+ 
+                 /* Save other fields */
+                 dest->smbc_type = src->smbc_type;
+@@ -960,10 +980,21 @@ smbc_readdir_internal(SMBCCTX * context,
+         } else {
+ 
+                 /* No encoding.  Just copy the entry as is. */
++		if (src->dirlen > max_namebuf_len) {
++			return -1;
++		}
+                 memcpy(dest, src, src->dirlen);
++		if (src->namelen + 1 < 1) {
++			/* Integer wrap */
++			return -1;
++		}
++		if (src->namelen + 1 >= max_namebuf_len) {
++			/* Comment off the end. */
++			return -1;
++		}
+                 dest->comment = (char *)(&dest->name + src->namelen + 1);
+         }
+-
++	return 0;
+ }
+ 
+ /*
+@@ -975,6 +1006,7 @@ SMBC_readdir_ctx(SMBCCTX *context,
+                  SMBCFILE *dir)
+ {
+         int maxlen;
++	int ret;
+ 	struct smbc_dirent *dirp, *dirent;
+ 	TALLOC_CTX *frame = talloc_stackframe();
+ 
+@@ -1024,7 +1056,12 @@ SMBC_readdir_ctx(SMBCCTX *context,
+         dirp = &context->internal->dirent;
+         maxlen = sizeof(context->internal->_dirent_name);
+ 
+-        smbc_readdir_internal(context, dirp, dirent, maxlen);
++        ret = smbc_readdir_internal(context, dirp, dirent, maxlen);
++	if (ret == -1) {
++		errno = EINVAL;
++		TALLOC_FREE(frame);
++                return NULL;
++	}
+ 
+         dir->dir_next = dir->dir_next->next;
+ 
+@@ -1082,6 +1119,7 @@ SMBC_getdents_ctx(SMBCCTX *context,
+ 	 */
+ 
+ 	while ((dirlist = dir->dir_next)) {
++		int ret;
+ 		struct smbc_dirent *dirent;
+ 		struct smbc_dirent *currentEntry = (struct smbc_dirent *)ndir;
+ 
+@@ -1096,8 +1134,13 @@ SMBC_getdents_ctx(SMBCCTX *context,
+                 /* Do urlencoding of next entry, if so selected */
+                 dirent = &context->internal->dirent;
+                 maxlen = sizeof(context->internal->_dirent_name);
+-                smbc_readdir_internal(context, dirent,
++		ret = smbc_readdir_internal(context, dirent,
+                                       dirlist->dirent, maxlen);
++		if (ret == -1) {
++			errno = EINVAL;
++			TALLOC_FREE(frame);
++			return -1;
++		}
+ 
+                 reqd = dirent->dirlen;
+ 
+diff --git a/source3/libsmb/libsmb_path.c b/source3/libsmb/libsmb_path.c
+index ed70ab37550..5b53b386a67 100644
+--- a/source3/libsmb/libsmb_path.c
++++ b/source3/libsmb/libsmb_path.c
+@@ -173,7 +173,7 @@ smbc_urlencode(char *dest,
+                 }
+         }
+ 
+-	if (max_dest_len == 0) {
++	if (max_dest_len <= 0) {
+ 		/* Ensure we return -1 if no null termination. */
+ 		return -1;
+ 	}
+-- 
+2.11.0
+

debian/patches/series

@@ -23,3 +23,5 @@ s3-smbd-Chain-code-can-return-uninitialized-memory-w.patch
 s3-smbd-Fix-SMB1-use-after-free-crash-bug.-CVE-2017-.patch
 CVE-2018-1050-11343-4.5.patch
 CVE-2018-1057-v4-5.metze01.patches.txt
+CVE-2018-10919.patch
+CVE-2018-10858-4.6.patch

libcli/security/access_check.c

@@ -374,6 +374,81 @@ static const struct GUID *get_ace_object_type(struct security_ace *ace)
 	return NULL;
 }
 
+/**
+ * Evaluates access rights specified in a object-specific ACE for an AD object.
+ * This logic corresponds to MS-ADTS 5.1.3.3.3 Checking Object-Specific Access.
+ * @param[in] ace - the ACE being processed
+ * @param[in/out] tree - remaining_access gets updated for the tree
+ * @param[out] grant_access - set to true if the ACE grants sufficient access
+ *                            rights to the object/attribute
+ * @returns NT_STATUS_OK, unless access was denied
+ */
+static NTSTATUS check_object_specific_access(struct security_ace *ace,
+					     struct object_tree *tree,
+					     bool *grant_access)
+{
+	struct object_tree *node = NULL;
+	const struct GUID *type = NULL;
+
+	*grant_access = false;
+
+	/* if no tree was supplied, we can't do object-specific access checks */
+	if (!tree) {
+		return NT_STATUS_OK;
+	}
+
+	/* Get the ObjectType GUID this ACE applies to */
+	type = get_ace_object_type(ace);
+
+	/*
+	 * If the ACE doesn't have a type, then apply it to the whole tree, i.e.
+	 * treat 'OA' ACEs as 'A' and 'OD' as 'D'
+	 */
+	if (!type) {
+		node = tree;
+	} else {
+
+		/* skip it if the ACE's ObjectType GUID is not in the tree */
+		node = get_object_tree_by_GUID(tree, type);
+		if (!node) {
+			return NT_STATUS_OK;
+		}
+	}
+
+	if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
+
+		/* apply the access rights to this node, and any children */
+		object_tree_modify_access(node, ace->access_mask);
+
+		/*
+		 * Currently all nodes in the tree request the same access mask,
+		 * so we can use any node to check if processing this ACE now
+		 * means the requested access has been granted
+		 */
+		if (node->remaining_access == 0) {
+			*grant_access = true;
+			return NT_STATUS_OK;
+		}
+
+		/*
+		 * As per 5.1.3.3.4 Checking Control Access Right-Based Access,
+		 * if the CONTROL_ACCESS right is present, then we can grant
+		 * access and stop any further access checks
+		 */
+		if (ace->access_mask & SEC_ADS_CONTROL_ACCESS) {
+			*grant_access = true;
+			return NT_STATUS_OK;
+		}
+	} else {
+
+		/* this ACE denies access to the requested object/attribute */
+		if (node->remaining_access & ace->access_mask){
+			return NT_STATUS_ACCESS_DENIED;
+		}
+	}
+	return NT_STATUS_OK;
+}
+
 /**
  * @brief Perform directoryservice (DS) related access checks for a given user
  *
@@ -405,8 +480,6 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 {
 	uint32_t i;
 	uint32_t bits_remaining;
-	struct object_tree *node;
-	const struct GUID *type;
 	struct dom_sid self_sid;
 
 	dom_sid_parse(SID_NT_SELF, &self_sid);
@@ -456,6 +529,8 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 	for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
 		struct dom_sid *trustee;
 		struct security_ace *ace = &sd->dacl->aces[i];
+		NTSTATUS status;
+		bool grant_access = false;
 
 		if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
 			continue;
@@ -486,35 +561,16 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 			break;
 		case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT:
 		case SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT:
-			/*
-			 * check only in case we have provided a tree,
-			 * the ACE has an object type and that type
-			 * is in the tree
-			 */
-			type = get_ace_object_type(ace);
+			status = check_object_specific_access(ace, tree,
+							      &grant_access);
 
-			if (!tree) {
-				continue;
-			}
-
-			if (!type) {
-				node = tree;
-			} else {
-				if (!(node = get_object_tree_by_GUID(tree, type))) {
-					continue;
-				}
+			if (!NT_STATUS_IS_OK(status)) {
+				return status;
 			}
 
-			if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
-				object_tree_modify_access(node, ace->access_mask);
-				if (node->remaining_access == 0) {
+			if (grant_access) {
 				return NT_STATUS_OK;
 			}
-			} else {
-				if (node->remaining_access & ace->access_mask){
-					return NT_STATUS_ACCESS_DENIED;
-				}
-			}
 			break;
 		default:	/* Other ACE types not handled/supported */
 			break;

source3/libsmb/libsmb_dir.c

@@ -929,27 +929,47 @@ SMBC_closedir_ctx(SMBCCTX *context,
 
 }
 
-static void
+static int
 smbc_readdir_internal(SMBCCTX * context,
                       struct smbc_dirent *dest,
                       struct smbc_dirent *src,
                       int max_namebuf_len)
 {
         if (smbc_getOptionUrlEncodeReaddirEntries(context)) {
+		int remaining_len;
 
                 /* url-encode the name.  get back remaining buffer space */
-                max_namebuf_len =
+                remaining_len =
                         smbc_urlencode(dest->name, src->name, max_namebuf_len);
 
+		/* -1 means no null termination. */
+		if (remaining_len < 0) {
+			return -1;
+		}
+
                 /* We now know the name length */
                 dest->namelen = strlen(dest->name);
 
+		if (dest->namelen + 1 < 1) {
+			/* Integer wrap. */
+			return -1;
+		}
+
+		if (dest->namelen + 1 >= max_namebuf_len) {
+			/* Out of space for comment. */
+			return -1;
+		}
+
                 /* Save the pointer to the beginning of the comment */
                 dest->comment = dest->name + dest->namelen + 1;
 
+		if (remaining_len < 1) {
+			/* No room for comment null termination. */
+			return -1;
+		}
+
                 /* Copy the comment */
-                strncpy(dest->comment, src->comment, max_namebuf_len - 1);
-                dest->comment[max_namebuf_len - 1] = '\0';
+                strlcpy(dest->comment, src->comment, remaining_len);
 
                 /* Save other fields */
                 dest->smbc_type = src->smbc_type;
@@ -959,10 +979,21 @@ smbc_readdir_internal(SMBCCTX * context,
         } else {
 
                 /* No encoding.  Just copy the entry as is. */
+		if (src->dirlen > max_namebuf_len) {
+			return -1;
+		}
                 memcpy(dest, src, src->dirlen);
+		if (src->namelen + 1 < 1) {
+			/* Integer wrap */
+			return -1;
+		}
+		if (src->namelen + 1 >= max_namebuf_len) {
+			/* Comment off the end. */
+			return -1;
+		}
                 dest->comment = (char *)(&dest->name + src->namelen + 1);
         }
-
+	return 0;
 }
 
 /*
@@ -974,6 +1005,7 @@ SMBC_readdir_ctx(SMBCCTX *context,
                  SMBCFILE *dir)
 {
         int maxlen;
+	int ret;
 	struct smbc_dirent *dirp, *dirent;
 	TALLOC_CTX *frame = talloc_stackframe();
 
@@ -1023,7 +1055,12 @@ SMBC_readdir_ctx(SMBCCTX *context,
         dirp = &context->internal->dirent;
         maxlen = sizeof(context->internal->_dirent_name);
 
-        smbc_readdir_internal(context, dirp, dirent, maxlen);
+        ret = smbc_readdir_internal(context, dirp, dirent, maxlen);
+	if (ret == -1) {
+		errno = EINVAL;
+		TALLOC_FREE(frame);
+                return NULL;
+	}
 
         dir->dir_next = dir->dir_next->next;
 
@@ -1081,6 +1118,7 @@ SMBC_getdents_ctx(SMBCCTX *context,
 	 */
 
 	while ((dirlist = dir->dir_next)) {
+		int ret;
 		struct smbc_dirent *dirent;
 		struct smbc_dirent *currentEntry = (struct smbc_dirent *)ndir;
 
@@ -1095,8 +1133,13 @@ SMBC_getdents_ctx(SMBCCTX *context,
                 /* Do urlencoding of next entry, if so selected */
                 dirent = &context->internal->dirent;
                 maxlen = sizeof(context->internal->_dirent_name);
-                smbc_readdir_internal(context, dirent,
+		ret = smbc_readdir_internal(context, dirent,
                                       dirlist->dirent, maxlen);
+		if (ret == -1) {
+			errno = EINVAL;
+			TALLOC_FREE(frame);
+			return -1;
+		}
 
                 reqd = dirent->dirlen;
 

source3/libsmb/libsmb_path.c

@@ -173,6 +173,11 @@ smbc_urlencode(char *dest,
                 }
         }
 
+	if (max_dest_len <= 0) {
+		/* Ensure we return -1 if no null termination. */
+		return -1;
+	}
+
 	*dest++ = '\0';
 	max_dest_len--;
 

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;
@@ -64,6 +66,254 @@ static bool aclread_is_inaccessible(struct ldb_message_element *el) {
 	return el->flags & LDB_FLAG_INTERNAL_INACCESSIBLE_ATTRIBUTE;
 }
 
+/*
+ * Returns the access mask required to read a given attribute
+ */
+static uint32_t get_attr_access_mask(const struct dsdb_attribute *attr,
+				     uint32_t sd_flags)
+{
+
+	uint32_t access_mask = 0;
+	bool is_sd;
+
+	/* nTSecurityDescriptor is a special case */
+	is_sd = (ldb_attr_cmp("nTSecurityDescriptor",
+			      attr->lDAPDisplayName) == 0);
+
+	if (is_sd) {
+		if (sd_flags & (SECINFO_OWNER|SECINFO_GROUP)) {
+			access_mask |= SEC_STD_READ_CONTROL;
+		}
+		if (sd_flags & SECINFO_DACL) {
+			access_mask |= SEC_STD_READ_CONTROL;
+		}
+		if (sd_flags & SECINFO_SACL) {
+			access_mask |= SEC_FLAG_SYSTEM_SECURITY;
+		}
+	} else {
+		access_mask = SEC_ADS_READ_PROP;
+	}
+
+	if (attr->searchFlags & SEARCH_FLAG_CONFIDENTIAL) {
+		access_mask |= SEC_ADS_CONTROL_ACCESS;
+	}
+
+	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;
@@ -77,6 +327,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);
@@ -183,26 +434,7 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares)
 				aclread_mark_inaccesslible(&msg->elements[i]);
 				continue;
 			}
-			/* nTSecurityDescriptor is a special case */
-			if (is_sd) {
-				access_mask = 0;
-
-				if (ac->sd_flags & (SECINFO_OWNER|SECINFO_GROUP)) {
-					access_mask |= SEC_STD_READ_CONTROL;
-				}
-				if (ac->sd_flags & SECINFO_DACL) {
-					access_mask |= SEC_STD_READ_CONTROL;
-				}
-				if (ac->sd_flags & SECINFO_SACL) {
-					access_mask |= SEC_FLAG_SYSTEM_SECURITY;
-				}
-			} else {
-				access_mask = SEC_ADS_READ_PROP;
-			}
-
-			if (attr->searchFlags & SEARCH_FLAG_CONFIDENTIAL) {
-				access_mask |= SEC_ADS_CONTROL_ACCESS;
-			}
+			access_mask = get_attr_access_mask(attr, ac->sd_flags);
 
 			if (access_mask == 0) {
 				aclread_mark_inaccesslible(&msg->elements[i]);
@@ -223,18 +455,14 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares)
 			 * in anycase.
 			 */
 			if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) {
-				if (!ac->indirsync) {
-					/*
-					 * do not return this entry if attribute is
-					 * part of the search filter
-					 */
-					if (dsdb_attr_in_parse_tree(ac->req->op.search.tree,
-								msg->elements[i].name)) {
-						talloc_free(tmp_ctx);
-						return LDB_SUCCESS;
-					}
-					aclread_mark_inaccesslible(&msg->elements[i]);
-				} else {
+				bool in_search_filter;
+
+				/* check if attr is part of the search filter */
+				in_search_filter = dsdb_attr_in_parse_tree(ac->req->op.search.tree,
+								msg->elements[i].name);
+
+				if (in_search_filter) {
+
 					/*
 					 * We are doing dirysnc answers
 					 * and the object shouldn't be returned (normally)
@@ -243,13 +471,17 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares)
 					 * (remove the object if it is not deleted, or return
 					 * just the objectGUID if it's deleted).
 					 */
-					if (dsdb_attr_in_parse_tree(ac->req->op.search.tree,
-								msg->elements[i].name)) {
+					if (ac->indirsync) {
 						ldb_msg_remove_attr(msg, "replPropertyMetaData");
 						break;
 					} else {
-						aclread_mark_inaccesslible(&msg->elements[i]);
+
+						/* do not return this entry */
+						talloc_free(tmp_ctx);
+						return LDB_SUCCESS;
 					}
+				} else {
+					aclread_mark_inaccesslible(&msg->elements[i]);
 				}
 			} else if (ret != LDB_SUCCESS) {
 				ldb_debug_set(ldb, LDB_DEBUG_FATAL,
@@ -261,6 +493,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++;

source4/dsdb/tests/python/acl.py

@@ -981,6 +981,74 @@ class AclSearchTests(AclTests):
         res_list = res[0].keys()
         self.assertEquals(sorted(res_list), sorted(ok_list))
 
+    def assert_search_on_attr(self, dn, samdb, attr, expected_list):
+
+        expected_num = len(expected_list)
+        res = samdb.search(dn, expression="(%s=*)" % attr, scope=SCOPE_SUBTREE)
+        self.assertEquals(len(res), expected_num)
+
+        res_list = [ x["dn"] for x in res if x["dn"] in expected_list ]
+        self.assertEquals(sorted(res_list), sorted(expected_list))
+
+    def test_search7(self):
+        """Checks object search visibility when users don't have full rights"""
+        self.create_clean_ou("OU=ou1," + self.base_dn)
+        mod = "(A;;LC;;;%s)(A;;LC;;;%s)" % (str(self.user_sid),
+                                            str(self.group_sid))
+        self.sd_utils.dacl_add_ace("OU=ou1," + self.base_dn, mod)
+        tmp_desc = security.descriptor.from_sddl("D:(A;;RPWPCRCCDCLCLORCWOWDSDDTSW;;;DA)" + mod,
+                                                 self.domain_sid)
+        self.ldb_admin.create_ou("OU=ou2,OU=ou1," + self.base_dn, sd=tmp_desc)
+        self.ldb_admin.create_ou("OU=ou3,OU=ou2,OU=ou1," + self.base_dn,
+                                 sd=tmp_desc)
+        self.ldb_admin.create_ou("OU=ou4,OU=ou2,OU=ou1," + self.base_dn,
+                                 sd=tmp_desc)
+        self.ldb_admin.create_ou("OU=ou5,OU=ou3,OU=ou2,OU=ou1," + self.base_dn,
+                                 sd=tmp_desc)
+        self.ldb_admin.create_ou("OU=ou6,OU=ou4,OU=ou2,OU=ou1," + self.base_dn,
+                                 sd=tmp_desc)
+
+        ou2_dn = Dn(self.ldb_admin,  "OU=ou2,OU=ou1," + self.base_dn)
+        ou1_dn = Dn(self.ldb_admin,  "OU=ou1," + self.base_dn)
+
+        # even though unprivileged users can't read these attributes for OU2,
+        # the object should still be visible in searches, because they have
+        # 'List Contents' rights still. This isn't really disclosive because
+        # ALL objects have these attributes
+        visible_attrs = ["objectClass", "distinguishedName", "name",
+                         "objectGUID"]
+        two_objects = [ou2_dn, ou1_dn]
+
+        for attr in visible_attrs:
+            # a regular user should just see the 2 objects
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user3, attr,
+                                       expected_list=two_objects)
+
+            # whereas the following users have LC rights for all the objects,
+            # so they should see them all
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user, attr,
+                                       expected_list=self.full_list)
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user2, attr,
+                                       expected_list=self.full_list)
+
+        # however when searching on the following attributes, objects will not
+        # be visible unless the user has Read Property rights
+        hidden_attrs = ["objectCategory", "instanceType", "ou", "uSNChanged",
+                        "uSNCreated", "whenCreated"]
+        one_object = [ou1_dn]
+
+        for attr in hidden_attrs:
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user3, attr,
+                                       expected_list=one_object)
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user, attr,
+                                       expected_list=one_object)
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_user2, attr,
+                                       expected_list=one_object)
+
+            # admin has RP rights so can still see all the objects
+            self.assert_search_on_attr(str(ou1_dn), self.ldb_admin, attr,
+                                       expected_list=self.full_list)
+
 #tests on ldap delete operations
 class AclDeleteTests(AclTests):
 

source4/dsdb/tests/python/ldap.py

@@ -599,6 +599,15 @@ class BasicTests(samba.tests.TestCase):
         except LdbError, (num, _):
             self.assertEquals(num, ERR_NO_SUCH_ATTRIBUTE)
 
+        #
+        # When searching the unknown attribute should be ignored
+        expr = "(|(cn=ldaptestgroup)(thisdoesnotexist=x))"
+        res = ldb.search(base=self.base_dn,
+                         expression=expr,
+                         scope=SCOPE_SUBTREE)
+        self.assertTrue(len(res) == 1,
+                        "Search including unknown attribute failed")
+
         delete_force(self.ldb, "cn=ldaptestgroup,cn=users," + self.base_dn)
 
         # attributes not in objectclasses and mandatory attributes missing test

source4/selftest/tests.py

@@ -606,6 +606,9 @@ for env in ["ad_dc_ntvfs", "fl2000dc", "fl2003dc", "fl2008r2dc"]:
         # therefore skip it in that configuration
         plantestsuite_loadlist("samba4.ldap.passwords.python(%s)" % env, env, [python, os.path.join(samba4srcdir, "dsdb/tests/python/passwords.py"), "$SERVER", '-U"$USERNAME%$PASSWORD"', "-W$DOMAIN", '$LOADLIST', '$LISTOPT'])
 
+env = "ad_dc_ntvfs"
+plantestsuite_loadlist("samba4.ldap.confidential_attr.python(%s)" % env, env, [python, os.path.join(samba4srcdir, "dsdb/tests/python/confidential_attr.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT'])
+
 for env in ["ad_dc_ntvfs"]:
     # This test takes a lot of time, so we run it against a minimum of
     # environments, please only add new ones if there's really a