From: Steven Danneman <steven.danneman@isilon.com>
Date: Sat, 15 Nov 2008 21:07:15 +0000 (-0800)
Subject: Fix extended DN parse error when AD object does not have a SID.
X-Git-Url: http://git.samba.org/?p=metze%2Fsamba%2Fwip.git;a=commitdiff_plain;h=6d59be1e6d83d4faf145c9b6d574bab9f2acb36a

Fix extended DN parse error when AD object does not have a SID.

Some AD objects, like Exchange Public Folders, can be members of Security
Groups but do not have a SID attribute.  This patch adds more granular return
errors to ads_get_sid_from_extended_dn().  Callers can now determine if a parse
error occured because of bad input, or the DN was valid but contained no SID.

I updated all callers to ignore SIDless objects when appropriate.

Also did some cleanup to the out paths of lookup_usergroups_memberof()
---

diff --git a/source3/include/proto.h b/source3/include/proto.h
index 33425849d1fc..1cdf6c9cbc85 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -1920,10 +1920,10 @@ ADS_STATUS ads_get_joinable_ous(ADS_STRUCT *ads,
 				TALLOC_CTX *mem_ctx,
 				char ***ous,
 				size_t *num_ous);
-bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx, 
-				  const char *extended_dn, 
-				  enum ads_extended_dn_flags flags, 
-				  DOM_SID *sid);
+ADS_STATUS ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
+					const char *extended_dn,
+					enum ads_extended_dn_flags flags,
+					DOM_SID *sid);
 char* ads_get_dnshostname( ADS_STRUCT *ads, TALLOC_CTX *ctx, const char *machine_name );
 char* ads_get_upn( ADS_STRUCT *ads, TALLOC_CTX *ctx, const char *machine_name );
 char* ads_get_samaccountname( ADS_STRUCT *ads, TALLOC_CTX *ctx, const char *machine_name );
diff --git a/source3/libads/ldap.c b/source3/libads/ldap.c
index c651b33efe91..f55cfa784a35 100644
--- a/source3/libads/ldap.c
+++ b/source3/libads/ldap.c
@@ -3115,45 +3115,51 @@ ADS_STATUS ads_get_joinable_ous(ADS_STRUCT *ads,
  * @param extended_dn string
  * @param flags string type of extended_dn
  * @param sid pointer to a DOM_SID
- * @return boolean inidicating success
+ * @return NT_STATUS_OK on success,
+ *	   NT_INVALID_PARAMETER on error,
+ *	   NT_STATUS_NOT_FOUND if no SID present
  **/
-bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
-				  const char *extended_dn,
-				  enum ads_extended_dn_flags flags,
-				  DOM_SID *sid)
+ADS_STATUS ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
+					const char *extended_dn,
+					enum ads_extended_dn_flags flags,
+					DOM_SID *sid)
 {
 	char *p, *q, *dn;
 
 	if (!extended_dn) {
-		return False;
+		return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 	}
 
 	/* otherwise extended_dn gets stripped off */
 	if ((dn = talloc_strdup(mem_ctx, extended_dn)) == NULL) {
-		return False;
+		return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 	}
 	/*
 	 * ADS_EXTENDED_DN_HEX_STRING:
 	 * <GUID=238e1963cb390f4bb032ba0105525a29>;<SID=010500000000000515000000bb68c8fd6b61b427572eb04556040000>;CN=gd,OU=berlin,OU=suse,DC=ber,DC=suse,DC=de
 	 *
 	 * ADS_EXTENDED_DN_STRING (only with w2k3):
-	<GUID=63198e23-39cb-4b0f-b032-ba0105525a29>;<SID=S-1-5-21-4257769659-666132843-1169174103-1110>;CN=gd,OU=berlin,OU=suse,DC=ber,DC=suse,DC=de
+	 * <GUID=63198e23-39cb-4b0f-b032-ba0105525a29>;<SID=S-1-5-21-4257769659-666132843-1169174103-1110>;CN=gd,OU=berlin,OU=suse,DC=ber,DC=suse,DC=de
+	 *
+	 * Object with no SID, such as an Exchange Public Folder
+	 * <GUID=28907fb4bdf6854993e7f0a10b504e7c>;CN=public,CN=Microsoft Exchange System Objects,DC=sd2k3ms,DC=west,DC=isilon,DC=com
 	 */
 
 	p = strchr(dn, ';');
 	if (!p) {
-		return False;
+		return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 	}
 
 	if (strncmp(p, ";<SID=", strlen(";<SID=")) != 0) {
-		return False;
+		DEBUG(5,("No SID present in extended dn\n"));
+		return ADS_ERROR_NT(NT_STATUS_NOT_FOUND);
 	}
 
 	p += strlen(";<SID=");
 
 	q = strchr(p, '>');
 	if (!q) {
-		return False;
+		return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 	}
 
 	*q = '\0';
@@ -3164,7 +3170,7 @@ bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
 
 	case ADS_EXTENDED_DN_STRING:
 		if (!string_to_sid(sid, p)) {
-			return False;
+			return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 		}
 		break;
 	case ADS_EXTENDED_DN_HEX_STRING: {
@@ -3173,21 +3179,21 @@ bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
 
 		buf_len = strhex_to_str(buf, sizeof(buf), p, strlen(p));
 		if (buf_len == 0) {
-			return False;
+			return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 		}
 
 		if (!sid_parse(buf, buf_len, sid)) {
 			DEBUG(10,("failed to parse sid\n"));
-			return False;
+			return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 		}
 		break;
 		}
 	default:
 		DEBUG(10,("unknown extended dn format\n"));
-		return False;
+		return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER);
 	}
 
