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

[Alexander Bokovoy]

On Wed, 05 Dec 2012, Simo Sorce wrote:
>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.
This is why I have 'compare_rids' argument. The code in
filter_login_info actually uses it in places where we don't care about
RIDs -- in case of domain SID comparison we have to compare all sub
auths since the last one belongs to the domain.


>> +    /* 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.
Probably harmless here.


>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.
Be surprised, it is the way the SID sub auths comparison is done in Samba.

>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.
Yes. We have two basic needs here:
- test that SID starts with specific prefix (our domain), or
- test that SID is well-known SIDs/exact domain SID

I tried to pack two cases in the same comparison function but this
obviously not working well. So maybe I'll split them explicitly.


>> +    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.
I'll move it into the conditional so that we don't allocate memore
unless there is an error to report. This is though a bit fragile -- if
memory allocation in dom_sid_string() fails...

BTW, I'm yet to see any of these krb5_klog_syslog() messages. They never
appear in logs even if I call the function unconditionally.

>> -    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.
0 means it's a match, true. And it this case we compare extra SID to our
domain SID and have to remove extra SID then because we cannot allow our
own SIDs stuffed into MS-KILE by foreign KDC.

>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.
OK.

>
>>
>The rest and the approach looks otherwise good to me.
>
>Simo.
>
>-- 
>Simo Sorce * Red Hat, Inc * New York
>



-- 
/ Alexander Bokovoy