391180 - NBIO: can not return to interrupted libpkix code path with completed IO

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

There are no state for some code paths in pkix_build.c that can be interrupted with blocked IO. I suspect the program will not be able to return to such code paths after an IO completion. 

Example from pkix_build.c: we will not get to the code that continued cert chain validation if enter the function with non-null state.

 3589 static PKIX_Error *
3590 pkix_Build_InitiateBuildChain(
3591         PKIX_ProcessingParams *procParams,
3592         void **pNBIOContext,
3593         PKIX_ForwardBuilderState **pState,
3594         PKIX_BuildResult **pBuildResult,
3595         PKIX_VerifyNode **pVerifyNode,
3596         void *plContext)
....
3640         if (*pState != NULL) {
3641             state = *pState;
3642             *pState = NULL; /* no net change in reference count */
3643             /* attempted shortcut ran into non-blocking I/O */
3644         } else {
3645 
3646             PKIX_CHECK(PKIX_ProcessingParams_GetDate
3647                     (procParams, &testDate, plContext),
3648                     PKIX_PROCESSINGPARAMSGETDATEFAILED);
....
3935             /* Check whether this cert verification has been cached. */
3936             PKIX_CHECK(pkix_CacheCertChain_Lookup
3937                     (targetCert,
3938                     anchors,
3939                     testDate,
3940                     &cacheHit,
3941                     &buildResult,
3942                     plContext),
3943                     PKIX_CACHECERTCHAINLOOKUPFAILED);
3944     
3945             if (cacheHit) {
3946                     /*
3947                      * We found something in cache. Verify that the anchor
3948                      * cert is still trusted,
3949                      */
3950                     PKIX_CHECK(PKIX_BuildResult_GetValidateResult
3951                             (buildResult, &valResult, plContext),
3952                             PKIX_BUILDRESULTGETVALIDATERESULTFAILED);
3953     
3954                     PKIX_CHECK(PKIX_ValidateResult_GetTrustAnchor
3955                             (valResult, &matchingAnchor, plContext),
3956                             PKIX_VALIDATERESULTGETTRUSTANCHORFAILED);
3957     
3958                     PKIX_DECREF(valResult);
3959     
3960                     PKIX_CHECK(PKIX_TrustAnchor_GetTrustedCert
3961                             (matchingAnchor, &trustedCert, plContext),
3962                             PKIX_TRUSTANCHORGETTRUSTEDCERTFAILED);
3963     
3964                     PKIX_CHECK(PKIX_PL_Cert_IsCertTrusted
3965                             (trustedCert, &trusted, plContext),
3966                             PKIX_CERTISCERTTRUSTEDFAILED);
3967     
3968                     if (trusted == PKIX_TRUE) {
3969                             /*
3970                              * Since the key usage may vary for different
3971                              * applications, we need to verify the chain again.
3972                              */
3973                             PKIX_CHECK(PKIX_BuildResult_GetCertChain
3974                                     (buildResult, &certList, plContext),
3975                                     PKIX_BUILDRESULTGETCERTCHAINFAILED);
3976     
3977                             PKIX_CHECK(pkix_Build_ValidationCheckers
3978                                     (state,
3979                                     certList,
3980                                     matchingAnchor,
3981                                     plContext),
3982                                     PKIX_BUILDVALIDATIONCHECKERSFAILED);
3983     
3984                             PKIX_CHECK_ONLY_FATAL(pkix_Build_ValidateEntireChain
3985                                     (state,
3986                                     matchingAnchor,
3987                                     &nbioContext,
3988                                     &valResult,
3989                                     verifyNode,
3990                                     plContext),
3991                                     PKIX_BUILDVALIDATEENTIRECHAINFAILED);
3992 
3993                         if (nbioContext != NULL) {
3994                                 /* IO still pending, resume later */
3995                                 *pNBIOContext = nbioContext;
3996                                 goto cleanup;
3997                             } else {
3998                                 PKIX_DECREF(state->reversedCertChain);
.... 
4115 cleanup:
4116 
4117         PKIX_DECREF(targetConstraints);
4118         PKIX_DECREF(targetParams);
4119         PKIX_DECREF(anchors);
4120         PKIX_DECREF(targetSubjNames);
4121         PKIX_DECREF(targetCert);
4122         PKIX_DECREF(crlChecker);
4123         PKIX_DECREF(certStores);
4124         PKIX_DECREF(certStore);
4125         PKIX_DECREF(userCheckers);
4126         PKIX_DECREF(hintCerts);
4127         PKIX_DECREF(firstHintCert);
4128         PKIX_DECREF(testDate);
4129         PKIX_DECREF(targetPubKey);
4130         PKIX_DECREF(tentativeChain);
4131         PKIX_DECREF(valResult);
4132         PKIX_DECREF(certList);
4133         PKIX_DECREF(matchingAnchor);
4134         PKIX_DECREF(trustedCert);
4135         PKIX_DECREF(state);
4136 
4137         PKIX_RETURN(BUILD);
4138 }

Comment 1

[Alexei Volkov]

Another place where it seems the NBIO code was not tested:

File pkix_pl_httpdefaultclient.c: functions pkix_pl_HttpDefaultClient_RecvHdrContinue and  pkix_pl_HttpDefaultClient_RecvBodyContinue write received bytes in to the buffer pointer referenced from PKIX_PL_Socket object. It is incorrect, since the header parsing function works only with a buffer referenced from PKIX_PL_HttpDefaultClient object. 

Comment 2

[Alexei Volkov]

Yet another place: most of the code that follows PKIX_CHECK_ONLY_FATAL macro does not clear pkixTermErrorRecieved variable. This will make libpkix to throw an error each time when a no-fatal error happen and returned by a function surrounded by the macro.

In addition, all places in the code where pkixTermErrorRecieved is set to true should be checked for similar condiditons.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.