| Subject: | PKINIT use of OpenSSL OID table is not thread-safe or application-friendly |
OpenSSL contains two global internal tables which map between object
identifiers and integer identifiers (called NIDs) for application
convenience. (These tables also map OIDs to "short names" and "long
names" but that is unimportant to this bug.) The first table is
immutable and maps frequently used OIDs to to API constants such as
NID_sha1. The second table is a mutable and can be populated by callers
using OBJ_create or OBJ_add_object. Query functions such as OBJ_nid2obj
and OBJ_sn2nid consult the mutable table first, then the immutable
table. OBJ_cleanup frees and resets the mutable table. As of this
writing, the mutable table does not appear to be protected by any mutex
calls, even if mutex callbacks are registered by the OpenSSL caller.
On initialization, PKINIT adds nine PKINIT-related OIDs to the mutable
table if they don't have pre-existing mappings. On cleanup, PKINIT
calls OBJ_cleanup. A global counter in the PKINIT code (not protected
by any mutex) tries to prevent PKINIT from removing its own OIDs from
the mutable table when another context has the PKINIT module loaded.
In addition, pkinit_pkcs7type2oid registers 1.2.840.113549.1.7.1, aka
id-data from RFC 3369. Comments indicate that that the goal is to
intentionally prevent OpenSSL from recognizing the CMS id-data OID.
There are three big problems here:
1. There are obvious thread-safety issues. PKINIT does not protect its
counter and OpenSSL does not protect its mutable table.
2. The OBJ_cleanup call will throw away any mutable table entries
created by other callers of OpenSSL.
3. The shadow entry for id-data could interfere with other callers of
OpenSSL. In fact, the registration is explicitly deferred to avoid
breaking "things like the use of pkcs12" within PKINIT itself.
To solve these issues, we should avoid using the mutable table. The
work for this breaks down as follows:
* Six of the OIDs are only used to compare against OIDs from received
messages or certificates. These can be created as anonymous OIDs with
OBJ_txt2oid and freed with ASN1_OBJECT_free.
* The other four OIDs (including the shadowed one) are converted to NID
and passed to PKCS7_set0_type_other, which converts the NID back to an
ASN1_OBJECT pointer and sets the PKCS7 handle's "type" field to that.
Converting to NID and back won't work with an anonymous OID. Since the
PKCS7 structure is public, we could avoid using PKCS7_set0_type_other
and just do it ourselves, using a duplicate of an anonymous OID for the
type.
* We have to stop shadowing the CMS id-data OID. More analysis is
required here. Unfortunately, the need to prevent OpenSSL from
recognizing id-data relates to draft9 compatibility, which is not
covered by automated tests.
identifiers and integer identifiers (called NIDs) for application
convenience. (These tables also map OIDs to "short names" and "long
names" but that is unimportant to this bug.) The first table is
immutable and maps frequently used OIDs to to API constants such as
NID_sha1. The second table is a mutable and can be populated by callers
using OBJ_create or OBJ_add_object. Query functions such as OBJ_nid2obj
and OBJ_sn2nid consult the mutable table first, then the immutable
table. OBJ_cleanup frees and resets the mutable table. As of this
writing, the mutable table does not appear to be protected by any mutex
calls, even if mutex callbacks are registered by the OpenSSL caller.
On initialization, PKINIT adds nine PKINIT-related OIDs to the mutable
table if they don't have pre-existing mappings. On cleanup, PKINIT
calls OBJ_cleanup. A global counter in the PKINIT code (not protected
by any mutex) tries to prevent PKINIT from removing its own OIDs from
the mutable table when another context has the PKINIT module loaded.
In addition, pkinit_pkcs7type2oid registers 1.2.840.113549.1.7.1, aka
id-data from RFC 3369. Comments indicate that that the goal is to
intentionally prevent OpenSSL from recognizing the CMS id-data OID.
There are three big problems here:
1. There are obvious thread-safety issues. PKINIT does not protect its
counter and OpenSSL does not protect its mutable table.
2. The OBJ_cleanup call will throw away any mutable table entries
created by other callers of OpenSSL.
3. The shadow entry for id-data could interfere with other callers of
OpenSSL. In fact, the registration is explicitly deferred to avoid
breaking "things like the use of pkcs12" within PKINIT itself.
To solve these issues, we should avoid using the mutable table. The
work for this breaks down as follows:
* Six of the OIDs are only used to compare against OIDs from received
messages or certificates. These can be created as anonymous OIDs with
OBJ_txt2oid and freed with ASN1_OBJECT_free.
* The other four OIDs (including the shadowed one) are converted to NID
and passed to PKCS7_set0_type_other, which converts the NID back to an
ASN1_OBJECT pointer and sets the PKCS7 handle's "type" field to that.
Converting to NID and back won't work with an anonymous OID. Since the
PKCS7 structure is public, we could avoid using PKCS7_set0_type_other
and just do it ourselves, using a duplicate of an anonymous OID for the
type.
* We have to stop shadowing the CMS id-data OID. More analysis is
required here. Unfortunately, the need to prevent OpenSSL from
recognizing id-data relates to draft9 compatibility, which is not
covered by automated tests.