Closed Bug 842856 Opened 8 years ago Closed 8 years ago

Add SEC_PKCS7VerifyDetachedSignatureAtTime for verifying the certificate chain at the current time instead of the signing time

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: briansmith)

References

Details

Attachments

(3 files, 1 obsolete file)

The SEC_PKCS7VerifyDetachedSignatureAtTime function was originally added
to the copy in NSS in mozilla-central in bug 834091.

This bug seeks to add the function to NSS officially. Attached is the
current NSS patch in mozilla-central.
Attached patch Proposed patchSplinter Review
You can diff against the mozilla-central patch to see the changes
I made.

1. I removed the special meaning of atTime=0 for
SEC_PKCS7VerifyDetachedSignatureAtTime. The caller can call PR_Now()
easily.

2. I documented the "keepcerts" argument.

3. I documented the "atTime" argument to the internal function
sec_pkcs7_verify_signature.

4. I changed the nested tertiary ? : expression to an if-else-if-else
statement. If you prefer the original form, I'll change it back.
Attachment #715794 - Flags: superreview?(rrelyea)
Attachment #715794 - Flags: review?(bsmith)
Comment on attachment 715794 [details] [diff] [review]
Proposed patch

Review of attachment 715794 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/security/nss/lib/pkcs7/p7decode.c
@@ +1254,5 @@
>   *	The verification checks that the signing cert is valid and trusted
> + *	for the purpose specified by "certusage" at
> + * 	- "atTime" if "atTime" is non-zero, or
> + * 	- the signing time if the signing time is available in "cinfo", or
> + *	- the current time (as returned by PR_Now).

Wan-Teh, my understanding is that you wanted to remove the special mean of a zero atTime. But, with this code, a zero atTime passed to SEC_PKCS7VerifyDetachedSignatureAtTime will still have a special, and now undocumented, meaning.

I think that instead sec_pkcs7_verify_signature should take a PRTime* atTime, and treat a NULL atTime like it currently treats a zero atTime. Then SEC_PKCS7VerifyDetachedSignatureAtTime can pass in a non-NULL atTime and SEC_PKCS7VerifyDetachedSignature can pass in a NULL atTime and everything will work as callers expect. In particular, a caller that passes PRTime(0) to SEC_PKCS7VerifyDetachedSignatureAtTime will get the cert validated as of January 1, 1970.

Do you agree? If so, I can make this change to the patch.
Attachment #715794 - Flags: review?(bsmith) → review+
BTW, I did verify that the caller within Gecko is already passing PR_Now() and not zero to SEC_PKCS7VerifyDetachedSignatureAtTime.
(In reply to Brian Smith (:bsmith) from comment #2)
>
> Wan-Teh, my understanding is that you wanted to remove the special mean of a
> zero atTime. But, with this code, a zero atTime passed to
> SEC_PKCS7VerifyDetachedSignatureAtTime will still have a special, and now
> undocumented, meaning.

Ah, you're right. I missed this.

I think the easiest fix is for SEC_PKCS7VerifyDetachedSignatureAtTime to require
atTime to be at least, say, year 1993, which is the year of publication of PKCS
#7 v1.5. (I can't find an earlier version of PKCS #7.)

But you said in private email that we can abandon this patch, right?
This is a patch on top of attachment 715794 [details] [diff] [review]. It simply makes it so that a zero atTime is treated as Jan 1, 1970 (or so), which is what a zero PRTime normally means.
Attachment #715785 - Attachment is obsolete: true
Attachment #728118 - Flags: review?(rrelyea)
Assignee: wtc → bsmith
Target Milestone: 3.14.4 → 3.15
Brian,

I thought you're going to remove the use of
SEC_PKCS7VerifyDetachedSignatureAtTime in mozilla-central, in
which case this bug should be closed WONTFIX. Are you still
planning to do that?
Wan-Teh, my understanding of the discussions was:
Because it's being used by b2g in a shipped product, we add it to NSS.
What are the plans for that one? Apparently Aurora currently does not build against plain NSS 3.14.3 so I'd like to know when Firefox (and others) are using a NSS release version again.
Comment on attachment 715794 [details] [diff] [review]
Proposed patch

r+ rrelyea
Attachment #715794 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 728118 [details] [diff] [review]
Do not interpret a zero atTime to mean PR_Now()

r+ rrelyea
Attachment #728118 - Flags: review?(rrelyea) → review+
It's great to see progress on this bug.

I would prefer to include this patch in today's beta tag.
Whiteboard: checkin-needed
I consider to check it in myself, to allow me to proceed with the beta tag.
A local build and test cycle with these patches succeeded.
Checked in.
https://hg.mozilla.org/projects/nss/rev/8b2a6b065391
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Attachment #745462 - Flags: review?(bsmith) → review+
You need to log in before you can comment on or make changes to this bug.