nss: only cache valid CRL entries

[danielgustafsson]

Change the logic around such that we only keep CRLs that NSS actually ended up caching around for later deletion. If CERT_CacheCRL() fails then there is little point in delaying the freeing of the CRL as it is not used.

[kdudka]

You need to call CERT_UncacheCRL() before releasing the memory to avoid a possible use after free later on. If CERT_UncacheCRL() fails, which is unlikely, it is IMO better to leak the memory. Something like:

if(SECSuccess == CERT_UncacheCRL(db, crl_der))
  SECITEM_FreeItem(crl_der, PR_TRUE);

[danielgustafsson]

Ah, nice catch, fixed and rebased.

[danielgustafsson]

A thing to note which I failed to write in the commitmessage (but will amend before pushing if approved) is that without this we leave a dangling pointer on nss_crl_list as we perform SECITEM_FreeItem() on the underlying storage pointed to in case CRL caching fails.

[kdudka]

A thing to note which I failed to write in the commitmessage (but will amend before pushing if approved) is that without this we leave a dangling pointer on nss_crl_list as we perform SECITEM_FreeItem() on the underlying storage pointed to in case CRL caching fails.

I do not understand the above note. Are you talking about the original implementation? It did not (immediately) call SECITEM_FreeItem() upon failure of CERT_CacheCRL(). The only problem was that the memory was released too late as I understand it.

[danielgustafsson]

On 20 Jun 2019, at 15:16, Kamil Dudka ***@***.***> wrote: I do not understand the above note. Are you talking about the original implementation? It did not (immediately) call SECITEM_FreeItem() upon failure of CERT_CacheCRL(). The only problem was that the memory was released too late as I understand it.

Sorry, I managed to confuse myself reading the code - the original implementation did indeed not have such a bug. I clearly need more coffee today..

[kdudka]

No problem, thank you for clarifying it! And thank you for improving the code!

[jay]

Thanks