-	return True;
+	return ADS_ERROR_NT(NT_STATUS_OK);
 }
 
 /**
@@ -3208,7 +3214,8 @@ bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
 				   DOM_SID **sids)
 {
 	int i;
-	size_t dn_count;
+	ADS_STATUS rc;
+	size_t dn_count, ret_count = 0;
 	char **dn_strings;
 
 	if ((dn_strings = ads_pull_strings(ads, mem_ctx, msg, field,
@@ -3223,18 +3230,25 @@ bool ads_get_sid_from_extended_dn(TALLOC_CTX *mem_ctx,
 	}
 
 	for (i=0; i<dn_count; i++) {
-
-		if (!ads_get_sid_from_extended_dn(mem_ctx, dn_strings[i],
-						  flags, &(*sids)[i])) {
-			TALLOC_FREE(*sids);
-			TALLOC_FREE(dn_strings);
-			return 0;
+		rc = ads_get_sid_from_extended_dn(mem_ctx, dn_strings[i],
+						  flags, &(*sids)[i]);
+		if (!ADS_ERR_OK(rc)) {
+			if (NT_STATUS_EQUAL(ads_ntstatus(rc),
+			    NT_STATUS_NOT_FOUND)) {
+				continue;
+			}
+			else {
+				TALLOC_FREE(*sids);
+				TALLOC_FREE(dn_strings);
+				return 0;
+			}
 		}
+		ret_count++;
 	}
 
 	TALLOC_FREE(dn_strings);
 
-	return dn_count;
+	return ret_count;
 }
 
 /********************************************************************
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index 1a5ed5f6c1eb..18cc1cbd0381 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -643,7 +643,8 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 					   TALLOC_CTX *mem_ctx,
 					   const char *user_dn,
 					   DOM_SID *primary_group,
-					   size_t *p_num_groups, DOM_SID **user_sids)
+					   size_t *p_num_groups,
+					   DOM_SID **user_sids)
 {
 	ADS_STATUS rc;
 	NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
@@ -652,8 +653,8 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 	size_t num_groups = 0;
 	DOM_SID *group_sids = NULL;
 	int i;
-	char **strings;
-	size_t num_strings = 0;
+	char **strings = NULL;
+	size_t num_strings = 0, num_sids = 0;
 
 
 	DEBUG(3,("ads: lookup_usergroups_memberof\n"));
@@ -668,7 +669,7 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 
 	if (!ads) {
 		domain->last_status = NT_STATUS_SERVER_DISABLED;
-		goto done;
+		return NT_STATUS_UNSUCCESSFUL;
 	}
 
 	rc = ads_search_retry_extended_dn_ranged(ads, mem_ctx, user_dn, attrs,
@@ -693,21 +694,26 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 
 	group_sids = TALLOC_ZERO_ARRAY(mem_ctx, DOM_SID, num_strings + 1);
 	if (!group_sids) {
-		TALLOC_FREE(strings);
 		status = NT_STATUS_NO_MEMORY;
 		goto done;
 	}
 
 	for (i=0; i<num_strings; i++) {
-
-		if (!ads_get_sid_from_extended_dn(mem_ctx, strings[i],
+		rc = ads_get_sid_from_extended_dn(mem_ctx, strings[i],
 						  ADS_EXTENDED_DN_HEX_STRING,
-						  &(group_sids)[i])) {
-			TALLOC_FREE(group_sids);
-			TALLOC_FREE(strings);
-			status = NT_STATUS_NO_MEMORY;
-			goto done;
+						  &(group_sids)[i]);
+		if (!ADS_ERR_OK(rc)) {
+			/* ignore members without SIDs */
+			if (NT_STATUS_EQUAL(ads_ntstatus(rc),
+			    NT_STATUS_NOT_FOUND)) {
+				continue;
+			}
+			else {
+				status = ads_ntstatus(rc);
+				goto done;
+			}
 		}
