[PATCHES] winbindd: fix sid->xid for SID History SIDs

[Ralph Böhme]

Hi Uri,

On Mon, Mar 20, 2017 at 06:49:54AM +0200, Uri Simchoni via samba-technical wrote:
> We few weeks ago we've discussed SID history and id-mapping -
> https://lists.samba.org/archive/samba-technical/2017-February/118771.html.
> 
> Attached is a proposed initial fix for the issue, which focuses on
> avoiding wrong results.
> 
> The fix finds the domain of the SID by resolving a SID with same domain
> component and an RID of 513 (domain users), which hopefully never gets
> migrated.
> 
> We've discussed other means such as smb.conf stuff or netsamlogon - I
> think those methods can come on top of this method, because if they
> don't work we should always fall back to something. The added resolving
> doesn't cost much because it's in the same round-trip.
> 
> The key thing about this fix is that doesn't try to translate sid->xid
> in any possible case (such as when old domain is gone and forgotten), it
> just avoids getting the *wrong* result. As such, it's a good minimal fix
> that can be applied to stable versions. For master, we can add the
> smb.conf-based stuff, that will support more cases.
> 
> Review appreciated.
> Thanks,
> Uri.

mostly lgtm, just one issue, see below.

Fwiw, I'm currently working on another issue in sids2xids. Not really related
but I'm mentioning it here as you're currently having fun with the same area of
code.

We're leaving the decision whether to provide mapping for unresolved SIDs to the
idmap backend, which can lead to interesting issues. Eg, with the following
config:


        idmap config * : backend = autorid
        idmap config * : range = 1000000-1999999

        idmap config FOO : backend = rid
        idmap config FOO : range = 2000000 - 2999999

a sid2xid request for a SID from domain FOO that doesn't exist in the domain
may end up in the default idmap backend where autorid would happily assign a
range and provide a mapping.

I tried to address this issue in an earlier fix
d5af3f3b6565da624fe6f6e4cbea818392c0c68f for bug 11961. Unfortunately the fix
was not quite correct (to say the least...) and also introduced a similar
problem with idmap_rid: bug 12597.

Imho we should just fail early if a SID can't be resolved and refuse to provide
a UNIX id mapping, I have a WIP branch that does this here:

<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/winbindd-lookupsids-sids2xids>

> From 3fcf7130a086cb3c6bc3dc444dd1010d8078ae29 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Fri, 17 Mar 2017 23:58:35 +0200
> Subject: [PATCH 4/4] winbindd-sids2xids: reliably determine idmap backend name
> 
> For SIDs which belong to domains, determine the backend name
> by the domain name of "domain users" SID from the same domain.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
> 
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
>  source3/winbindd/wb_sids2xids.c | 61 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
> index 9da08d2..ea0d9c7 100644
> --- a/source3/winbindd/wb_sids2xids.c
> +++ b/source3/winbindd/wb_sids2xids.c
> @@ -207,6 +207,11 @@ static bool wb_add_domain_users_to_lookup(TALLOC_CTX *ctx,
>  }
>  
>  static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type);
> +static const char *
> +wb_get_idmap_backend_name(const struct lsa_RefDomainList *domains,
> +			  const struct lsa_TransNameArray *names,
> +			  const struct dom_sid *sids,
> +			  uint32_t idx);
>  static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index(
>  	TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src,
>  	uint32_t domain_index);
> @@ -238,7 +243,8 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
>  
>  	for (i=0; i<state->num_non_cached; i++) {
>  		struct dom_sid dom_sid;
> -		struct lsa_DomainInfo *info;
> +		const char *dom_name = wb_get_idmap_backend_name(
> +		    domains, names, state->non_cached, i);
>  		struct lsa_TranslatedName *n = &names->names[i];
>  		struct wbint_TransID *t = &state->ids.ids[i];
>  		int domain_index;
> @@ -246,11 +252,10 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
>  		sid_copy(&dom_sid, &state->non_cached[i]);
>  		sid_split_rid(&dom_sid, &t->rid);
>  
> -		info = &domains->domains[n->sid_index];
>  		t->type = lsa_SidType_to_id_type(n->sid_type);
>  
>  		domain_index = init_lsa_ref_domain_list(
> -			state, &state->idmap_doms, info->name.string, &dom_sid);
> +		    state, &state->idmap_doms, dom_name, &dom_sid);
>  		if (domain_index == -1) {
>  			tevent_req_oom(req);
>  			return;
> @@ -309,6 +314,56 @@ static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type)
>  	return type;
>  }
>  
> +static const char *
> +wb_get_idmap_backend_name(const struct lsa_RefDomainList *domains,
> +			  const struct lsa_TransNameArray *names,
> +			  const struct dom_sid *sids,
> +			  uint32_t idx)
> +{
> +	struct dom_sid sid = sids[idx];
> +	struct lsa_TranslatedName *n = &names->names[idx];
> +	struct lsa_DomainInfo *info = NULL;
> +	if (!is_domain_principal_sid(&sid)) {
> +		/* "special" SID */
> +		goto ret;
> +	}
> +
> +	if (sid_check_is_in_our_sam(&sid)) {
> +		/* our domain - the lookup is always
> +		 * correct for ID mapping.
> +		 */
> +		goto ret;
> +	}
> +
> +	sid_split_rid(&sid, NULL);
> +	sid_append_rid(&sid, DOMAIN_RID_USERS);
> +
> +	for (idx = 0; idx < names->count; ++idx) {
> +		if (!dom_sid_equal(&sid, &sids[idx])) {
> +			continue;
> +		}
> +
> +		/* has the DC been able to resolve
> +		 * "domain users" for same domain?
> +		 */
> +		n = &names->names[idx];
> +		if (lsa_SidType_to_id_type(n->sid_type) ==
> +		    ID_TYPE_NOT_SPECIFIED) {
> +			/* default id-mapping backend */
> +			return "";
> +		}
> +
> +		goto ret;
> +	}

doesn't this add O(n^2) overhead for finding the domain SID? When mapping a
token with a large number of groups this may be noticable.

Maybe add_sid_to_array_unique() should be add_sid_to_array_front_unique().

Cheerio!
-slow