389904 - avoid multiple decoding/encoding while creating and using PKIX_PL_X500Name

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

PKIX_PL_X500Name is the structure that has only one member which is CERTName *.
The API functions that create PKIX_PL_X500Name, create it from s string(or utf8 string) of characters that actually is already a product CERTName to ASCII(UTF8) conversion. Later those functions convert the string back to CERTName while creating PKIX_PL_X500Name. At the same time the function that retrieves DER encoded CERTName from PKIX_PL_X500Name structure encodes CERTName using SEC_Asn1Encoder.

As the result der encoded distinguished name goes though 4 transformation before it get used as a search key when pkix looks for a particular cert. Those transformations are:
    decoding to CERTName ->
    conversion to utf8 char* ->
    conversion from utf8 to CERTName ->
    encoding to DER.

One of the way to fix the problem is to add SECItem, that will contain DER encoded data of this CERTName, as member to PKIX_PL_X500Name  structure. Extend API to supply CERTName der data during creation. Deprecate the old constructors of PKIX_PL_X500Name and allow only tests to use them.

Comment 1

[Alexei Volkov]

Attachment: Fix for the problem as it was proposed

Patch when applied brings down number of failures from 116 to 51.
LibPKIX tests all passed.

Still investigating where the library has the rest of the problems.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Question about libPKIX.  This patch eliminates a function named
pkix_pl_X500Name_CompareDERBytes, which took two X500 name structures
(Very much like NSS CERTName structs) and did a full binary comparison
on all the attribute value pairs in the name.  

That code surprised me.  Doing a full binary comparison is what the old NSS
code does, and we know it fails NIST's name comparision tests.  I thought
that libPKIX was supposed to replace that with a proper name comparison 
algorithm that does things like:  (from RFC 3280 page 21)
 - ignore case in Printabl eStrings
 - ignore all leading and trailing spaces in printable strings
 - collapse multiple spaces down to a single space in printable strings
 - compare all AttributeTypeAndValues in a RelativeDistinguishedName SET 
   without regard to their order.
 - (MAYBE) compare strings of different character encodings.

Clearly pkix_pl_X500Name_CompareDERBytes did not do that, and 
SECItem_CompareItem on the equivalent DER encodings is a more efficient 
replacement for the existing behavior of pkix_pl_X500Name_CompareDERBytes.

But it seems to me that the right thing to do is not to further entrench
the binary comparison scheme, and is instead to embrace the RFC 3280 subset
of the X.509 name comparison rules (which I summarized above).  

Does libPKIX have a function that can correctly compare two DN, 
RelativeDistinguishedName by RelativeDistinguishedName, handling the 
different string types correctly?  If so, IMO, we should use it.  

NSS already has a (severly underused) function to do that.  It uses NSS's 
CERTName type rather than libPKIX's PKIX_PL_X500Name type, but I think 
the two types are equivalent, if not identical (except in name).

    SECComparison CERT_CompareName(CERTName *a, CERTName *b)
             /security/nss/lib/certdb/secname.c, line 573 

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Actually, CERT_CompareName has (at least) one flaw: It does not ignore the
order of the ATAVs in the RDN when comparing ATAVs.  See Bug 372242

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Review comments:

1. Don't write any new uses of the PKIX_PL_NSSCALLRV macro. 
code the statement, not the macro.

2. name changes:
   pkix_pl_X500Name_GetSECName -> pkix_pl_X500Name_GetDERName
   pSECName  -> pDERName

3: use one call to SECITEM_DupItem instead of two calls to 
   PORT_ArenaZAlloc and SECITEM_CopyItem

4. Add comment explaining WHY we're adding a SECItem with the DER Name
   to pkix_pl_X500Name.  Put comment in pkix_pl_x500name.h

5. in PKIX_PL_X500NameStruct, have actual CERTName and SECItem structs,
   not pointers to those things.  

