Patches for CERT_CacheOCSPResponseFromSideChannel

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Adam Langley)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Created attachment 434271 [details] [diff] [review]
change the caching semantics of side channel responses (by Adam Langley)

This bug continues the work of bug 542538 on caching stapled OCSP responses.

The first patch was originally attached in bug 542538 comment 27.
Attachment #434271 - Flags: review?(wtc)
(Reporter)

Comment 1

7 years ago
Created attachment 434274 [details] [diff] [review]
fix bug that I noticed in my travels (by Adam Langley)

This patch was originally attached by Adam Langley in bug 542538 comment 28,
with the following description:

I don't believe that cert_RememberOCSPProcessingFailure will currently work in
all cases. It calls ocsp_CreateOrUpdateCacheEntry with single==NULL and that
causes ocsp_CreateOrUpdateCacheEntry to set the missingResponseError on any
cache entry that it finds.

However, if there is already a cache entry, and that cache entry has a
certStatusArena, then setting missingResponseError is not sufficient. It's the
non-NULLness of certStatusArena that is checked in
ocsp_GetCachedOCSPResponseStatusIfFresh
Attachment #434274 - Flags: review?(wtc)
(Reporter)

Updated

7 years ago
Target Milestone: 3.12.7 → 3.12.9

Comment 2

5 years ago
I'm really glad I just found this bug report and these patches!!!

With the current state of NSS in CVS,
OCSP stapling / side channel data will never be stored in the OCSP cache,
if the OCSP response has a valid signature and says revoked cert.

I agree with this patch to change this behaviour,
and always cache such information, even if it came from a side channel.

This change of behaviour is necessary for my work in bug 700701,
where I want to enhance the tstclnt tool to report the information retrieved
via the side channel.
Blocks: 700701

Updated

5 years ago
Target Milestone: 3.12.9 → 3.14

Comment 3

5 years ago
These patches no longer apply cleanly, but luckily,
it's only a matter of context.

Luckily this means, 
it's still fine to review the patches that are attached in this bug.


Unfortunately the context between your patch and the patch in bug 360420 is too close.

I merged our patches, and there are just two lines of context between some of the changes, so it's impossible to create separate unified patches, not even with cvs -u2

I think we should get the code in this bug reviewed first and checked in,
then I will attach an updated patch to bug 360420.

Until then I'll carry the merged version of your patch in my tree and 
include it in future patches to bug 360420.
Blocks: 360420
Target Milestone: 3.14 → 3.14.1
(Reporter)

Updated

5 years ago
Target Milestone: 3.14.1 → 3.14.2

Comment 4

4 years ago
Comment on attachment 434271 [details] [diff] [review]
change the caching semantics of side channel responses (by Adam Langley)

I agree with these patches. I've merged them into the "injection" patches in bug 360420, since patches are overlapping.
Attachment #434271 - Flags: review?(wtc) → review+

Comment 5

4 years ago
Comment on attachment 434274 [details] [diff] [review]
fix bug that I noticed in my travels (by Adam Langley)

r=kaie on both patches
Attachment #434274 - Flags: review?(wtc) → review+

Comment 6

4 years ago
Marking as resolved, based on Kai's comment #4 and bug 360420
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: 3.14.2 → 3.14.3

Updated

4 years ago
Target Milestone: 3.14.3 → 3.14.4

Comment 7

4 years ago
checked in, see bug 360420 comment 160

Updated

4 years ago
Target Milestone: 3.14.4 → 3.15
You need to log in before you can comment on or make changes to this bug.