[Freeipa-devel] [PATCH] 0096 support Windows Server 2012

[Simo Sorce]

On Wed, 2012-12-05 at 14:16 +0200, Alexander Bokovoy wrote:
[..]
> Attached is a prototype to implement logic above. I haven't added
> filtering for anything but our own domain SIDs yet, want to get review
> for this part before going further.

Comments inline.
>  
> +static int dom_sid_cmp(const struct dom_sid *sid1, const struct
> dom_sid *sid2, bool compare_rids)
> +{
> +    int c, num;
> +
> +    if (sid1 == sid2) {
> +        return 0;
> +    }
> +
> +    if (sid1 == NULL) {
> +        return -1;
> +    }
> +
> +    if (sid2 == NULL) {
> +        return 1;
> +    }
> +
> +    /* If SIDs have different revisions, they are different */
> +    if (sid1->sid_rev_num != sid2->sid_rev_num)
> +        return sid1->sid_rev_num - sid2->sid_rev_num;
> +
> +    /* When number of authorities is different, sids are different */
> +    if (sid1->num_auths != sid2->num_auths)
> +        return sid1->num_auths - sid2->num_auths;
> +
> +    /* Optionally skip RIDs if asked */
> +    num = sid1->num_auths - 1;
> +    if (!compare_rids) {
> +        num--;
> +        if (num < 0) return sid1->sub_auths[0] - sid2->sub_auths[0];
> +    }

I a not sure this works if you pass in a domain SID and an actual user
SID, because they are of different lengths. A Domain SID is just like a
USER SID but misses the last authority which represents the RID.

Ie:

Domain SID: S-1-5-21-12345-6789-101123
User SID:   S-1-5-21-12345-6789-101123-501

I think the above function will make comparisons between domain SID and
User SID (which is the only comparison we care about) never succeed.

> +    /* for same size authorities compare them backwards
> +     * since RIDs are likely different */
> +    for (c = num; c >= 0; --c)
> +        if (sid1->sub_auths[c] != sid2->sub_auths[c])
> +            return sid1->sub_auths[c] - sid2->sub_auths[c];
> +
> +    /* Finally, compare Identifier authorities */
> +    for (c = 0; c < SID_ID_AUTHS; c++)
> +        if (sid1->id_auth[c] != sid2->id_auth[c])
> +            return sid1->id_auth[c] - sid2->id_auth[c];

I am wondering, wouldn't it be more efficient if we did a simple
memcmp() here ?
After all these are arrays and should be fully packed.

Also by testing backwards returning the classic -1, 0, +1 makes little
sense because you do not know if a higher authority was 'bigger' or
'smaller' but you found a difference already in a following one.

I would just return true or false from this function, either they match
or they don't. By returning -1,0,1 you mislead the reader in believing
this function might be used in a sorting algorithm, when it cannot as
is.

> +    return 0;
> +}
> +
>  static int sid_append_rid(struct dom_sid *sid, uint32_t rid)
>  {
>      if (sid->num_auths >= SID_SUB_AUTHS) {
> @@ -1070,8 +1118,9 @@ static krb5_error_code
> filter_logon_info(krb5_context context,
>       * attempt at getting us to sign fake credentials with the help
> of a
>       * compromised trusted realm */
>  
> +    struct ipadb_context *ipactx;
>      struct ipadb_adtrusts *domain;
> -    char *domsid;
> +    int i, j, result, count;
>  
>      domain = get_domain_from_realm_update(context, realm);
>      if (!domain) {
> @@ -1089,27 +1138,48 @@ static krb5_error_code
> filter_logon_info(krb5_context context,
>          return EINVAL;
>      }
>  
> -    /* check sid */
> -    domsid = dom_sid_string(NULL, info->info->info3.base.domain_sid);
> -    if (!domsid) {
> -        return EINVAL;
> -    }

I think you can keep the above for reporting debugging purposes later so
you do not have to change the log message.

> -    if (strcmp(domsid, domain->domain_sid) != 0) {
> +    /* check exact sid */
> +    result = dom_sid_cmp(&domain->domsid,
> info->info->info3.base.domain_sid, true);
> +    if (result != 0) {
>          krb5_klog_syslog(LOG_ERR, "PAC Info mismatch: domain = %s, "
> -                                  "expected domain SID = %s, "
> -                                  "found domain SID = %s",
> -                                  domain->domain_name,
> domain->domain_sid,
> -                                  domsid);
> -        talloc_free(domsid);
> +                                  "expected domain SID = %s, ",
> +                                  domain->domain_name,
> domain->domain_sid);
>          return EINVAL;
>      }
> -    talloc_free(domsid);
>  
> -    /* According to MS-KILE, info->info->info3.sids must be zero, so
> check
> -     * that it is the case here */
> +    /* According to MS-KILE 25.0, info->info->info3.sids may be non
> zero, so check
> +     * should include different possibilities into account
> +     * */
>      if (info->info->info3.sidcount != 0) {
> -        return EINVAL;
> +        ipactx = ipadb_get_context(context);
> +        if (!ipactx && !ipactx->mspac) {
> +            return KRB5_KDB_DBNOTINITED;
> +        }
> +        count = info->info->info3.sidcount;
> +        i = 0;
> +        j = 0;
> +        do {
> +            /* Compare SIDs without taking RID into account */
> +            result = dom_sid_cmp(&ipactx->mspac->domsid,
> info->info->info3.sids[i].sid, false);
> +            if (result == 0) {
> +                krb5_klog_syslog(LOG_ERR, "PAC Info mismatch: domain
> = %s, "
> +                     "extra sid should be not from the domain %s but
> received RID %d ",
> +                     domain->domain_name,
> ipactx->mspac->flat_domain_name,
> +
> info->info->info3.sids[i].sid->sub_auths[info->info->info3.sids[i].sid->num_auths-1]);
> +                j++;
> +                memmove(info->info->info3.sids+i,
> info->info->info3.sids+i+1, count-i-1);
> +            }

Sorry but doesn't 0 means it's a match ? Looks to me using true/false is
also less confusing.
Also the log message would use a bit of rework, I suggest: "PAC
Filtering issue: sid [%s] is not allowed from a trusted source and will
be excluded." Before this you do a sid to string, this is ok even if
slow as this is an important error condition and should not be common.

> 
The rest and the approach looks otherwise good to me.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York