6. Either 
a) PKIX_PL_X500NameStruct must have a pointer to its arena, or 
b) Each object allocated by PKIX_PL_Object_Alloc must have its own arena.
If (B) is too big a change to do now, then put the arena pointer into 
PKIX_PL_X500NameStruct.

7.  Functions that are only used when BUILD_LIBPKIX_TEST is defined should
all be inside #ifdef BUILD_LIBPKIX_TEST.  This includes (for example):
PKIX_PL_X500Name_Create 

8 in PKIX_PL_X500Name_Match() fix the PKIX_ENTER function name string.
If the SECITEM_CompareItem says not a match, then compare the CERTNames
with CERT_CompareName, and make the final result based on that.

9. change pkix_pl_X500Name_Equals to call PKIX_PL_X500Name_Match instead of
SECITEM_CompareItem or pkix_pl_X500Name_CompareDERBytes.

This may seem like a large list, but I think the amount of work remaining
to be done is pretty small.  This is encouraging, because this part of 
libPKIX needs less work than I thought.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 274243 [details] [diff] [review]
Fix for the problem as it was proposed

r- 
Please see previous comment for review feedback.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

One more review comment: 

10: Instead of 
PKIX_ERRORENTRY(BADVALUEOFARGUMENT,Bad value of argument),
I suggest something like:
PKIX_ERRORENTRY(X500NAMEHASNODERNAME,X500Name has no DER name),

Comment 7

[Alexei Volkov]

Attachment: includes review changes

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275562 [details] [diff] [review]
includes review changes

All my remaining review comments concern the file
nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c

With all those nasty PKIX_PL_NSSCALLRV macros eliminated,
I was able to see something about this code that I could not 
have noticed in its previous form.  

The functions named pkix_pl_X500Name_Get<something> are very inconsistent 
about the allocation of the values they return.  

One function, pkix_pl_X500Name_GetCERTName, returns the address of the 
X500Name object's own private copy of the CERTName, so that the caller 
shares the copy from the X500Name object.  The caller must be careful
NOT to destroy it while the X500Name object exists, and must be careful 
not to USE it after the X500Name object is destroyed.  This seems bad.

One function, pkix_pl_X500Name_GetDERName, takes an arenapool pointer 
argument, and returns a copy of its own value, allocated from that arenapool
(or from the heap if the arenapool pointer is NULL).  This seems good.

Other functions return a string that was newly allocated from the heap, 
that the caller must remember to destroy.  They include
pkix_pl_X500Name_GetCommonName
pkix_pl_X500Name_GetCountryName
pkix_pl_X500Name_GetOrgName

The comments for these functions say nothing about these differences.
They say nothing about who owns the returned memory and whose resposibility
it is to destroy that memory.  

I think these problems need to be addressed, perhaps with more comments in 
the code, or perhaps with a functional change (e.g. for GetCertName).
But I observe that all these problems were there before this patch was 
written, and this patch makes them no better and no worse.  It merely 
makes them more obvious (which is a GOOD thing).  So I will give this 
patch an r+, and perhaps we should file a separate bug about the memory
management issues described above.


>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c,v
>retrieving revision 1.2
>diff -U 10 -p -r1.2 pkix_pl_x500name.c
>--- nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c	9 Dec 2006 00:27:37 -0000	1.2
>+++ nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c	7 Aug 2007 05:17:34 -0000


