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 33425849d1fc94063646a6b283b459e194171451..1cdf6c9cbc859ead3d5dc5236ba44a0f11b592a7 100644 (file)
--- 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 c651b33efe91aa48a4057c9e6c2af19e1a6651d2..f55cfa784a35169b83f3560f9502764952ae4ca5 100644 (file)
--- 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 1a5ed5f6c1eb073e53dd9feb6ab62d2dd0430f4a..18cc1cbd038186fff12aa879173f74fb276c583b 100644 (file)
--- 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)) {