430859 - PKIX: Policy mapping fails verification with error "invalid arguments"

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Christophe Ravel]

Policy Mapping configuration:

Root CA [RootCA]
   v
CA1 (PolicyExt: OID.1.0, Mapping: Issuer=OID.1.0, Subject=OID.1.1) [RootCACA1cert.der]
   v
CA2 (PolicyExt: OID.1.1) [ CA1CA2cert.der]
   v
EE cert (PolicyExt: OID.1.1) [CA2User1cert.der]


------
AllDB database contains the certificates for RootCA, CA1 and CA2.

-----------------------------------------------
vfychain -d AllDB -pp -vv -o OID.1.1 CA2User1cert.der -t RootCA
Chain is bad, -8187 = security library: invalid arguments.
RESULT: FAIL
===============================================

Comments from Nelson:
This should have passed.  This is a bug.

Invalid arguments is a TERRIBLE error code.
Invalid arguments means "a programming error was experienced".
It does not mean "the cert chain is good" or "the cert chain is bad".

Comment 1

[Christophe Ravel]

See Bug 430856 – "Policy mapping gives false positive result" for test environment to reproduce this error.

Comment 2

[Christophe Ravel]

Here is the call stack for the invalid args error.

nss3.dll!PKIX_DoThrow(pkixStdVarsStr * stdVars=0x0012faa4, PKIX_ERRORCLASS errClass=PKIX_FATAL_ERROR, PKIX_ERRORCODE errCode=PKIX_NULLARGUMENT, PKIX_ERRORCLASS overrideClass=PKIX_AIAMGR_ERROR, void * plContext=0x00d8e728)  Line 53	C

nss3.dll!PKIX_DoReturn(pkixStdVarsStr * stdVars=0x0012faa4, PKIX_ERRORCLASS errClass=PKIX_FATAL_ERROR, int doLogger=1, void * plContext=0x00d8e728)  Line 88 + 0x1f bytes	C

>>nss3.dll!pkix_VerifyNode_SetError(PKIX_VerifyNodeStruct * node=0x00da6164, PKIX_ErrorStruct * >>error=0x00000000, void * plContext=0x00d8e728)  Line 1134 + 0x34 bytes	C

nss3.dll!pkix_BuildForwardDepthFirstSearch(void * * pNBIOContext=0x0012fca8, PKIX_ForwardBuilderStateStruct * * pState=0x0012fbdc, PKIX_ValidateResultStruct * * pValResult=0x0012fc48, void * plContext=0x00d8e728)  Line 2706 + 0x14 bytes	C

nss3.dll!pkix_Build_InitiateBuildChain(PKIX_ProcessingParamsStruct * procParams=0x00d921b4, void * * pNBIOContext=0x0012fd24, PKIX_ForwardBuilderStateStruct * * pState=0x0012fcf0, PKIX_BuildResultStruct * * pBuildResult=0x0012fd28, PKIX_VerifyNodeStruct * * pVerifyNode=0x0012fd78, void * plContext=0x00d8e728)  Line 4198 + 0x18 bytes	C

nss3.dll!PKIX_BuildChain(PKIX_ProcessingParamsStruct * procParams=0x00d921b4, void * * pNBIOContext=0x0012fd7c, void * * pState=0x0012fd70, PKIX_BuildResultStruct * * pBuildResult=0x0012fd90, PKIX_VerifyNodeStruct * * pVerifyNode=0x0012fd78, void * plContext=0x00d8e728)  Line 4388 + 0x1d bytes	C

nss3.dll!CERT_PKIXVerifyCert(CERTCertificateStr * cert=0x00d8d280, __int64 usages=2, CERTValInParam * paramsIn=0x0041b9a8, CERTValOutParam * paramsOut=0x0041b8e8, void * wincx=0x00000000)  Line 2160 + 0x1d bytes	C

vfychain.exe!main(int argc=12, char * * argv=0x00c35a18, char * * envp=0x00c33668)  Line 518 + 0x1d bytes	C

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Here is an analysis of how the null pointer error occurs.

step 1, call stack is:

nss3.dll!pkix_BuildForwardDepthFirstSearch()  Line 3047 
nss3.dll!pkix_Build_InitiateBuildChain()  Line 4198 
nss3.dll!PKIX_BuildChain()  Line 4388 
nss3.dll!CERT_PKIXVerifyCert()  Line 2160
vfychain.exe!main()  Line 518 

