287563 - Please make cert_CompareNameWithConstraints a non-static function

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Hanfei Yu]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214

This function in lib/certdb/genname.c is useful for checking NameConstraints. It
can save duplicate codes in our component, please make it non-static.

Or even better, the function CERT_CompareNameSpace can also be separated into
two functions. One for getting NameConstraintsExten (name such as
CERT_FindNameConstraintsExten) and the other for doing the actual
checking/comparison.

Reproducible: Always

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Hanfei, You proposed splitting this function.  Would you care to contribute
a patch implementing that suggestion?

Comment 2

[Hanfei Yu]

Here are the two functions CERT_FindNameConstraintsExten() and
CERT_CheckNameSpace() that split CERT_CompareNameSpace(). I also include here
another function CERT_AddNameConstraintByGeneralName() that might be useful for
 creating/adding a CERTNameConstraint's permitted or excluded list from a 
CERTGeneralName.

Please also add their extern declarations in cert.h.  Thanks.

----------- genname.c --------
/* This function adds and links a CERTGeneralName to a CERTNameConstraint
** list. Most likely the CERTNameConstraint passed in is either the permited
** list or the excluded list of a CERTNameConstraints.
*/
SECStatus
CERT_AddNameConstraintByGeneralName(PLArenaPool *arena,
				    CERTNameConstraint **constraints,
				    CERTGeneralName *name)
{
    SECStatus rv;
    CERTNameConstraint *current = NULL;
    CERTNameConstraint *first = *constraints;
    CERTNameConstraint *last = *constraints;

    current = PORT_ArenaZNew(arena, CERTNameConstraint);
    if (current == NULL) {
	return SECFailure;
    }

    rv = cert_CopyOneGeneralName(arena, &current->name, name);
    if (rv != SECSuccess) {
	return SECFailure;
    }

    current->name.l.prev = current->name.l.next = &(current->name.l);

    if (first == NULL) {
	first = last = current;
	*constraints = current;
    }
    current->l.prev = &(last->l);
    current->l.next = last->l.next;
    last->l.next = &(current->l);

    return SECSuccess;

}

/* This function extracts the name constraints extension from the CA cert.
*/
SECStatus
CERT_FindNameConstraintsExten(PRArenaPool      *arena,
			      CERTCertificate  *cert,
			      CERTNameConstraints **constraints)
{
    SECStatus            rv;
    SECItem              constraintsExtension;

    *constraints = NULL;

    rv = CERT_FindCertExtension(cert, SEC_OID_X509_NAME_CONSTRAINTS, 
                                &constraintsExtension);
    if (rv != SECSuccess) {
	if (PORT_GetError() == SEC_ERROR_EXTENSION_NOT_FOUND) {
	    rv = SECSuccess;
	}
	goto done;
    }

    /* TODO: mark arena */
    *constraints = cert_DecodeNameConstraints(arena, &constraintsExtension);

    if (*constraints == NULL) { /* decode failed */
	rv = SECFailure;
	goto done;
    }
    PORT_Free (constraintsExtension.data);

done:
    return rv;
}

/* This function verifies name against all the constraints relevant
** to that type of the name.
*/
SECStatus
CERT_CheckNameSpace(PRArenaPool          *arena,
		    CERTNameConstraints  *constraints,
		    CERTGeneralName      *currentName)
{
    CERTNameConstraint  *matchingConstraints;
    SECStatus            rv = SECSuccess;

    if (constraints->excluded != NULL) {
 	rv = CERT_GetNameConstraintByType(constraints->excluded, 
					  currentName->type, 
					  &matchingConstraints, arena);
	if (rv == SECSuccess && matchingConstraints != NULL) {
 	    rv = cert_CompareNameWithConstraints(currentName, 
						 matchingConstraints,
						 PR_TRUE);
	}
	if (rv != SECSuccess) {
	    return(rv);
	}
    }

    if (constraints->permited != NULL) {
 	rv = CERT_GetNameConstraintByType(constraints->permited, 
					  currentName->type, 
					  &matchingConstraints, arena);
	if (rv == SECSuccess && matchingConstraints != NULL) {
 	    rv = cert_CompareNameWithConstraints(currentName, 
						 matchingConstraints,
						 PR_FALSE);
	}
	if (rv != SECSuccess) {
	    return(rv);
	}
    }

    return(SECSuccess);
}

/* Extract the name constraints extension from the CA cert.
** Test each and every name in namesList against all the constraints
** relevant to that type of name.
** Returns NULL for success, all names are acceptable.
** If some name is not acceptable, returns a pointer to the cert that
** contained that name.
*/
SECStatus
CERT_CompareNameSpace(CERTCertificate  *cert,
		      CERTGeneralName  *namesList,
 		      CERTCertificate **certsList,
 		      PRArenaPool      *arena,
 		      CERTCertificate **pBadCert)
{
    SECStatus            rv;
    CERTNameConstraints  *constraints;
    CERTGeneralName      *currentName;
    int                  count = 0;
    CERTCertificate      *badCert = NULL;

    rv = CERT_FindNameConstraintsExten(arena, cert, &constraints);
    if (rv != SECSuccess) {
	    count = -1;
	    goto done;
    }

    currentName = namesList;
    do {
	rv = CERT_CheckNameSpace(arena, constraints, currentName);
	if (rv != SECSuccess) {
	    break;
	}
 	currentName = CERT_GetNextGeneralName(currentName);
 	count ++;
    } while (currentName != namesList);

done:
    if (rv != SECSuccess) {
	badCert = (count >= 0) ? certsList[count] : cert;
    }
    if (pBadCert)
	*pBadCert = badCert;
    /* TODO: release back to mark */
    return rv;
}

Comment 3

[Hanfei Yu]

Attachment: Split CERT_CompareNameSpace in genname.c into two functions. Also provide a new one.

Comment 4

[Julien Pierre]

Retargetting to 3.12 .

Comment 5

[Julien Pierre]

Attachment: Updated patch that includes header file changes

Checking in genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.28.10.2; previous revision: 1.28.10.1
done
Checking in cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--	cert.h
new revision: 1.53.10.1; previous revision: 1.53
done

Comment 6

[Julien Pierre]

This was checked in on NSS_LIBPKIX_BRANCH only .

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This bug has a patch that changes an NSS source file that is NOT one of 
the new files for libPKIX.  It does not appear to have been reviewed.  
IINM, the new libPKIX code depends on these changes, and will not build 
without them.  According to comment 6, these changes were committed only
on the libPKIX branch.  So, I suspect that the new libPKIX files that were
recently committed on the trunk do not build without error, since this patch
is not on the trunk.  

So, for NSS 3.12, we need to review this patch very carefully (the name
constraints code is rather tricky) and commit it on the trunk if good.

Comment 8

[Alexei Volkov]

will be integrate as a part of the patch for bug 263373

Comment 9

[Alexei Volkov]

(In reply to comment #8)
> will be integrate as a part of the patch for bug 263373
                                               ^^^^^^^^^^
Correction: bug 358785