From: Viktor Dukhovni Date: Wed, 17 Sep 2025 09:07:07 +0000 (+1000) Subject: Check for OBJ_create() conflicts after write lock. X-Git-Tag: openssl-3.6.0~20 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e70c3efd7c387cb32d8efc0b594e02d0dbca737b;p=thirdparty%2Fopenssl.git Check for OBJ_create() conflicts after write lock. For now subsequent calls to OBJ_create() with identical inputs return NID_undef. It may be better to return the previous NID in the future. The real work actually happens in OBJ_add_object(). Duplicate compares *all* the input object's fields with any of the objects found by lookup. If these are identical, then necessarily all the lookups found the same data, and we can return the existing nid in low-level calls via OBJ_add_object() that specify the nid also. If any of the fields are different the new object is not installed and NID_undef is returned. Reviewed-by: Neil Horman Reviewed-by: Bernd Edlinger Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28582) (cherry picked from commit 556ba81601a37530c3620016bdeaf13094b4fb91) --- diff --git a/crypto/objects/obj_dat.c b/crypto/objects/obj_dat.c index 6d97f200242..469e8b5784a 100644 --- a/crypto/objects/obj_dat.c +++ b/crypto/objects/obj_dat.c @@ -39,6 +39,7 @@ struct added_obj_st { static unsigned long added_obj_hash(const ADDED_OBJ *ca); static int added_obj_cmp(const ADDED_OBJ *ca, const ADDED_OBJ *cb); +static int add_object(const ASN1_OBJECT *obj, int indirect); static LHASH_OF(ADDED_OBJ) *added = NULL; static CRYPTO_RWLOCK *ossl_obj_lock = NULL; @@ -155,6 +156,19 @@ static unsigned long added_obj_hash(const ADDED_OBJ *ca) return ret; } +/* + * Compare two ASN1_OBJECTs, including SNAME and LNAME, but not NIDs. + */ +static int obj_equivalent(const ASN1_OBJECT *a, const ASN1_OBJECT *b) +{ + return a->length == b->length + && memcmp(a->data, b->data, (size_t)a->length) == 0 + && (a->sn == NULL) == (b->sn == NULL) + && strcmp(a->sn ? a->sn : "", b->sn ? b->sn : "") == 0 + && (a->ln == NULL) == (b->ln == NULL) + && strcmp(a->ln ? a->ln : "", b->ln ? b->ln : "") == 0; +} + static int added_obj_cmp(const ADDED_OBJ *ca, const ADDED_OBJ *cb) { ASN1_OBJECT *a, *b; @@ -722,15 +736,11 @@ int OBJ_create(const char *oid, const char *sn, const char *ln) goto err; } - tmpoid->nid = OBJ_new_nid(1); - - if (tmpoid->nid == NID_undef) - goto err; - + tmpoid->nid = NID_undef; tmpoid->sn = (char *)sn; tmpoid->ln = (char *)ln; - ok = OBJ_add_object(tmpoid); + ok = add_object(tmpoid, 1); tmpoid->sn = NULL; tmpoid->ln = NULL; @@ -754,14 +764,35 @@ const unsigned char *OBJ_get0_data(const ASN1_OBJECT *obj) return obj->data; } -int OBJ_add_object(const ASN1_OBJECT *obj) +static int add_object(const ASN1_OBJECT *obj, int indirect) { - ASN1_OBJECT *o = NULL; + ASN1_OBJECT *o = NULL, *dup = NULL; ADDED_OBJ *ao[4] = { NULL, NULL, NULL, NULL }, *aop[4]; - int i; + int i, ret = NID_undef, nid = obj->nid; + + /* + * Indirect calls leave the NID unspecified, in which case we generate a + * fresh NID here. Direct calls via `OBJ_add_object()` must explicity + * specify the nid, and we then also check against the compile-time bsearch + * lists that the indirect calls have checked while holding a read lock. + */ + if (indirect) { + if (nid != NID_undef + || (nid = OBJ_new_nid(1)) < NUM_NID + || (o = OBJ_dup(obj)) == NULL) + return ret; + o->nid = nid; + } else if (nid < NUM_NID + || (obj->data != NULL + && OBJ_bsearch_obj(&obj, obj_objs, NUM_OBJ) != NULL) + || (obj->sn != NULL + && OBJ_bsearch_sn(&obj, sn_objs, NUM_SN) != NULL) + || (obj->ln != NULL + && OBJ_bsearch_ln(&obj, ln_objs, NUM_LN) != NULL) + || (o = OBJ_dup(obj)) == NULL) { + return ret; + } - if ((o = OBJ_dup(obj)) == NULL) - return NID_undef; if ((ao[ADDED_NID] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL || (o->length != 0 && obj->data != NULL @@ -778,24 +809,41 @@ int OBJ_add_object(const ASN1_OBJECT *obj) } for (i = ADDED_DATA; i <= ADDED_NID; i++) { - if (ao[i] != NULL) { - ao[i]->type = i; - ao[i]->obj = o; - aop[i] = lh_ADDED_OBJ_retrieve(added, ao[i]); - if (aop[i] != NULL) - aop[i]->type = -1; - (void)lh_ADDED_OBJ_insert(added, ao[i]); - if (lh_ADDED_OBJ_error(added)) { - if (aop[i] != NULL) - aop[i]->type = i; - while (i-- > ADDED_DATA) { + if (ao[i] == NULL) + continue; + ao[i]->type = i; + ao[i]->obj = o; + if ((aop[i] = lh_ADDED_OBJ_retrieve(added, ao[i])) != NULL) + dup = aop[i]->obj; + } + + if (dup != NULL) { + /* + * We found a possible conflict. If the caller did not specify a NID, + * return NID_undef to signal the conflict. Otherwise, if the NID and + * parameters are unchanged, return the old NID, else NID_undef to + * signal the conflict. This ensures that object registrations are + * immutable. + * + * In the future, ideally also return an equivalent existing NID also + * when the caller did not specify a NID, as in OBJ_create(). + */ + if (obj->nid == dup->nid && obj_equivalent(obj, dup)) + ret = dup->nid; + goto err; + } + + for (i = ADDED_DATA; i <= ADDED_NID; i++) { + if (ao[i] == NULL) + continue; + (void)lh_ADDED_OBJ_insert(added, ao[i]); + if (lh_ADDED_OBJ_error(added)) { + while (i-- > ADDED_DATA) { + if (ao[i] != NULL) lh_ADDED_OBJ_delete(added, ao[i]); - if (aop[i] != NULL) - aop[i]->type = i; - } - ERR_raise(ERR_LIB_OBJ, ERR_R_CRYPTO_LIB); - goto err; } + ERR_raise(ERR_LIB_OBJ, ERR_R_CRYPTO_LIB); + goto err; } } o->flags &= @@ -811,7 +859,12 @@ int OBJ_add_object(const ASN1_OBJECT *obj) for (i = ADDED_DATA; i <= ADDED_NID; i++) OPENSSL_free(ao[i]); ASN1_OBJECT_free(o); - return NID_undef; + return ret; +} + +int OBJ_add_object(const ASN1_OBJECT *obj) +{ + return add_object(obj, 0); } int OBJ_obj2nid(const ASN1_OBJECT *a)