+		num_sids++;
 	}
 
 	if (i == 0) {
@@ -716,7 +722,7 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 		goto done;
 	}
 
-	for (i=0; i<num_strings; i++) {
+	for (i=0; i<num_sids; i++) {
 
 		/* ignore Builtin groups from ADS - Guenther */
 		if (sid_check_is_in_builtin(&group_sids[i])) {
@@ -734,8 +740,11 @@ static NTSTATUS lookup_usergroups_memberof(struct winbindd_domain *domain,
 	*p_num_groups = num_groups;
 	status = (*user_sids != NULL) ? NT_STATUS_OK : NT_STATUS_NO_MEMORY;
 
-	DEBUG(3,("ads lookup_usergroups (memberof) succeeded for dn=%s\n", user_dn));
+	DEBUG(3,("ads lookup_usergroups (memberof) succeeded for dn=%s\n",
+		user_dn));
+
 done:
+	TALLOC_FREE(strings);
 	TALLOC_FREE(group_sids);
 
 	return status;
@@ -1015,10 +1024,20 @@ static NTSTATUS lookup_groupmem(struct winbindd_domain *domain,
 		char *name, *domain_name;
 		DOM_SID sid;
 
-	        if (!ads_get_sid_from_extended_dn(tmp_ctx, members[i], args.val,
-		    &sid)) {
-			status = NT_STATUS_INVALID_PARAMETER;
-	                goto done;
+	        rc = ads_get_sid_from_extended_dn(tmp_ctx, members[i], args.val,
+		    &sid);
+		if (!ADS_ERR_OK(rc)) {
+			if (NT_STATUS_EQUAL(ads_ntstatus(rc),
+			    NT_STATUS_NOT_FOUND)) {
+				/* Group members can be objects, like Exchange
+				 * Public Folders, that don't have a SID.  Skip
+				 * them. */
+				continue;
+			}
+			else {
+				status = ads_ntstatus(rc);
+				goto done;
+			}
 		}
 		if (lookup_cached_sid(mem_ctx, &sid, &domain_name, &name,
 		    &name_type)) {