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
>From fd3b4f6747c59a0f540b3bba63b3aedaf6dca68b Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
