From 6aeab13f06c4a9e63f4ddcb37d19f24de42aaff4 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Fri, 1 Jan 2021 17:34:54 -0600 Subject: [PATCH] hdb: Use a CHOICE instead of ASN1_BAD_ID Checking the error code of decoding a blob as an hdb_entry or hdb_entry_alias to determine which of those the blob is depends on a detail of the Heimdal ASN.1 compiler and library that shouldn't be depended on. Using a CHOICE adds no octets to the encoding: HDB-EntryOrAlias ::= CHOICE { entry hdb_entry, alias hdb_entry_alias } since we're adding no additional tags and the two arms of the CHOICE already differ in tag (hdb_entry's tag is a [UNIVERSAL Sequence] tag, while hdb_entry_alias's is an [APPLICATION 0] tag). --- lib/hdb/Makefile.am | 1 + lib/hdb/common.c | 170 ++++++++++++++++++++----------------- lib/hdb/hdb.asn1 | 5 ++ lib/hdb/libhdb-exports.def | 14 +++ lib/hdb/test_dbinfo.c | 61 +++++++++++++ lib/hdb/version-script.map | 14 +++ 6 files changed, 188 insertions(+), 77 deletions(-) diff --git a/lib/hdb/Makefile.am b/lib/hdb/Makefile.am index f1f22ebb1..9443532f5 100644 --- a/lib/hdb/Makefile.am +++ b/lib/hdb/Makefile.am @@ -35,6 +35,7 @@ gen_files_hdb = \ asn1_HDB_extension.x \ asn1_HDB_extensions.x \ asn1_HDB_EncTypeList.x \ + asn1_HDB_EntryOrAlias.x \ asn1_KeyRotation.x \ asn1_KeyRotationFlags.x \ asn1_hdb_entry.x \ diff --git a/lib/hdb/common.c b/lib/hdb/common.c index d65642948..cd3a88946 100644 --- a/lib/hdb/common.c +++ b/lib/hdb/common.c @@ -143,14 +143,22 @@ add_default_salts(krb5_context context, HDB *db, hdb_entry *entry) return ret; } -krb5_error_code -_hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal, - unsigned flags, krb5_kvno kvno, hdb_entry_ex *entry) +static krb5_error_code +fetch_entry_or_alias(krb5_context context, + HDB *db, + krb5_const_principal principal, + unsigned flags, + hdb_entry_ex *entry) { + HDB_EntryOrAlias eoa; krb5_principal enterprise_principal = NULL; krb5_data key, value; krb5_error_code ret; + value.length = 0; + value.data = 0; + key = value; + if (principal->name.name_type == KRB5_NT_ENTERPRISE_PRINCIPAL) { if (principal->name.name_string.len != 1) { ret = KRB5_PARSE_MALFORMED; @@ -166,41 +174,58 @@ _hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal, principal = enterprise_principal; } - hdb_principal2key(context, principal, &key); - if (enterprise_principal) - krb5_free_principal(context, enterprise_principal); - ret = db->hdb__get(context, db, key, &value); - krb5_data_free(&key); - if(ret) - return ret; - ret = hdb_value2entry(context, &value, &entry->entry); - /* HDB_F_GET_ANY indicates request originated from KDC (not kadmin) */ - if (ret == ASN1_BAD_ID && (flags & (HDB_F_CANON|HDB_F_GET_ANY)) == 0) { - krb5_data_free(&value); - return HDB_ERR_NOENTRY; - } else if (ret == ASN1_BAD_ID) { - hdb_entry_alias alias; - - ret = hdb_value2entry_alias(context, &value, &alias); - if (ret) { - krb5_data_free(&value); - return ret; - } - hdb_principal2key(context, alias.principal, &key); - krb5_data_free(&value); - free_hdb_entry_alias(&alias); - - ret = db->hdb__get(context, db, key, &value); - krb5_data_free(&key); - if (ret) - return ret; - ret = hdb_value2entry(context, &value, &entry->entry); - if (ret) { - krb5_data_free(&value); - return ret; - } + ret = hdb_principal2key(context, principal, &key); + if (ret == 0) + ret = db->hdb__get(context, db, key, &value); + if (ret == 0) + ret = decode_HDB_EntryOrAlias(value.data, value.length, &eoa, NULL); + if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_entry) { + entry->entry = eoa.u.entry; + } else if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias) { + krb5_data_free(&key); + ret = hdb_principal2key(context, eoa.u.alias.principal, &key); + if (ret == 0) + ret = db->hdb__get(context, db, key, &value); + if (ret == 0) + /* No alias chaining */ + ret = hdb_value2entry(context, &value, &entry->entry); + } else if (ret == 0) + ret = ENOTSUP; + if (ret == 0 && enterprise_principal) { + /* + * Whilst Windows does not canonicalize enterprise principal names if + * the canonicalize flag is unset, the original specification in + * draft-ietf-krb-wg-kerberos-referrals-03.txt says we should. + */ + entry->entry.flags.force_canonicalize = 1; } + + /* HDB_F_GET_ANY indicates request originated from KDC (not kadmin) */ + if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias && + (flags & (HDB_F_CANON|HDB_F_GET_ANY)) == 0) { + + /* `principal' was alias but canon not req'd */ + free_hdb_entry(&entry->entry); + ret = HDB_ERR_NOENTRY; + } + + krb5_free_principal(context, enterprise_principal); krb5_data_free(&value); + krb5_data_free(&key); + principal = enterprise_principal = NULL; + return ret; +} + +krb5_error_code +_hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal, + unsigned flags, krb5_kvno kvno, hdb_entry_ex *entry) +{ + krb5_error_code ret; + + ret = fetch_entry_or_alias(context, db, principal, flags, entry); + if (ret) + return ret; + if ((flags & HDB_F_DECRYPT) && (flags & HDB_F_ALL_KVNOS)) { /* Decrypt the current keys */ ret = hdb_unseal_keys(context, db, &entry->entry); @@ -249,14 +274,6 @@ _hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal, return ret; } } - if (enterprise_principal) { - /* - * Whilst Windows does not canonicalize enterprise principal names if - * the canonicalize flag is unset, the original specification in - * draft-ietf-krb-wg-kerberos-referrals-03.txt says we should. - */ - entry->entry.flags.force_canonicalize = 1; - } return 0; } @@ -336,48 +353,47 @@ hdb_add_aliases(krb5_context context, HDB *db, return 0; } +/* Check if new aliases are already used for other entries */ static krb5_error_code hdb_check_aliases(krb5_context context, HDB *db, hdb_entry_ex *entry) { - const HDB_Ext_Aliases *aliases; - int code; + const HDB_Ext_Aliases *aliases = NULL; + HDB_EntryOrAlias eoa; + krb5_data akey, value; size_t i; + int ret; - /* check if new aliases already is used */ + memset(&eoa, 0, sizeof(eoa)); + krb5_data_zero(&value); + akey = value; - code = hdb_entry_get_aliases(&entry->entry, &aliases); - if (code) - return code; + ret = hdb_entry_get_aliases(&entry->entry, &aliases); + for (i = 0; ret == 0 && aliases && i < aliases->aliases.len; i++) { + ret = hdb_principal2key(context, &aliases->aliases.val[i], &akey); + if (ret == 0) + ret = db->hdb__get(context, db, akey, &value); + if (ret == 0) + ret = decode_HDB_EntryOrAlias(value.data, value.length, &eoa, NULL); + if (ret == 0 && eoa.element != choice_HDB_EntryOrAlias_entry && + eoa.element != choice_HDB_EntryOrAlias_alias) + ret = ENOTSUP; + if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_entry) + /* New alias names an existing non-alias entry in the HDB */ + ret = HDB_ERR_EXISTS; + if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias && + !krb5_principal_compare(context, eoa.u.alias.principal, + entry->entry.principal)) + /* New alias names an existing alias of a different entry */ + ret = HDB_ERR_EXISTS; + if (ret == HDB_ERR_NOENTRY) /* from db->hdb__get */ + /* New alias is a name that doesn't exist in the HDB */ + ret = 0; - for (i = 0; aliases && i < aliases->aliases.len; i++) { - hdb_entry_alias alias; - krb5_data akey, value; - - code = hdb_principal2key(context, &aliases->aliases.val[i], &akey); - if (code == 0) { - code = db->hdb__get(context, db, akey, &value); - krb5_data_free(&akey); - } - if (code == HDB_ERR_NOENTRY) - continue; - else if (code) - return code; - - code = hdb_value2entry_alias(context, &value, &alias); + free_HDB_EntryOrAlias(&eoa); krb5_data_free(&value); - - if (code == ASN1_BAD_ID) - return HDB_ERR_EXISTS; - else if (code) - return code; - - code = krb5_principal_compare(context, alias.principal, - entry->entry.principal); - free_hdb_entry_alias(&alias); - if (code == 0) - return HDB_ERR_EXISTS; + krb5_data_free(&akey); } - return 0; + return ret; } /* diff --git a/lib/hdb/hdb.asn1 b/lib/hdb/hdb.asn1 index 0fec2e2c6..c6a8cbe4b 100644 --- a/lib/hdb/hdb.asn1 +++ b/lib/hdb/hdb.asn1 @@ -240,4 +240,9 @@ hdb_entry_alias ::= [APPLICATION 0] SEQUENCE { principal[0] Principal OPTIONAL } +HDB-EntryOrAlias ::= CHOICE { + entry hdb_entry, + alias hdb_entry_alias +} + END diff --git a/lib/hdb/libhdb-exports.def b/lib/hdb/libhdb-exports.def index 52737fe99..8645a2631 100644 --- a/lib/hdb/libhdb-exports.def +++ b/lib/hdb/libhdb-exports.def @@ -107,12 +107,18 @@ EXPORTS asn1_HDBFlags_units copy_Event copy_HDB_EncTypeList + copy_hdb_entry + copy_hdb_entry_alias + copy_HDB_EntryOrAlias copy_HDB_extensions copy_HDB_Ext_KeyRotation copy_Key copy_Keys copy_Salt decode_HDB_EncTypeList + decode_hdb_entry + decode_hdb_entry_alias + decode_HDB_EntryOrAlias decode_HDB_Ext_Aliases decode_HDB_extension decode_HDB_Ext_KeyRotation @@ -120,6 +126,9 @@ EXPORTS decode_Key decode_Keys encode_HDB_EncTypeList + encode_hdb_entry + encode_hdb_entry_alias + encode_HDB_EntryOrAlias encode_HDB_Ext_Aliases encode_HDB_extension encode_HDB_Ext_KeyRotation @@ -129,6 +138,8 @@ EXPORTS free_Event free_HDB_EncTypeList free_hdb_entry + free_hdb_entry_alias + free_HDB_EntryOrAlias free_HDB_Ext_Aliases free_HDB_extension free_HDB_extensions @@ -144,6 +155,9 @@ EXPORTS int2KeyRotationFlags KeyRotationFlags2int length_HDB_EncTypeList + length_hdb_entry + length_hdb_entry_alias + length_HDB_EntryOrAlias length_HDB_Ext_Aliases length_HDB_extension length_HDB_Ext_KeyRotation diff --git a/lib/hdb/test_dbinfo.c b/lib/hdb/test_dbinfo.c index b94b75bb3..7119d762e 100644 --- a/lib/hdb/test_dbinfo.c +++ b/lib/hdb/test_dbinfo.c @@ -44,6 +44,65 @@ struct getargs args[] = { static int num_args = sizeof(args) / sizeof(args[0]); +/* + * Prove that HDB_EntryOrAlias being a CHOICE of hdb_entry or hdb_entry_alias + * adds nothing to the encoding of those types. + */ +static +void +check_HDB_EntryOrAlias(krb5_context context) +{ + HDB_EntryOrAlias eoa; + hdb_entry entry; + hdb_entry_alias alias; + krb5_data v; + size_t len; + int ret; + + memset(&entry, 0, sizeof(entry)); + memset(&alias, 0, sizeof(alias)); + memset(&eoa, 0, sizeof(eoa)); + krb5_data_zero(&v); + + ret = krb5_make_principal(context, &alias.principal, "KTH.SE", "foo", + NULL); + if (ret) + krb5_err(context, 1, ret, "krb5_make_principal"); + ASN1_MALLOC_ENCODE(hdb_entry_alias, v.data, v.length, &alias, &len, ret); + if (ret) + krb5_err(context, 1, ret, "encode_HDB_EntryOrAlias"); + if (v.length != len) + abort(); + ret = decode_HDB_EntryOrAlias(v.data, v.length, &eoa, &len); + if (ret) + krb5_err(context, 1, ret, "decode_HDB_EntryOrAlias"); + if (v.length != len) + abort(); + free_HDB_EntryOrAlias(&eoa); + free_hdb_entry_alias(&alias); + krb5_data_free(&v); + + ret = krb5_make_principal(context, &entry.principal, "KTH.SE", "foo", + NULL); + if (ret) + krb5_err(context, 1, ret, "krb5_make_principal"); + entry.kvno = 5; + entry.flags.initial = 1; + ASN1_MALLOC_ENCODE(hdb_entry, v.data, v.length, &entry, &len, ret); + if (ret) + krb5_err(context, 1, ret, "encode_HDB_EntryOrAlias"); + if (v.length != len) + abort(); + ret = decode_HDB_EntryOrAlias(v.data, v.length, &eoa, &len); + if (ret) + krb5_err(context, 1, ret, "decode_HDB_EntryOrAlias"); + if (v.length != len) + abort(); + free_HDB_EntryOrAlias(&eoa); + free_hdb_entry(&entry); + krb5_data_free(&v); +} + int main(int argc, char **argv) { @@ -68,6 +127,8 @@ main(int argc, char **argv) if (ret) errx (1, "krb5_init_context failed: %d", ret); + check_HDB_EntryOrAlias(context); + ret = hdb_get_dbinfo(context, &info); if (ret) krb5_err(context, 1, ret, "hdb_get_dbinfo"); diff --git a/lib/hdb/version-script.map b/lib/hdb/version-script.map index 87c082b66..2de270bfe 100644 --- a/lib/hdb/version-script.map +++ b/lib/hdb/version-script.map @@ -111,12 +111,18 @@ HEIMDAL_HDB_1.0 { asn1_HDBFlags_units; copy_Event; copy_HDB_EncTypeList; + copy_hdb_entry; + copy_hdb_entry_alias; + copy_HDB_EntryOrAlias; copy_HDB_extensions; copy_HDB_Ext_KeyRotation; copy_Key; copy_Keys; copy_Salt; decode_HDB_EncTypeList; + decode_hdb_entry; + decode_hdb_entry_alias; + decode_HDB_EntryOrAlias; decode_HDB_Ext_Aliases; decode_HDB_extension; decode_HDB_Ext_KeyRotation; @@ -124,6 +130,9 @@ HEIMDAL_HDB_1.0 { decode_Key; decode_Keys; encode_HDB_EncTypeList; + encode_hdb_entry; + encode_hdb_entry_alias; + encode_HDB_EntryOrAlias; encode_HDB_Ext_Aliases; encode_HDB_extension; encode_HDB_Ext_KeyRotation; @@ -134,6 +143,8 @@ HEIMDAL_HDB_1.0 { free_Event; free_HDB_EncTypeList; free_hdb_entry; + free_hdb_entry_alias; + free_HDB_EntryOrAlias; free_HDB_Ext_Aliases; free_HDB_extension; free_HDB_extensions; @@ -149,6 +160,9 @@ HEIMDAL_HDB_1.0 { int2KeyRotationFlags; KeyRotationFlags2int; length_HDB_EncTypeList; + length_hdb_entry; + length_hdb_entry_alias; + length_HDB_EntryOrAlias; length_HDB_Ext_Aliases; length_HDB_extension; length_HDB_Ext_KeyRotation;