[Freeipa-devel] [PATCH] 0096 support Windows Server 2012
Alexander Bokovoy
abokovoy at redhat.com
Wed Dec 5 21:01:20 UTC 2012
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.
>
>> + /* 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.
New patch attached.
It filters out statically compiled in list of well-known SID prefixes
and SIDs belonging to our own domain.
I'll add fetching the white list from the LDAP in next version.
--
/ Alexander Bokovoy
-------------- next part --------------
>From fd3b4f6747c59a0f540b3bba63b3aedaf6dca68b Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Thu, 22 Nov 2012 17:45:40 +0200
Subject: [PATCH] ipa-kdb: Support Windows 2012 Server
Windows 2012 Server changed procedure how KERB_VALIDATION_INFO ([MS-PAC] section 2.5)
is populated. Detailed description is available in [MS-KILE] version 25.0 and above.
Refactor KERB_VALIDATION_INFO verification and ensure we filter out extra SIDs in
case they belong to our domain.
https://fedorahosted.org/freeipa/ticket/3231
---
daemons/ipa-kdb/ipa_kdb_mspac.c | 268 +++++++++++++++++++++++++++++++++++++---
1 file changed, 253 insertions(+), 15 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index ed2c7fb8c8c4975ce942335f4688d32f7c84f937..ee1c6124f8d04cb10d091f11883834620c5c35ea 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -30,11 +30,15 @@ struct ipadb_adtrusts {
char *domain_name;
char *flat_name;
char *domain_sid;
+ struct dom_sid domsid;
};
struct ipadb_mspac {
char *flat_domain_name;
char *flat_server_name;
+ struct dom_sid domsid;
+ struct dom_sid *well_known_sids;
+
char *fallback_group;
uint32_t fallback_rid;
@@ -84,6 +88,36 @@ static char *memberof_pac_attrs[] = {
NULL
};
+static char *mspac_well_known_sids[] = {
+ "S-1-0",
+ "S-1-1",
+ "S-1-2",
+ "S-1-3",
+ "S-1-5-1",
+ "S-1-5-2",
+ "S-1-5-3",
+ "S-1-5-4",
+ "S-1-5-5",
+ "S-1-5-6",
+ "S-1-5-7",
+ "S-1-5-8",
+ "S-1-5-9",
+ "S-1-5-10",
+ "S-1-5-11",
+ "S-1-5-12",
+ "S-1-5-13",
+ "S-1-5-14",
+ "S-1-5-15",
+ "S-1-5-16",
+ "S-1-5-17",
+ "S-1-5-18",
+ "S-1-5-19",
+ "S-1-5-20",
+};
+
+#define LEN_WELL_KNOWN_SIDS (sizeof(mspac_well_known_sids)/sizeof(char*))
+
+
#define SID_ID_AUTHS 6
#define SID_SUB_AUTHS 15
#define MAX(a,b) (((a)>(b))?(a):(b))
@@ -213,6 +247,104 @@ static struct dom_sid *dom_sid_dup(TALLOC_CTX *memctx,
return new_sid;
}
+/* checks if sid1 is a domain of sid2 or compares them exactly if exact_check is true
+ * returns
+ * true -- if sid1 is a domain of sid2 (including full exact match)
+ * false -- otherwise
+ *
+ * dom_sid_check() is supposed to be used with sid1 representing domain SID
+ * and sid2 being either domain or resource SID in the domain
+ */
+static bool dom_sid_check(const struct dom_sid *sid1, const struct dom_sid *sid2, bool exact_check)
+{
+ int c, num;
+
+ if (sid1 == sid2) {
+ return true;
+ }
+
+ if (sid1 == NULL) {
+ return false;
+ }
+
+ if (sid2 == NULL) {
+ return false;
+ }
+
+ /* If SIDs have different revisions, they are different */
+ if (sid1->sid_rev_num != sid2->sid_rev_num)
+ return false;
+
+ /* When number of authorities is different, sids are different
+ * if we were asked to check prefix exactly */
+ num = sid2->num_auths - sid1->num_auths;
+ if (num != 0) {
+ if (exact_check) {
+ return false;
+ } else {
+ /* otherwise we are dealing with prefix check
+ * and sid2 should have RID compared to the sid1 */
+ if (num != 1) {
+ return false;
+ }
+ }
+ }
+
+ /* now either sid1->num_auths == sid2->num_auths or sid1 has no RID */
+
+ /* for same size authorities compare them backwards
+ * since RIDs are likely different */
+ for (c = sid1->num_auths; c >= 0; --c)
+ if (sid1->sub_auths[c] != sid2->sub_auths[c])
+ return false;
+
+ /* Finally, compare Identifier authorities */
+ for (c = 0; c < SID_ID_AUTHS; c++)
+ if (sid1->id_auth[c] != sid2->id_auth[c])
+ return false;
+
+ return true;
+}
+
+static bool dom_sid_is_prefix(const struct dom_sid *sid1, const struct dom_sid *sid2)
+{
+ int c;
+
+ if (sid1 == sid2) {
+ return true;
+ }
+
+ if (sid1 == NULL) {
+ return false;
+ }
+
+ if (sid2 == NULL) {
+ return false;
+ }
+
+ /* If SIDs have different revisions, they are different */
+ if (sid1->sid_rev_num != sid2->sid_rev_num)
+ return false;
+
+ if (sid1->num_auths > sid2->num_auths)
+ return false;
+
+ /* now sid1->num_auths <= sid2->num_auths */
+
+ /* compare up to sid1->num_auth authorities since RIDs are
+ * likely different and we are searching for the prefix */
+ for (c = 0; c < sid1->num_auths; c++)
+ if (sid1->sub_auths[c] != sid2->sub_auths[c])
+ return false;
+
+ /* Finally, compare Identifier authorities */
+ for (c = 0; c < SID_ID_AUTHS; c++)
+ if (sid1->id_auth[c] != sid2->id_auth[c])
+ return false;
+
+ return true;
+}
+
static int sid_append_rid(struct dom_sid *sid, uint32_t rid)
{
if (sid->num_auths >= SID_SUB_AUTHS) {
@@ -1059,6 +1191,22 @@ static struct ipadb_adtrusts *get_domain_from_realm_update(krb5_context context,
return domain;
}
+static void filter_logon_info_log_message(struct dom_sid *sid)
+{
+ char *domstr = NULL;
+
+ domstr = dom_sid_string(NULL, sid);
+ if (domstr) {
+ krb5_klog_syslog(LOG_ERR, "PAC filtering issue: SID [%s] is not allowed "
+ "from a trusted source and will be excluded.", domstr);
+ talloc_free(domstr);
+ } else {
+ krb5_klog_syslog(LOG_ERR, "PAC filtering issue: SID is not allowed "
+ "from a trusted source and will be excluded."
+ "Unable to allocate memory to display SID.");
+ }
+}
+
static krb5_error_code filter_logon_info(krb5_context context,
TALLOC_CTX *memctx,
krb5_data realm,
@@ -1070,8 +1218,11 @@ 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, k, count;
+ bool result;
+ char *domstr = NULL;
domain = get_domain_from_realm_update(context, realm);
if (!domain) {
@@ -1089,27 +1240,61 @@ 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;
- }
-
- if (strcmp(domsid, domain->domain_sid) != 0) {
+ /* check exact sid */
+ result = dom_sid_check(&domain->domsid, info->info->info3.base.domain_sid, true);
+ if (!result) {
+ domstr = dom_sid_string(NULL, info->info->info3.base.domain_sid);
+ if (!domstr) {
+ return EINVAL;
+ }
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);
+ domain->domain_name, domain->domain_sid, domstr);
+ talloc_free(domstr);
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 SID with our domain without taking RID into account */
+ result = dom_sid_check(&ipactx->mspac->domsid, info->info->info3.sids[i].sid, false);
+ if (result) {
+ filter_logon_info_log_message(info->info->info3.sids[i].sid);
+ } else {
+ for(k = 0; k < LEN_WELL_KNOWN_SIDS; k++) {
+ result = dom_sid_is_prefix(&ipactx->mspac->well_known_sids[k], info->info->info3.sids[i].sid);
+ if (result) {
+ filter_logon_info_log_message(info->info->info3.sids[i].sid);
+ break;
+ }
+ }
+ }
+ if (result) {
+ j++;
+ memmove(info->info->info3.sids+i, info->info->info3.sids+i+1, count-i-1);
+ }
+ i++;
+ } while (i < count);
+
+ if (j != 0) {
+ info->info->info3.sids = talloc_realloc(memctx, info->info->info3.sids, struct netr_SidAttr, count-j);
+ info->info->info3.sidcount = count-j;
+ if (!info->info->info3.sids) {
+ info->info->info3.sidcount = 0;
+ return ENOMEM;
+ }
+ }
}
/* According to MS-KILE, ResourceGroups must be zero, so check
@@ -1531,9 +1716,33 @@ void ipadb_mspac_struct_free(struct ipadb_mspac **mspac)
}
}
+ if ((*mspac)->well_known_sids) {
+ free((*mspac)->well_known_sids);
+ }
+
*mspac = NULL;
}
+#define LEN_WELL_KNOWN_SIDS (sizeof(mspac_well_known_sids)/sizeof(char*))
+krb5_error_code ipadb_mspac_fill_well_known_sids(struct ipadb_mspac *mspac)
+{
+ int i;
+
+ mspac->well_known_sids = calloc(LEN_WELL_KNOWN_SIDS, sizeof(struct dom_sid));
+
+ if (mspac->well_known_sids == NULL) {
+ return ENOMEM;
+ }
+
+ for (i = 0; i < LEN_WELL_KNOWN_SIDS; i++) {
+ if (mspac_well_known_sids[i] != NULL) {
+ (void) string_to_sid(mspac_well_known_sids[i], &(mspac->well_known_sids[i]));
+ }
+ }
+
+ return 0;
+}
+
krb5_error_code ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx)
{
struct ipadb_adtrusts *t;
@@ -1595,6 +1804,12 @@ krb5_error_code ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx)
ret = EINVAL;
goto done;
}
+
+ ret = string_to_sid(t[n].domain_sid, &t[n].domsid);
+ if (ret) {
+ ret = EINVAL;
+ goto done;
+ }
}
ret = 0;
@@ -1611,6 +1826,7 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
{
char *dom_attrs[] = { "ipaNTFlatName",
"ipaNTFallbackPrimaryGroup",
+ "ipaNTSecurityIdentifier",
NULL };
char *grp_attrs[] = { "ipaNTSecurityIdentifier", NULL };
krb5_error_code kerr;
@@ -1664,6 +1880,22 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
goto done;
}
+ ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry,
+ "ipaNTSecurityIdentifier",
+ &resstr);
+ if (ret) {
+ kerr = ret;
+ goto done;
+ }
+
+ ret = string_to_sid(resstr, &ipactx->mspac->domsid);
+ if (ret) {
+ kerr = ret;
+ free(resstr);
+ goto done;
+ }
+ free(resstr);
+
free(ipactx->mspac->flat_server_name);
ipactx->mspac->flat_server_name = get_server_netbios_name();
if (!ipactx->mspac->flat_server_name) {
@@ -1725,6 +1957,12 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx)
kerr = ipadb_mspac_get_trusted_domains(ipactx);
+ if (kerr) {
+ goto done;
+ }
+
+ kerr = ipadb_mspac_fill_well_known_sids(ipactx->mspac);
+
done:
ldap_msgfree(result);
return kerr;
--
1.8.0
More information about the Freeipa-devel
mailing list