at line 3038

            if (state->status == BUILD_VALCHAIN2) {
                    PKIX_CHECK_ONLY_FATAL
                            (pkix_Build_ValidateEntireChain
                            (state,
                            trustAnchor,
                            &nbio,
                            &valResult,
                            verifyNode,
                            plContext),
                            PKIX_BUILDVALIDATEENTIRECHAINFAILED);

pkix_Build_ValidateEntireChain returns an error.  The error is:

 stdVars.aPkixErrorResult
	errCode		PKIX_CHECKCHAINFAILED	PKIX_ERRORCODE
	errClass	PKIX_BUILD_ERROR	PKIX_ERRORCLASS
	plErr		0			unsigned int
	cause	0x00dacd34 
	    errCode	PKIX_CERTCHAINCHECKERCHECKFAILED	PKIX_ERRORCODE
	    errClass	PKIX_VALIDATE_ERROR		PKIX_ERRORCLASS
	    plErr	-8049	
	    cause	0x00000000 
	    info	0x00000000 

The PKIX_CHECK_ONLY_FATAL macro sets pkixTempErrorReceived to 1, and
sets pkixErrorClass to PKIX_BUILD_ERROR, and continues.

Step 2, a long time later in the execution of the same function call,
at line 2683, we call pkix_Build_VerifyCertificate.  It returns NULL,
so verifyError is NULL, but pkixTempErrorReceived is still 1 from before.
pkixErrorResult is now zero.

So, at line 2702,
            if (PKIX_ERROR_RECEIVED) {
                    if (state->verifyNode != NULL) {
                        PKIX_CHECK_FATAL(pkix_VerifyNode_SetError
                            (verifyNode, verifyError, plContext),
                            PKIX_VERIFYNODESETERRORFAILED);

PKIX_ERROR_RECEIVED is true, but verifyError is NULL.  
That is the immediate cause of the FATAL Null parameter error 
returned by pkix_VerifyNode_SetError.

If I clear pkixTempErrorReceived at line 2702, then the vfychain program
outputs 

Chain is bad, -8172 = Peer's certificate issuer has been marked as not trusted by the user.

Comment 4

[Alexei Volkov]

I concur that the reason for the error(bad arguments) that was initially reported in the bug was PKIX_TRUE value of pkixTempErrorReceived that was set by PKIX_CHECK_ONLY_FATAL macro after executing pkix_Build_ValidateEntireChain at pkix_build.c:3045. 

There are multiple cases in pkix_BuildForwardDepthFirstSearch there pkixTempErrorReceived is set to PKIX_TRUE, but is not cleaned after it goes out of the area of immediate use. They all should be fixed.

The error that Nelson reported in his last comment(-8172) I believe is related to bug 430856.

Comment 5

[Alexei Volkov]

Attachment: Patch v1

reset pkixTempErrorReseived to PKIX_FALSE in multiple places through out pkix_BuildForwardDepthFirstSearch function.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 320392 [details] [diff] [review]
Patch v1

The patch inserts one statement into the code in numerous places.  It is:
    pkixTempErrorReceived = PKIX_FALSE;
I wanted to convince myself (or disprove to myself) that the places where
it was inserted were proper and complete, that is, that there weren't any
places missing, and that there weren't any places where they were incorrect.

I tried to see if I could find a pattern, or "rule", that had governed
where those had been inserted.  I decided the statement was inserted
following any test of PKIX_ERROR_RECEIVED (which took place outside of a 
macro), in the path of execution that occurs when PKIX_ERROR_RECEIVED is 
true, but not always immediately after each test.  

In some cases, some other tests were allowed to be done after the 
PKIX_ERROR_RECEIVED test, before clearing that variable.  I didn't 
understand why, by what rule, it was OK to not clear that flag in some
cases.  

My big remaining concern is: How do we know that this patch patches all 
the places where it's needed? 
Or, to ask it another way, how did you come to decided that that was the
set of places to put the statements?

I think this patch probably fixes the problem for the one reported case
where we ran into it (until now).  But I wonder if we might choose 
another approach once we know how to tell that the job is complete.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 320392 [details] [diff] [review]
Patch v1

At Alexei's request, I am giving this patch an r-.

Alexei, please add a comment to this bug explaining how you chose this particular set of places to insert this line of code.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 320392 [details] [diff] [review]
Patch v1

I'm changing my review to r+.  I'm not sure this patch is complete, but it's better than the code without it, and some fix is needed now.

Comment 10

[Alexei Volkov]

I agree that the code is better with the patch v1 integrated. Integrating Patch v1. More to come.

Comment 11

[Alexei Volkov]

Attachment: Additional patch to lean up after PKIX_CHECK_ONLY_FATAL macro

I've reviewed places where the macro is used. The patch fixes the only problem that I've found: file pkix_pl_ldapcertstore.c:326 - pkixTempErrorReceived should be reset to PKIX_FALSE since, in case of error, it will effect returned crl list by destroying list pointed by crlList after "cleanup" label.

Comment 12

[Alexei Volkov]

Comment on attachment 330665 [details] [diff] [review]
Additional patch to lean up after PKIX_CHECK_ONLY_FATAL macro

Patch has been integrated.