> /*
>  * FUNCTION: pkix_pl_X500Name_RegisterSelf
>  * DESCRIPTION:
>  *  Registers PKIX_X500NAME_TYPE and its related functions with systemClasses[]
>@@ -425,134 +278,314 @@ pkix_pl_X500Name_RegisterSelf(void *plCo
>         entry.hashcodeFunction = pkix_pl_X500Name_Hashcode;
>         entry.toStringFunction = pkix_pl_X500Name_ToString;
>         entry.comparator = NULL;
>         entry.duplicateFunction = pkix_duplicateImmutable;
> 
>         systemClasses[PKIX_X500NAME_TYPE] = entry;
> 
>         PKIX_RETURN(X500NAME);
> }
> 
>+#ifdef BUILD_LIBPKIX_TESTS
>+/*
>+ * FUNCTION: pkix_pl_X500Name_CreateFromUtf8
>+ *
>+ * DESCRIPTION:
>+ *  Creates an X500Name object from the RFC1485 string representation pointed
>+ *  to by "stringRep", and stores the result at "pName". If the string cannot
>+ *  be successfully converted, a non-fatal error is returned.
>+ *
>+ * NOTE: ifdefed BUILD_LIBPKIX_TESTS function: this function is allowed to be
>+ * called only by pkix tests programs.
>+ * 
>+ * PARAMETERS:
>+ *  "stringRep"
>+ *      Address of the RFC1485 string to be converted. Must be non-NULL.
>+ *  "pName"
>+ *      Address where the X500Name result will be stored. Must be non-NULL.
>+ *  "plContext"
>+ *      Platform-specific context pointer.
>+ *
>+ * THREAD SAFETY:
>+ *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
>+ *
>+ * RETURNS:
>+ *  Returns NULL if the function succeeds.
>+ *  Returns an X500NAME Error if the function fails in a non-fatal way.
>+ *  Returns a Fatal Error if the function fails in an unrecoverable way.
>+ */
>+PKIX_Error *
>+pkix_pl_X500Name_CreateFromUtf8(
>+        char *stringRep,
>+        PKIX_PL_X500Name **pName,
>+        void *plContext)
>+{
>+        PKIX_PL_X500Name *x500Name = NULL;
>+        PRArenaPool *arena = NULL;
>+        CERTName *nssDN = NULL;
>+        SECItem *resultSecItem = NULL;
>+        
>+        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_CreateFromUtf8");
>+        PKIX_NULLCHECK_TWO(pName, stringRep);
>+
>+        nssDN = CERT_AsciiToName(stringRep);
>+        if (nssDN == NULL) {
>+            PKIX_ERROR(PKIX_COULDNOTCREATENSSDN);
>+        }
>+
>+        arena = nssDN->arena;
>+
>+        /* create a PKIX_PL_X500Name object */
>+        PKIX_CHECK(PKIX_PL_Object_Alloc
>+                    (PKIX_X500NAME_TYPE,
>+                    sizeof (PKIX_PL_X500Name),
>+                    (PKIX_PL_Object **)&x500Name,
>+                    plContext),
>+                    PKIX_COULDNOTCREATEX500NAMEOBJECT);
>+
>+        /* populate the nssDN field */
>+        x500Name->arena = arena;
>+        x500Name->nssDN.arena = arena;
>+        x500Name->nssDN.rdns = nssDN->rdns;
>+        
>+        resultSecItem =
>+            SEC_ASN1EncodeItem(arena, &x500Name->derName, nssDN,
>+                               CERT_NameTemplate);
>+        
>+        if (resultSecItem == NULL){
>+            PKIX_ERROR(PKIX_SECASN1ENCODEITEMFAILED);
>+        }
>+
>+        *pName = x500Name;
>+
>+cleanup:
>+
>+        if (PKIX_ERROR_RECEIVED){
>+            if (x500Name) {
>+                PKIX_PL_Object_DecRef((PKIX_PL_Object*)x500Name,
>+                                      plContext);
>+            } else if (nssDN) {
>+                CERT_DestroyName(nssDN);
>+            }
>+        }
>+
>+        PKIX_RETURN(X500NAME);
>+}
>+#endif /* BUILD_LIBPKIX_TESTS */
>+
>+/*
>+ * FUNCTION: pkix_pl_X500Name_GetCERTName
>+ *
>+ * DESCRIPTION:
>+ * 
>+ * Returns the pointer to CERTName member of X500Name structure.
>+ *
>+ * Returned pointed should not be freed.2
>+ *
>+ * PARAMETERS:
>+ *  "xname"
>+ *      Address of X500Name whose OrganizationName is to be extracted. Must be
>+ *      non-NULL.
>+ *  "pCERTName"
>+ *      Address where result will be stored. Must be non-NULL.
>+ *  "plContext"
>+ *      Platform-specific context pointer.
>+ *
>+ * THREAD SAFETY:
>+ *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
>+ *
>+ * RETURNS:
>+ *  Returns NULL if the function succeeds.
>+ *  Returns a Fatal Error if the function fails in an unrecoverable way.
>+ */
>+PKIX_Error *
>+pkix_pl_X500Name_GetCERTName(
>+        PKIX_PL_X500Name *xname,
>+        CERTName **pCERTName,
>+        void *plContext)
>+{
>+        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetCERTName");
>+        PKIX_NULLCHECK_TWO(xname, pCERTName);
>+
>+        *pCERTName = &xname->nssDN;
>+
>+        PKIX_RETURN(X500NAME);
>+}
>+
> /* --Public-Functions------------------------------------------------------- */
> 
> /*
>+ * FUNCTION: PKIX_PL_X500Name_CreateFromCERTName (see comments in pkix_pl_pki.h)
>+ */
>+
>+PKIX_Error *
>+PKIX_PL_X500Name_CreateFromCERTName(
>+        SECItem *derName,
>+        CERTName *name, 
>+        PKIX_PL_X500Name **pName,
>+        void *plContext)
>+{
>+        PRArenaPool *arena = NULL;
>+        SECStatus rv = SECFailure;
>+        PKIX_PL_X500Name *x500Name = NULL;
>+
>+        PKIX_ENTER(X500NAME, "PKIX_PL_X500Name_CreateFromCERTName");
>+        PKIX_NULLCHECK_ONE(pName);
>+        if (derName == NULL && name == NULL) {
>+            PKIX_ERROR(PKIX_NULLARGUMENT);
>+        }
>+
>+        arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>+        if (arena == NULL) {
>+            PKIX_ERROR(PKIX_PORTARENAALLOCFAILED);
>+        }
>+        
>+        PKIX_CHECK(PKIX_PL_Object_Alloc
>+                    (PKIX_X500NAME_TYPE,
>+                    sizeof (PKIX_PL_X500Name),
>+                    (PKIX_PL_Object **)&x500Name,
>+                    plContext),
>+                    PKIX_COULDNOTCREATEX500NAMEOBJECT);
>+
>+        x500Name->arena = arena;
>+        x500Name->nssDN.arena = NULL;
>+
>+        if (derName != NULL) {
>+            rv = SECITEM_CopyItem(arena, &x500Name->derName, derName);
>+            if (rv == SECFailure) {
>+                PKIX_ERROR(PKIX_SECITEMCOPYITEMFAILED);
>+            }
>+        }            
>+
>+        if (name != NULL) {
>+            rv = CERT_CopyName(arena, &x500Name->nssDN, name);
>+            if (rv == SECFailure) {
>+                PKIX_ERROR(PKIX_CERTCOPYNAMEFAILED);
>+            }
>+        } else {
>+            rv = SEC_QuickDERDecodeItem(arena, &x500Name->nssDN,
>+                                        CERT_NameTemplate,
>+                                        &x500Name->derName);
>+            if (rv == SECFailure) {
>+                PKIX_ERROR(PKIX_SECQUICKDERDECODERFAILED);
>+            }
>+        }
>+
>+        *pName = x500Name;
>+
>+cleanup:
>+        if (PKIX_ERROR_RECEIVED) {
>+            if (x500Name) {
>+                PKIX_PL_Object_DecRef((PKIX_PL_Object*)x500Name,
>+                                      plContext);
>+            } else if (arena) {                
>+                PORT_FreeArena(arena, PR_FALSE);
>+            }
>+        }
>+
>+        PKIX_RETURN(X500NAME);
>+}
>+
>+#ifdef BUILD_LIBPKIX_TESTS
>+/*
>  * FUNCTION: PKIX_PL_X500Name_Create (see comments in pkix_pl_pki.h)
>+ *
>+ * NOTE: ifdefed BUILD_LIBPKIX_TESTS function: this function is allowed
>+ * to be called only by pkix tests programs.
>  */
> PKIX_Error *
> PKIX_PL_X500Name_Create(
>         PKIX_PL_String *stringRep,
>         PKIX_PL_X500Name **pName,
>         void *plContext)
> {
>-        PKIX_PL_X500Name *x500Name = NULL;
>-        CERTName *nssDN = NULL;
>         char *utf8String = NULL;
>-        PKIX_UInt32 utf8Length;
>+        PKIX_UInt32 utf8Length = 0;
> 
>-        PKIX_ENTER(X500NAME, "PKIX_PL_X500Name_Create()");
>+        PKIX_ENTER(X500NAME, "PKIX_PL_X500Name_Create");
>         PKIX_NULLCHECK_TWO(pName, stringRep);
> 
>         /*
>          * convert the input PKIX_PL_String to PKIX_UTF8_NULL_TERM.
>          * we need to use this format specifier because
>          * CERT_AsciiToName expects a NULL-terminated UTF8 string.
>          * Since UTF8 allow NUL characters in the middle of the
>          * string, this is buggy. However, as a workaround, using
>          * PKIX_UTF8_NULL_TERM gives us a NULL-terminated UTF8 string.
>          */
> 
>         PKIX_CHECK(PKIX_PL_String_GetEncoded
>                     (stringRep,
>                     PKIX_UTF8_NULL_TERM,
>                     (void **)&utf8String,
>                     &utf8Length,
>                     plContext),
>                     PKIX_STRINGGETENCODEDFAILED);
> 
>-        PKIX_X500NAME_DEBUG("\t\tCalling CERT_AsciiToName).\n");
>-        /* this should be really be called CERT_UTF8ToName */
>-        nssDN = CERT_AsciiToName(utf8String);
>-        if (nssDN == NULL) {
>-                PKIX_ERROR(PKIX_COULDNOTCREATENSSDN);
>-        }
>-
>-        /* create a PKIX_PL_X500Name object */
>-        PKIX_CHECK(PKIX_PL_Object_Alloc
>-                    (PKIX_X500NAME_TYPE,
>-                    sizeof (PKIX_PL_X500Name),
>-                    (PKIX_PL_Object **)&x500Name,
>-                    plContext),
>-                    PKIX_COULDNOTCREATEX500NAMEOBJECT);
>-
>-        /* populate the nssDN field */
>-        x500Name->nssDN = nssDN;
>-
>-        *pName = x500Name;
>+        PKIX_CHECK(
>+            pkix_pl_X500Name_CreateFromUtf8(utf8String,
>+                                            pName, plContext),
>+            PKIX_X500NAMECREATEFROMUTF8FAILED);
> 
> cleanup:
>-
>         PKIX_FREE(utf8String);
> 
>-        if (nssDN && PKIX_ERROR_RECEIVED){
>-                PKIX_X500NAME_DEBUG("\t\tCalling CERT_DestroyName).\n");
>-                CERT_DestroyName(nssDN);
>-                nssDN = NULL;
>-        }
>-
>         PKIX_RETURN(X500NAME);
> }
>+#endif /* BUILD_LIBPKIX_TESTS */
> 
> /*
>  * FUNCTION: PKIX_PL_X500Name_Match (see comments in pkix_pl_pki.h)
>  */
> PKIX_Error *
> PKIX_PL_X500Name_Match(
>         PKIX_PL_X500Name *firstX500Name,
>         PKIX_PL_X500Name *secondX500Name,
>         PKIX_Boolean *pResult,
>         void *plContext)
> {
>-        CERTName *firstName = NULL;
>-        CERTName *secondName = NULL;
>+        SECItem *firstDerName = NULL;
>+        SECItem *secondDerName = NULL;
>         SECComparison cmpResult;
> 
>-        PKIX_ENTER(X500NAME, "PKIX_PL_X500Name_Equals");
>+        PKIX_ENTER(X500NAME, "PKIX_PL_X500Name_Match");
>         PKIX_NULLCHECK_THREE(firstX500Name, secondX500Name, pResult);
> 
>         if (firstX500Name == secondX500Name){
>                 *pResult = PKIX_TRUE;
>                 goto cleanup;
>         }
> 
>-        firstName = firstX500Name->nssDN;
>-        secondName = secondX500Name->nssDN;
>+        firstDerName = &firstX500Name->derName;
>+        secondDerName = &secondX500Name->derName;
> 
>-        PKIX_NULLCHECK_TWO(firstName, secondName);
>+        PKIX_NULLCHECK_TWO(firstDerName, secondDerName);
> 
>-        PKIX_X500NAME_DEBUG("\t\tCalling CERT_CompareName).\n");
>-        cmpResult = CERT_CompareName(firstName, secondName);
>+        cmpResult = SECITEM_CompareItem(firstDerName, secondDerName);
>+        if (cmpResult != SECEqual) {
>+            cmpResult = CERT_CompareName(&firstX500Name->nssDN,
>+                                         &secondX500Name->nssDN);
>+        }
> 
>         *pResult = (cmpResult == SECEqual);
>-
>+                   
> cleanup:
> 
>         PKIX_RETURN(X500NAME);
> }
> 
> /*
>  * FUNCTION: pkix_pl_X500Name_GetSECName
>  *
>  * DESCRIPTION:
>- *  Encodes as a SECItem the CERTName embodied by the X500Name object pointed
>- *  to by "xname", using the arena pointed to by "arena", and stores the result
>- *  at "pSECName". If the name cannot be successfully encoded, NULL is stored
>- *  at "pSECName".
>+ *  Returns a copy of CERTName DER representation allocated on passed in arena.
>+ *  If allocation on arena can not be done, NULL is stored at "pSECName".
>  *
>  * PARAMETERS:
>  *  "xname"
>  *      Address of X500Name whose CERTName flag is to be encoded. Must be
>  *      non-NULL.
>  *  "arena"
>  *      Address of the PRArenaPool to be used in the encoding, and in which
>  *      "pSECName" will be allocated. Must be non-NULL.
>  *  "pSECName"
>  *      Address where result will be stored. Must be non-NULL.
>@@ -560,104 +593,46 @@ cleanup:
>  *      Platform-specific context pointer.
>  *
>  * THREAD SAFETY:
>  *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
>  *
>  * RETURNS:
>  *  Returns NULL if the function succeeds.
>  *  Returns a Fatal Error if the function fails in an unrecoverable way.
>  */
> PKIX_Error *
>-pkix_pl_X500Name_GetSECName(
>+pkix_pl_X500Name_GetDERName(
>         PKIX_PL_X500Name *xname,
>         PRArenaPool *arena,
>-        SECItem **pSECName,
>+        SECItem **pDERName,
>         void *plContext)
> {
>+        SECItem *derName = NULL;
> 
>-        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetCertName");
>+        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetDERName");
> 
>-        PKIX_NULLCHECK_THREE(xname, arena, pSECName);
>+        PKIX_NULLCHECK_THREE(xname, arena, pDERName);
> 
>-        /*
>-         * SEC_ASN1EncodeItem returns NULL if unsuccessful. We just
>-         * store the NULL result.
>-         */
>-        PKIX_PL_NSSCALLRV(X500NAME, *pSECName, SEC_ASN1EncodeItem,
>-                (arena, NULL, (void *)xname->nssDN, CERT_NameTemplate));
>-
>-        PKIX_RETURN(X500NAME);
>-}
>-
>-/*
>- * FUNCTION: pkix_pl_X500Name_CreateFromUtf8
>- *
>- * DESCRIPTION:
>- *  Creates an X500Name object from the RFC1485 string representation pointed
>- *  to by "stringRep", and stores the result at "pName". If the string cannot
>- *  be successfully converted, a non-fatal error is returned.
>- *
>- * PARAMETERS:
>- *  "stringRep"
>- *      Address of the RFC1485 string to be converted. Must be non-NULL.
>- *  "pName"
>- *      Address where the X500Name result will be stored. Must be non-NULL.
>- *  "plContext"
>- *      Platform-specific context pointer.
>- *
>- * THREAD SAFETY:
>- *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
>- *
>- * RETURNS:
>- *  Returns NULL if the function succeeds.
>- *  Returns an X500NAME Error if the function fails in a non-fatal way.
>- *  Returns a Fatal Error if the function fails in an unrecoverable way.
>- */
>-PKIX_Error *
>-pkix_pl_X500Name_CreateFromUtf8(
>-        char *stringRep,
>-        PKIX_PL_X500Name **pName,
>-        void *plContext)
>-{
>-        PKIX_PL_X500Name *x500Name = NULL;
>-        CERTName *nssDN = NULL;
>-
>-        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_CreateFromUtf8");
>-        PKIX_NULLCHECK_TWO(pName, stringRep);
>-
>-        PKIX_PL_NSSCALLRV(X500NAME, nssDN, CERT_AsciiToName, (stringRep));
>-
>-        if (nssDN == NULL) {
>-                PKIX_ERROR(PKIX_COULDNOTCREATENSSDN);
>+        /* Return NULL is X500Name was not created from DER  */
>+        if (xname->derName.data == NULL) {
>+            *pDERName = NULL;
>+            goto cleanup;
>         }
> 
>-        /* create a PKIX_PL_X500Name object */
>-        PKIX_CHECK(PKIX_PL_Object_Alloc
>-                    (PKIX_X500NAME_TYPE,
>-                    sizeof (PKIX_PL_X500Name),
>-                    (PKIX_PL_Object **)&x500Name,
>-                    plContext),
>-                    PKIX_COULDNOTCREATEX500NAMEOBJECT);
>-
>-        /* populate the nssDN field */
>-        x500Name->nssDN = nssDN;
>-
>-        *pName = x500Name;
>+        derName = SECITEM_ArenaDupItem(arena, &xname->derName);
>+        if (derName == NULL) {
>+            PKIX_ERROR(PKIX_SECITEMCOPYITEMFAILED);
>+        }
> 
>+        *pDERName = derName;
> cleanup:
> 
>-        if (nssDN && PKIX_ERROR_RECEIVED){
>-                PKIX_X500NAME_DEBUG("\t\tCalling CERT_DestroyName).\n");
>-                CERT_DestroyName(nssDN);
>-                nssDN = NULL;
>-        }
>-
>         PKIX_RETURN(X500NAME);
> }
> 
> /*
>  * FUNCTION: pkix_pl_X500Name_GetCommonName
>  *
>  * DESCRIPTION:
>  *  Extracts the CommonName component of the X500Name object pointed to by
>  *  "xname", and stores the result at "pCommonName". If the CommonName cannot
>  *  be successfully extracted, NULL is stored at "pCommonName".
>@@ -682,25 +657,21 @@ cleanup:
>  */
> PKIX_Error *
> pkix_pl_X500Name_GetCommonName(
>         PKIX_PL_X500Name *xname,
>         unsigned char **pCommonName,
>         void *plContext)
> {
>         PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetCommonName");
>         PKIX_NULLCHECK_TWO(xname, pCommonName);
> 
>-        PKIX_PL_NSSCALLRV
>-                (X500NAME,
>-                *pCommonName,
>-                (unsigned char *)CERT_GetCommonName,
>-                (xname->nssDN));
>+        *pCommonName = (unsigned char *)CERT_GetCommonName(&xname->nssDN);
> 
>         PKIX_RETURN(X500NAME);
> }
> 
> /*
>  * FUNCTION: pkix_pl_X500Name_GetCountryName
>  *
>  * DESCRIPTION:
>  *  Extracts the CountryName component of the X500Name object pointed to by
>  *  "xname", and stores the result at "pCountryName". If the CountryName cannot
>@@ -726,25 +697,21 @@ pkix_pl_X500Name_GetCommonName(
>  */
> PKIX_Error *
> pkix_pl_X500Name_GetCountryName(
>         PKIX_PL_X500Name *xname,
>         unsigned char **pCountryName,
>         void *plContext)
> {
>         PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetCountryName");
>         PKIX_NULLCHECK_TWO(xname, pCountryName);
> 
>-        PKIX_PL_NSSCALLRV
>-                (X500NAME,
>-                *pCountryName,
>-                (unsigned char *)CERT_GetCountryName,
>-                (xname->nssDN));
>+        *pCountryName = (unsigned char*)CERT_GetCountryName(&xname->nssDN);
> 
>         PKIX_RETURN(X500NAME);
> }
> 
> /*
>  * FUNCTION: pkix_pl_X500Name_GetOrgName
>  *
>  * DESCRIPTION:
>  *  Extracts the OrganizationName component of the X500Name object pointed to by
>  *  "xname", and stores the result at "pOrgName". If the OrganizationName cannot
>@@ -770,31 +737,14 @@ pkix_pl_X500Name_GetCountryName(
>  */
> PKIX_Error *
> pkix_pl_X500Name_GetOrgName(
>         PKIX_PL_X500Name *xname,
>         unsigned char **pOrgName,
>         void *plContext)
> {
>         PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetOrgName");
>         PKIX_NULLCHECK_TWO(xname, pOrgName);
> 
>-        PKIX_PL_NSSCALLRV
>-                (X500NAME,
>-                *pOrgName,
>-                (unsigned char *)CERT_GetOrgName,
>-                (xname->nssDN));
>-
>-        PKIX_RETURN(X500NAME);
>-}
>-PKIX_Error *
>-pkix_pl_X500Name_GetCERTName(
>-        PKIX_PL_X500Name *xname,
>-        CERTName **pCERTName,
>-        void *plContext)
>-{
>-        PKIX_ENTER(X500NAME, "pkix_pl_X500Name_GetCERTName");
>-        PKIX_NULLCHECK_TWO(xname, pCERTName);
>-
>-        *pCERTName = xname->nssDN;
>+        *pOrgName = (unsigned char*)CERT_GetOrgName(&xname->nssDN);
> 
>         PKIX_RETURN(X500NAME);
> }

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Sorry about the large piece of the patch that appears in the previous comment.
I meant to delete it before submitting the review comment.  (Sigh.)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

One more review comment about that last patch.
In function PKIX_PL_X500Name_Match(), we see these lines:

>-        firstName = firstX500Name->nssDN;
>-        secondName = secondX500Name->nssDN;
>+        firstDerName = &firstX500Name->derName;
>+        secondDerName = &secondX500Name->derName;
> 
>-        PKIX_NULLCHECK_TWO(firstName, secondName);
>+        PKIX_NULLCHECK_TWO(firstDerName, secondDerName);

Clearly, firstDerName and secondDerName cannot be NULL there.
I think you probably wanted to test firstDerName->data and 
secondDerName->data.

Comment 11

[Alexei Volkov]

/cvsroot/mozilla/security/coreconf/config.mk,v  <--  config.mk
new revision: 1.20; previous revision: 1.19
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.4; previous revision: 1.3
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_pl_pki.h,v  <--  pkix_pl_pki.h
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c,v  <--  pkix_pl_pk11certstore.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.5; previous revision: 1.4
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c,v  <--  pkix_pl_crl.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_generalname.c,v  <--  pkix_pl_generalname.c
new revision: 1.4; previous revision: 1.3
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c,v  <--  pkix_pl_x500name.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.h,v  <--  pkix_pl_x500name.h
new revision: 1.3; previous revision: 1.2