Please make cert_CompareNameWithConstraints a non-static function

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Hanfei Yu, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hanfei, You proposed splitting this function.  Would you care to contribute
a patch implementing that suggestion?
Assignee: wtchang → julien.pierre.bugs
Priority: -- → P2
Target Milestone: --- → 3.11
(Reporter)

Comment 2

13 years ago
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;
}
(Reporter)

Comment 3

13 years ago
Created attachment 180787 [details]
Split CERT_CompareNameSpace in genname.c into two functions. Also provide a new one.
QA Contact: bishakhabanerjee → jason.m.reid

Comment 4

12 years ago
Retargetting to 3.12 .
Target Milestone: 3.11 → 3.12

Updated

12 years ago
OS: Solaris → All
Hardware: Sun → All
Version: unspecified → 3.10

Comment 5

12 years ago
Created attachment 189442 [details] [diff] [review]
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
Attachment #180787 - Attachment is obsolete: true

Comment 6

12 years ago
This was checked in on NSS_LIBPKIX_BRANCH only .
QA Contact: jason.m.reid → libraries
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.
Assignee: bugzilla → alexei.volkov.bugs
Whiteboard: PKIX
(Assignee)

Comment 8

10 years ago
will be integrate as a part of the patch for bug 263373
Depends on: 263373
(Assignee)

Comment 9

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

Depends on: 358785
No longer depends on: 263373
Blocks: 358785
Status: NEW → RESOLVED
Last Resolved: 10 years ago
No longer depends on: 358785
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.