435007 - avoid keeping a stale pointer in libpkix ocsp processing

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

In libpkix ocsp processing we keep a pointer to an ocsp id object that had its ownership transferred to another context.

Although I believe (hope) the code correctly ensured we never dereferenced that dangling pointer, we should ensure there is no chance to dereference it.

Nelson proposed to set the pointer to NULL as soon as we no longer own it, and I concur.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 321938 [details]
patch v1

Thanks, Kai, for writing this patch. 
I have a slight worry that there may be new crashes in places where 
certID->certID was never NULL before, but now will be NULL. 
I'm not confident that our OCSP testing tests enough different cases
for us to be confident.  

I suggest one additional change to some code patched by this patch.
In pkix_pl_OcspResponse_GetStatusForCert() I would also add:

>         PKIX_NULLCHECK_TWO(response->signerCert, response->request);
>+        PKIX_NULLCHECK_TWO(cid, cid->certID);
> 
>         rv = cert_ProcessOCSPResponse(response->handle,
>                                       response->nssOCSPResponse,
>                                       cid->certID,
>                                       response->signerCert,

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 321938 [details]
patch v1

clearing review request.

Please see bug 433386.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 321938 [details]
patch v1

A variation of this patch now lives in attachment 321990 [details] [diff] [review], I added Nelson's proposal from comment 2 (added null checks in two functions).

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Kai,  The attachment you described in comment 4 above was attached to 
Bug 433386, which is now resolved fixed.  So, is THIS bug now fixed?

Comment 6

[Kai Engert [:KaiE:]]

(In reply to comment #5)
> Kai,  The attachment you described in comment 4 above was attached to 
> Bug 433386, which is now resolved fixed.  So, is THIS bug now fixed?


Yes, thanks for the reminder.
Marking fixed by the checkin from bug 433386.