From 895c2f84a6a083fc8b9f69f962ed19da12ce3b40 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Sun, 31 Jan 2016 21:14:51 -0500 Subject: [PATCH] Long overdue cleanup of X509 policy tree verification Replace all magic numbers with #defined constants except in boolean functions that return 0 for failure and 1 for success. Avoid a couple memory leaks in error recovery code paths. Code style improvements. Reviewed-by: Dr. Stephen Henson --- crypto/x509/x509_vfy.c | 10 +- crypto/x509v3/pcy_node.c | 3 +- crypto/x509v3/pcy_tree.c | 458 +++++++++++++++++-------------------- include/openssl/x509_vfy.h | 28 ++- 4 files changed, 244 insertions(+), 255 deletions(-) diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 1f3b2b9dab..3438692e57 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -1505,12 +1505,12 @@ static int check_policy(X509_STORE_CTX *ctx) return 1; ret = X509_policy_check(&ctx->tree, &ctx->explicit_policy, ctx->chain, ctx->param->policies, ctx->param->flags); - if (ret == 0) { + if (ret == X509_PCY_TREE_INTERNAL) { X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE); return 0; } /* Invalid or inconsistent extensions */ - if (ret == -1) { + if (ret == X509_PCY_TREE_INVALID) { /* * Locate certificates with bad extensions and notify callback. */ @@ -1527,11 +1527,15 @@ static int check_policy(X509_STORE_CTX *ctx) } return 1; } - if (ret == -2) { + if (ret == X509_PCY_TREE_FAILURE) { ctx->current_cert = NULL; ctx->error = X509_V_ERR_NO_EXPLICIT_POLICY; return ctx->verify_cb(0, ctx); } + if (ret != X509_PCY_TREE_VALID) { + X509err(X509_F_CHECK_POLICY, ERR_R_INTERNAL_ERROR); + return 0; + } if (ctx->param->flags & X509_V_FLAG_NOTIFY_POLICY) { ctx->current_cert = NULL; diff --git a/crypto/x509v3/pcy_node.c b/crypto/x509v3/pcy_node.c index e8007c23f9..581c246b74 100644 --- a/crypto/x509v3/pcy_node.c +++ b/crypto/x509v3/pcy_node.c @@ -151,8 +151,7 @@ X509_POLICY_NODE *level_add_node(X509_POLICY_LEVEL *level, node_error: policy_node_free(node); - return 0; - + return NULL; } void policy_node_free(X509_POLICY_NODE *node) diff --git a/crypto/x509v3/pcy_tree.c b/crypto/x509v3/pcy_tree.c index cac2d51dc3..b3995d64e5 100644 --- a/crypto/x509v3/pcy_tree.c +++ b/crypto/x509v3/pcy_tree.c @@ -97,24 +97,26 @@ static void expected_print(BIO *err, X509_POLICY_LEVEL *lev, static void tree_print(char *str, X509_POLICY_TREE *tree, X509_POLICY_LEVEL *curr) { + BIO *err = BIO_new_fp(stderr, BIO_NOCLOSE); X509_POLICY_LEVEL *plev; - X509_POLICY_NODE *node; - int i; - BIO *err; - err = BIO_new_fp(stderr, BIO_NOCLOSE); + if (err == NULL) return; if (!curr) curr = tree->levels + tree->nlevel; else curr++; + BIO_printf(err, "Level print after %s\n", str); BIO_printf(err, "Printing Up to Level %ld\n", curr - tree->levels); for (plev = tree->levels; plev != curr; plev++) { + int i; + BIO_printf(err, "Level %ld, flags = %x\n", - plev - tree->levels, plev->flags); + (long)(plev - tree->levels), plev->flags); for (i = 0; i < sk_X509_POLICY_NODE_num(plev->nodes); i++) { - node = sk_X509_POLICY_NODE_value(plev->nodes, i); + X509_POLICY_NODE *node = sk_X509_POLICY_NODE_value(plev->nodes, i); + X509_POLICY_NODE_print(err, node, 2); expected_print(err, plev, node, 2); BIO_printf(err, " Flags: %x\n", node->data->flags); @@ -122,26 +124,17 @@ static void tree_print(char *str, X509_POLICY_TREE *tree, if (plev->anyPolicy) X509_POLICY_NODE_print(err, plev->anyPolicy, 2); } - BIO_free(err); - } -#else - -# define tree_print(a,b,c) /* */ - #endif /*- - * Initialize policy tree. Return values: - * 0 Some internal error occurred. - * -1 Inconsistent or invalid extensions in certificates. - * 1 Tree initialized OK. - * 2 Policy tree is empty. - * 5 Tree OK and requireExplicitPolicy true. - * 6 Tree empty and requireExplicitPolicy true. + * Return value: <= 0 on error, or positive bit mask: + * + * X509_PCY_TREE_VALID: valid tree + * X509_PCY_TREE_EMPTY: empty tree (including bare TA case) + * X509_PCY_TREE_EXPLICIT: explicit policy required */ - static int tree_init(X509_POLICY_TREE **ptree, STACK_OF(X509) *certs, unsigned int flags) { @@ -149,103 +142,112 @@ static int tree_init(X509_POLICY_TREE **ptree, STACK_OF(X509) *certs, X509_POLICY_LEVEL *level; const X509_POLICY_CACHE *cache; X509_POLICY_DATA *data = NULL; - X509 *x; - int ret = 1; - int i, n; - int explicit_policy; - int any_skip; - int map_skip; + int ret = X509_PCY_TREE_VALID; + int n = sk_X509_num(certs) - 1; /* RFC5280 paths omit the TA */ + int explicit_policy = (flags & X509_V_FLAG_EXPLICIT_POLICY) ? 0 : n+1; + int any_skip = (flags & X509_V_FLAG_INHIBIT_ANY) ? 0 : n+1; + int map_skip = (flags & X509_V_FLAG_INHIBIT_MAP) ? 0 : n+1; + int i; *ptree = NULL; - n = sk_X509_num(certs); - if (flags & X509_V_FLAG_EXPLICIT_POLICY) - explicit_policy = 0; - else - explicit_policy = n + 1; + /* Can't do anything with just a trust anchor */ + if (n == 0) + return X509_PCY_TREE_EMPTY; - if (flags & X509_V_FLAG_INHIBIT_ANY) - any_skip = 0; - else - any_skip = n + 1; + /* + * First setup the policy cache in all n non-TA certificates, this will be + * used in X509_verify_cert() which will invoke the verify callback for all + * certificates with invalid policy extensions. + */ + for (i = n - 1; i >= 0; i--) { + X509 *x = sk_X509_value(certs, i); - if (flags & X509_V_FLAG_INHIBIT_MAP) - map_skip = 0; - else - map_skip = n + 1; + /* Call for side-effect of computing hash and caching extensions */ + X509_check_purpose(x, -1, 0); + + /* If cache is NULL, likely ENOMEM: return immediately */ + if ((cache = policy_cache_set(x)) == NULL) + return X509_PCY_TREE_INTERNAL; + } - /* Can't do anything with just a trust anchor */ - if (n == 1) - return 1; /* - * First setup policy cache in all certificates apart from the trust - * anchor. Note any bad cache results on the way. Also can calculate - * explicit_policy value at this point. + * At this point check for invalid policies and required explicit policy. + * Note that the explicit_policy counter is a count-down to zero, with the + * requirement kicking in if and once it does that. The counter is + * decremented for every non-self-issued certificate in the path, but may + * be further reduced by policy constraints in a non-leaf certificate. + * + * The ultimate policy set is the interesection of all the policies along + * the path, if we hit a certificate with an empty policy set, and explicit + * policy is required we're done. */ - for (i = n - 2; i >= 0; i--) { - uint32_t ex_flags; - x = sk_X509_value(certs, i); + for (i = n - 1; + i >= 0 && (explicit_policy > 0 || (ret & X509_PCY_TREE_EMPTY) == 0); + i--) { + X509 *x = sk_X509_value(certs, i); + uint32_t ex_flags = X509_get_extension_flags(x); - /* - * Note, this modifies x->ex_flags. If cache NULL something bad - * happened: return immediately - */ - cache = policy_cache_set(x); - if (cache == NULL) - return 0; - /* - * If inconsistent extensions keep a note of it but continue - */ - ex_flags = X509_get_extension_flags(x); + /* All the policies are already cached, we can return early */ if (ex_flags & EXFLAG_INVALID_POLICY) - ret = -1; - /* - * Otherwise if we have no data (hence no CertificatePolicies) and - * haven't already set an inconsistent code note it. - */ - else if ((ret == 1) && !cache->data) - ret = 2; + return X509_PCY_TREE_INVALID; + + /* Access the cache which we now know exists */ + cache = policy_cache_set(x); + + if ((ret & X509_PCY_TREE_VALID) && cache->data == NULL) + ret = X509_PCY_TREE_EMPTY; if (explicit_policy > 0) { if (!(ex_flags & EXFLAG_SI)) explicit_policy--; - if ((cache->explicit_skip != -1) + if ((cache->explicit_skip >= 0) && (cache->explicit_skip < explicit_policy)) explicit_policy = cache->explicit_skip; } } - if (ret != 1) { - if (ret == 2 && !explicit_policy) - return 6; + if (explicit_policy == 0) + ret |= X509_PCY_TREE_EXPLICIT; + if ((ret & X509_PCY_TREE_VALID) == 0) return ret; - } /* If we get this far initialize the tree */ - tree = OPENSSL_zalloc(sizeof(*tree)); - if (tree == NULL) - return 0; - tree->levels = OPENSSL_zalloc(sizeof(*tree->levels) * n); - if (tree->levels == NULL) { + if ((tree = OPENSSL_zalloc(sizeof(*tree))) == NULL) + return X509_PCY_TREE_INTERNAL; + + /* + * http://tools.ietf.org/html/rfc5280#section-6.1.2, figure 3. + * + * The top level is implicitly for the trust anchor with valid expected + * policies of anyPolicy. (RFC 5280 has the TA at depth 0 and the leaf at + * depth n, we have the leaf at depth 0 and the TA at depth n). + */ + if ((tree->levels = OPENSSL_zalloc(sizeof(*tree->levels)*(n+1))) == NULL) { OPENSSL_free(tree); - return 0; + return X509_PCY_TREE_INTERNAL; } - tree->nlevel = n; + tree->nlevel = n+1; level = tree->levels; - - /* Root data: initialize to anyPolicy */ - data = policy_data_new(NULL, OBJ_nid2obj(NID_any_policy), 0); - - if (data == NULL || !level_add_node(level, data, NULL, tree)) + if ((data = policy_data_new(NULL, OBJ_nid2obj(NID_any_policy), 0)) == NULL) goto bad_tree; + if (level_add_node(level, data, NULL, tree) == NULL) { + policy_data_free(data); + goto bad_tree; + } - for (i = n - 2; i >= 0; i--) { - uint32_t ex_flags; - level++; - x = sk_X509_value(certs, i); - ex_flags = X509_get_extension_flags(x); + /* + * In this pass initialize all the tree levels and whether anyPolicy and + * policy mapping are inhibited at each level. + */ + for (i = n - 1; i >= 0; i--) { + X509 *x = sk_X509_value(certs, i); + uint32_t ex_flags = X509_get_extension_flags(x); + + /* Access the cache which we now know exists */ cache = policy_cache_set(x); + X509_up_ref(x); - level->cert = x; + (++level)->cert = x; if (!cache->anyPolicy) level->flags |= X509_V_FLAG_INHIBIT_ANY; @@ -253,16 +255,15 @@ static int tree_init(X509_POLICY_TREE **ptree, STACK_OF(X509) *certs, /* Determine inhibit any and inhibit map flags */ if (any_skip == 0) { /* - * Any matching allowed if certificate is self issued and not the - * last in the chain. + * Any matching allowed only if certificate is self issued and not + * the last in the chain. */ if (!(ex_flags & EXFLAG_SI) || (i == 0)) level->flags |= X509_V_FLAG_INHIBIT_ANY; } else { if (!(ex_flags & EXFLAG_SI)) any_skip--; - if ((cache->any_skip >= 0) - && (cache->any_skip < any_skip)) + if ((cache->any_skip >= 0) && (cache->any_skip < any_skip)) any_skip = cache->any_skip; } @@ -271,45 +272,40 @@ static int tree_init(X509_POLICY_TREE **ptree, STACK_OF(X509) *certs, else { if (!(ex_flags & EXFLAG_SI)) map_skip--; - if ((cache->map_skip >= 0) - && (cache->map_skip < map_skip)) + if ((cache->map_skip >= 0) && (cache->map_skip < map_skip)) map_skip = cache->map_skip; } - } *ptree = tree; - - if (explicit_policy) - return 1; - else - return 5; + return ret; bad_tree: - X509_policy_tree_free(tree); - - return 0; - + return X509_PCY_TREE_INTERNAL; } +/* + * Return value: 1 on success, 0 otherwise + */ static int tree_link_matching_nodes(X509_POLICY_LEVEL *curr, X509_POLICY_DATA *data) { X509_POLICY_LEVEL *last = curr - 1; - X509_POLICY_NODE *node; int i, matched = 0; + /* Iterate through all in nodes linking matches */ for (i = 0; i < sk_X509_POLICY_NODE_num(last->nodes); i++) { - node = sk_X509_POLICY_NODE_value(last->nodes, i); + X509_POLICY_NODE *node = sk_X509_POLICY_NODE_value(last->nodes, i); + if (policy_node_match(last, node, data->valid_policy)) { - if (!level_add_node(curr, data, node, NULL)) + if (level_add_node(curr, data, node, NULL) == NULL) return 0; matched = 1; } } if (!matched && last->anyPolicy) { - if (!level_add_node(curr, data, last->anyPolicy, NULL)) + if (level_add_node(curr, data, last->anyPolicy, NULL) == NULL) return 0; } return 1; @@ -318,16 +314,17 @@ static int tree_link_matching_nodes(X509_POLICY_LEVEL *curr, /* * This corresponds to RFC3280 6.1.3(d)(1): link any data from * CertificatePolicies onto matching parent or anyPolicy if no match. + * + * Return value: 1 on success, 0 otherwise. */ - static int tree_link_nodes(X509_POLICY_LEVEL *curr, const X509_POLICY_CACHE *cache) { int i; - X509_POLICY_DATA *data; for (i = 0; i < sk_X509_POLICY_DATA_num(cache->data); i++) { - data = sk_X509_POLICY_DATA_value(cache->data, i); + X509_POLICY_DATA *data = sk_X509_POLICY_DATA_value(cache->data, i); + /* Look for matching nodes in previous level */ if (!tree_link_matching_nodes(curr, data)) return 0; @@ -338,35 +335,38 @@ static int tree_link_nodes(X509_POLICY_LEVEL *curr, /* * This corresponds to RFC3280 6.1.3(d)(2): Create new data for any unmatched * policies in the parent and link to anyPolicy. + * + * Return value: 1 on success, 0 otherwise. */ - static int tree_add_unmatched(X509_POLICY_LEVEL *curr, const X509_POLICY_CACHE *cache, const ASN1_OBJECT *id, X509_POLICY_NODE *node, X509_POLICY_TREE *tree) { X509_POLICY_DATA *data; + if (id == NULL) id = node->data->valid_policy; /* * Create a new node with qualifiers from anyPolicy and id from unmatched * node. */ - data = policy_data_new(NULL, id, node_critical(node)); - - if (data == NULL) + if ((data = policy_data_new(NULL, id, node_critical(node))) == NULL) return 0; + /* Curr may not have anyPolicy */ data->qualifier_set = cache->anyPolicy->qualifier_set; data->flags |= POLICY_DATA_FLAG_SHARED_QUALIFIERS; - if (!level_add_node(curr, data, node, tree)) { + if (level_add_node(curr, data, node, tree) == NULL) { policy_data_free(data); return 0; } - return 1; } +/* + * Return value: 1 on success, 0 otherwise. + */ static int tree_link_unmatched(X509_POLICY_LEVEL *curr, const X509_POLICY_CACHE *cache, X509_POLICY_NODE *node, X509_POLICY_TREE *tree) @@ -397,11 +397,12 @@ static int tree_link_unmatched(X509_POLICY_LEVEL *curr, } } - return 1; - } +/* + * Return value: 1 on success, 0 otherwise + */ static int tree_link_any(X509_POLICY_LEVEL *curr, const X509_POLICY_CACHE *cache, X509_POLICY_TREE *tree) @@ -417,19 +418,22 @@ static int tree_link_any(X509_POLICY_LEVEL *curr, return 0; } /* Finally add link to anyPolicy */ - if (last->anyPolicy) { - if (!level_add_node(curr, cache->anyPolicy, last->anyPolicy, NULL)) - return 0; - } + if (last->anyPolicy && + level_add_node(curr, cache->anyPolicy, last->anyPolicy, NULL) == NULL) + return 0; return 1; } -/* - * Prune the tree: delete any child mapped child data on the current level - * then proceed up the tree deleting any data with no children. If we ever - * have no data on a level we can halt because the tree will be empty. +/*- + * Prune the tree: delete any child mapped child data on the current level then + * proceed up the tree deleting any data with no children. If we ever have no + * data on a level we can halt because the tree will be empty. + * + * Return value: <= 0 error, otherwise one of: + * + * X509_PCY_TREE_VALID: valid tree + * X509_PCY_TREE_EMPTY: empty tree */ - static int tree_prune(X509_POLICY_TREE *tree, X509_POLICY_LEVEL *curr) { STACK_OF(X509_POLICY_NODE) *nodes; @@ -468,41 +472,43 @@ static int tree_prune(X509_POLICY_TREE *tree, X509_POLICY_LEVEL *curr) if (curr == tree->levels) { /* If we zapped anyPolicy at top then tree is empty */ if (!curr->anyPolicy) - return 2; - return 1; + return X509_PCY_TREE_EMPTY; + break; } } - - /* Unreachable */ - + return X509_PCY_TREE_VALID; } +/* + * Return value: 1 on success, 0 otherwise. + */ static int tree_add_auth_node(STACK_OF(X509_POLICY_NODE) **pnodes, X509_POLICY_NODE *pcy) { - if (*pnodes == NULL) { - *pnodes = policy_node_cmp_new(); - if (*pnodes == NULL) - return 0; - } else if (sk_X509_POLICY_NODE_find(*pnodes, pcy) != -1) - return 1; - - if (!sk_X509_POLICY_NODE_push(*pnodes, pcy)) + if (*pnodes == NULL && + (*pnodes = policy_node_cmp_new()) == NULL) return 0; - - return 1; - + if (sk_X509_POLICY_NODE_find(*pnodes, pcy) != -1) + return 1; + return sk_X509_POLICY_NODE_push(*pnodes, pcy) != 0; } -/* - * Calculate the authority set based on policy tree. The 'pnodes' parameter - * is used as a store for the set of policy nodes used to calculate the user - * set. If the authority set is not anyPolicy then pnodes will just point to - * the authority set. If however the authority set is anyPolicy then the set - * of valid policies (other than anyPolicy) is store in pnodes. The return - * value of '2' is used in this case to indicate that pnodes should be freed. - */ +#define TREE_CALC_FAILURE 0 +#define TREE_CALC_OK_NOFREE 1 +#define TREE_CALC_OK_DOFREE 2 +/*- + * Calculate the authority set based on policy tree. The 'pnodes' parameter is + * used as a store for the set of policy nodes used to calculate the user set. + * If the authority set is not anyPolicy then pnodes will just point to the + * authority set. If however the authority set is anyPolicy then the set of + * valid policies (other than anyPolicy) is store in pnodes. + * + * Return value: + * TREE_CALC_FAILURE on failure, + * TREE_CALC_OK_NOFREE on success and pnodes need not be freed, + * TREE_CALC_OK_DOFREE on success and pnodes needs to be freed + */ static int tree_calculate_authority_set(X509_POLICY_TREE *tree, STACK_OF(X509_POLICY_NODE) **pnodes) { @@ -515,7 +521,7 @@ static int tree_calculate_authority_set(X509_POLICY_TREE *tree, /* If last level contains anyPolicy set is anyPolicy */ if (curr->anyPolicy) { if (!tree_add_auth_node(&tree->auth_policies, curr->anyPolicy)) - return 0; + return TREE_CALC_FAILURE; addnodes = pnodes; } else /* Add policies to authority set */ @@ -533,19 +539,25 @@ static int tree_calculate_authority_set(X509_POLICY_TREE *tree, for (j = 0; j < sk_X509_POLICY_NODE_num(curr->nodes); j++) { node = sk_X509_POLICY_NODE_value(curr->nodes, j); if ((node->parent == anyptr) - && !tree_add_auth_node(addnodes, node)) - return 0; + && !tree_add_auth_node(addnodes, node)) { + if (addnodes == pnodes) { + sk_X509_POLICY_NODE_free(*pnodes); + *pnodes = NULL; + } + return TREE_CALC_FAILURE; + } } } - if (addnodes == pnodes) - return 2; + return TREE_CALC_OK_DOFREE; *pnodes = tree->auth_policies; - - return 1; + return TREE_CALC_OK_NOFREE; } +/* + * Return value: 1 on success, 0 otherwise. + */ static int tree_calculate_user_set(X509_POLICY_TREE *tree, STACK_OF(ASN1_OBJECT) *policy_oids, STACK_OF(X509_POLICY_NODE) *auth_nodes) @@ -553,7 +565,6 @@ static int tree_calculate_user_set(X509_POLICY_TREE *tree, int i; X509_POLICY_NODE *node; ASN1_OBJECT *oid; - X509_POLICY_NODE *anyPolicy; X509_POLICY_DATA *extra; @@ -561,7 +572,6 @@ static int tree_calculate_user_set(X509_POLICY_TREE *tree, * Check if anyPolicy present in authority constrained policy set: this * will happen if it is a leaf node. */ - if (sk_ASN1_OBJECT_num(policy_oids) <= 0) return 1; @@ -602,9 +612,14 @@ static int tree_calculate_user_set(X509_POLICY_TREE *tree, return 0; } return 1; - } +/*- + * Return value: <= 0 error, otherwise one of: + * X509_PCY_TREE_VALID: valid tree + * X509_PCY_TREE_EMPTY: empty tree + * (see tree_prune()). + */ static int tree_evaluate(X509_POLICY_TREE *tree) { int ret, i; @@ -614,19 +629,19 @@ static int tree_evaluate(X509_POLICY_TREE *tree) for (i = 1; i < tree->nlevel; i++, curr++) { cache = policy_cache_set(curr->cert); if (!tree_link_nodes(curr, cache)) - return 0; + return X509_PCY_TREE_INTERNAL; if (!(curr->flags & X509_V_FLAG_INHIBIT_ANY) && !tree_link_any(curr, cache, tree)) - return 0; + return X509_PCY_TREE_INTERNAL; +#ifdef OPENSSL_POLICY_DEBUG tree_print("before tree_prune()", tree, curr); +#endif ret = tree_prune(tree, curr); - if (ret != 1) + if (ret != X509_PCY_TREE_VALID) return ret; } - - return 1; - + return X509_PCY_TREE_VALID; } static void exnode_free(X509_POLICY_NODE *node) @@ -661,111 +676,70 @@ void X509_policy_tree_free(X509_POLICY_TREE *tree) /*- * Application policy checking function. * Return codes: - * 0 Internal Error. - * 1 Successful. - * -1 One or more certificates contain invalid or inconsistent extensions - * -2 User constrained policy set empty and requireExplicit true. + * X509_PCY_TREE_FAILURE: Failure to satisfy explicit policy + * X509_PCY_TREE_INVALID: Inconsistent or invalid extensions + * X509_PCY_TREE_INTERNAL: Internal error, most likely malloc + * X509_PCY_TREE_VALID: Success (null tree if empty or bare TA) */ - int X509_policy_check(X509_POLICY_TREE **ptree, int *pexplicit_policy, STACK_OF(X509) *certs, STACK_OF(ASN1_OBJECT) *policy_oids, unsigned int flags) { + int init_ret; int ret; X509_POLICY_TREE *tree = NULL; STACK_OF(X509_POLICY_NODE) *nodes, *auth_nodes = NULL; - *ptree = NULL; + *ptree = NULL; *pexplicit_policy = 0; - ret = tree_init(&tree, certs, flags); - - switch (ret) { - - /* Tree empty requireExplicit False: OK */ - case 2: - return 1; + init_ret = tree_init(&tree, certs, flags); - /* Some internal error */ - case -1: - return -1; + if (init_ret <= 0) + return init_ret; - /* Some internal error */ - case 0: - return 0; - - /* Tree empty requireExplicit True: Error */ - - case 6: - *pexplicit_policy = 1; - return -2; - - /* Tree OK requireExplicit True: OK and continue */ - case 5: + if ((init_ret & X509_PCY_TREE_EXPLICIT) == 0) { + if (init_ret & X509_PCY_TREE_EMPTY) { + X509_policy_tree_free(tree); + return X509_PCY_TREE_VALID; + } + } else { *pexplicit_policy = 1; - break; - - /* Tree OK: continue */ - - case 1: - if (!tree) - /* - * tree_init() returns success and a null tree - * if it's just looking at a trust anchor. - * I'm not sure that returning success here is - * correct, but I'm sure that reporting this - * as an internal error which our caller - * interprets as a malloc failure is wrong. - */ - return 1; - break; + /* Tree empty and requireExplicit True: Error */ + if (init_ret & X509_PCY_TREE_EMPTY) + return X509_PCY_TREE_FAILURE; } - if (!tree) - goto error; ret = tree_evaluate(tree); - +#ifdef OPENSSL_POLICY_DEBUG tree_print("tree_evaluate()", tree, NULL); - +#endif if (ret <= 0) goto error; - /* Return value 2 means tree empty */ - if (ret == 2) { + if (ret == X509_PCY_TREE_EMPTY) { X509_policy_tree_free(tree); - if (*pexplicit_policy) - return -2; - else - return 1; + if (init_ret & X509_PCY_TREE_EXPLICIT) + return X509_PCY_TREE_FAILURE; + return X509_PCY_TREE_VALID; } /* Tree is not empty: continue */ - - ret = tree_calculate_authority_set(tree, &auth_nodes); - - if (!ret) - goto error; - - if (!tree_calculate_user_set(tree, policy_oids, auth_nodes)) + if ((ret = tree_calculate_authority_set(tree, &auth_nodes)) == 0 || + !tree_calculate_user_set(tree, policy_oids, auth_nodes)) goto error; - - if (ret == 2) + if (ret == TREE_CALC_OK_DOFREE) sk_X509_POLICY_NODE_free(auth_nodes); - if (tree) - *ptree = tree; + *ptree = tree; - if (*pexplicit_policy) { + if (init_ret & X509_PCY_TREE_EXPLICIT) { nodes = X509_policy_tree_get0_user_policies(tree); if (sk_X509_POLICY_NODE_num(nodes) <= 0) - return -2; + return X509_PCY_TREE_FAILURE; } - - return 1; + return X509_PCY_TREE_VALID; error: - X509_policy_tree_free(tree); - - return 0; - + return X509_PCY_TREE_INTERNAL; } diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index ef54208a3c..4e458d20ff 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -55,17 +55,16 @@ * [including the GNU Public Licence.] */ -#ifndef HEADER_X509_H -# include -/* - * openssl/x509.h ends up #include-ing this file at about the only - * appropriate moment. - */ -#endif - #ifndef HEADER_X509_VFY_H # define HEADER_X509_VFY_H +/* + * Protect against recursion, x509.h and x509_vfy.h each include the other. + */ +# ifndef HEADER_X509_H +# include +# endif + # include # include # include @@ -583,6 +582,19 @@ const X509_VERIFY_PARAM *X509_VERIFY_PARAM_get0(int id); const X509_VERIFY_PARAM *X509_VERIFY_PARAM_lookup(const char *name); void X509_VERIFY_PARAM_table_cleanup(void); +/* Non positive return values are errors */ +#define X509_PCY_TREE_FAILURE -2 /* Failure to satisfy explicit policy */ +#define X509_PCY_TREE_INVALID -1 /* Inconsistent or invalid extensions */ +#define X509_PCY_TREE_INTERNAL 0 /* Internal error, most likely malloc */ + +/* + * Positive return values form a bit mask, all but the first are internal to + * the library and don't appear in results from X509_policy_check(). + */ +#define X509_PCY_TREE_VALID 1 /* The policy tree is valid */ +#define X509_PCY_TREE_EMPTY 2 /* The policy tree is empty */ +#define X509_PCY_TREE_EXPLICIT 4 /* Explicit policy required */ + int X509_policy_check(X509_POLICY_TREE **ptree, int *pexplicit_policy, STACK_OF(X509) *certs, STACK_OF(ASN1_OBJECT) *policy_oids, unsigned int flags); -- 2.25.1