Closed Bug 834091 Opened 11 years ago Closed 11 years ago

JARSignatureVerification uses the signing time instead of the current time for verification of the certificate chain

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: briansmith, Assigned: macajc)

References

Details

Attachments

(1 file, 2 obsolete files)

Originally we did not think it was necessary to fix this issue for B2G 1.0. However, now we have changed our minds and we consider it very important, as a defense-in-depth measure for the Marketplace.

I suggest that we add a new variant of SEC_PKCS7VerifyDetachedSignature to NSS, SEC_PKCS7VerifyDetachedSignatureAtTime, which has the same signature as SEC_PKCS7VerifyDetachedSignature plus a |PRTime time| parameter. This variant would ignore the signingTime data in the PKCS7 signature, and instead it would always use the given time (which would almost always be PR_Now()) for its call to CERT_VerifyCert.
Someone will ask this question in triage, so I'll ask it:

Can you explain why this has to block a bit more? It's a bit unclear from reading this right now.
To be honest, I'm also curious about why the change of mind. Although I can think of at least one reason of why it could be a good idea to change this now instead of later.

We can take this if you wish, Brian. In fact I think we can have a patch ready for when you awake ;). Well do it as you asked, adding a new function, although I have a question/doubt:

Why don't we just fix SEC_PKCS7VerifyDetachedSignature so it *always* use PR_Now as the date to check the signature? 

That would be the correct thing to do, since signature dates (the ones that the signer include in the signature) are at most informative, not authoritative. So they should be irrelevant as to whether a signature is valid or not. Specially after the signing certificate is no longer valid.
blocking-b2g: tef? → tef+
Attached patch V1, with required changes. (obsolete) — Splinter Review
I think this patch does what c0 requested. Note that since I added a new function that's used externally (in JARSignatureVerification.cpp) I had to touch smime.def and increase the NSS version there. Otherwise the new function doesn't get included and the toolkit/library build fails.
Assignee: bsmith → macajc
Status: NEW → ASSIGNED
Attachment #705886 - Flags: review?(bsmith)
Comment on attachment 705886 [details] [diff] [review]
V1, with required changes.

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

Please forgive the unsolicited feedback. I'm taking a look at some of these patches to get more familiar with PSM/NSS and to free up some of bsmith's time. These comments are just suggestions, so if Brian tells you to do the exact opposite of what I say, my apologies.

::: security/manager/ssl/src/JARSignatureVerification.cpp
@@ +590,2 @@
>                                          &sfCalculatedDigest.get(), HASH_AlgSHA1,
> +                                        false, PR_Now())) {

These two lines should be indented 6 more spaces to line up with the line above them.

::: security/nss/lib/pkcs7/p7decode.c
@@ +1280,5 @@
>  sec_pkcs7_verify_signature(SEC_PKCS7ContentInfo *cinfo,
>  			   SECCertUsage certusage,
>  			   const SECItem *detached_digest,
>  			   HASH_HashType digest_type,
> +			   PRBool keepcerts, PRInt64 atTime)

As each other argument here is on its own line, I would expect that "PRInt64 atTime" should be on its own line.

@@ +1438,5 @@
>       */
>      if (CERT_VerifyCert (certdb, cert, PR_TRUE, certusage,
> +                         (atTime ? 
> +                          atTime : 
> +                          encoded_stime != NULL ? stime : PR_Now()),

There are some trailing spaces here.
Also, this expression is a bit cumbersome to have inline in the call to CERT_VerifyCert. Maybe break it out and use a temporary variable?

@@ +1788,5 @@
> + * SEC_PKCS7VerifyDetachedSignatureAtTime
> + *	Look at a PKCS7 contentInfo and check if the signature matches
> + *	a passed-in digest (calculated, supposedly, from detached contents).
> + *	The verification checks that the signing cert is valid (at the atTime
> + *      time if !=0 or at the signing time if the pkcs7 has a signing time or

Maybe write out "if atTime is nonzero" instead of "!=0".
Also, to avoid duplicating a long comment, you could say something like "The same as SEC_PKCS7VerifyDetachedSignature, except..." and then talk about the atTime argument.

@@ +1800,5 @@
> +				       SECCertUsage certusage,
> +				       const SECItem *detached_digest,
> +				       HASH_HashType digest_type,
> +				       PRBool keepcerts, 
> +				       int64 atTime) 

Trailing space at the end of this line and the line above.

::: security/nss/lib/pkcs7/secpkcs7.h
@@ +139,5 @@
> + * SEC_PKCS7VerifyDetachedSignatureAtTime
> + *	Look at a PKCS7 contentInfo and check if the signature matches
> + *	a passed-in digest (calculated, supposedly, from detached contents).
> + *	The verification checks that the signing cert is valid (at the atTime
> + *      time if !=0 or at the signing time if the pkcs7 has a signing time or

Whatever the other comment ends up being, make sure they're the same, I guess.

@@ +150,5 @@
> +						     SECCertUsage certusage,
> +						     const SECItem *detached_digest,
> +						     HASH_HashType digest_type,
> +						     PRBool keepcerts, 
> +						     PRInt64 atTime); 

Trailing spaces again.
Attachment #705886 - Flags: feedback+
Includes the feedback comments
Attachment #705886 - Attachment is obsolete: true
Attachment #705886 - Flags: review?(bsmith)
Attachment #706002 - Flags: review?(bsmith)
(In reply to David Keeler (:keeler) from comment #4)
> Comment on attachment 705886 [details] [diff] [review]
> V1, with required changes.
> 
> Review of attachment 705886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please forgive the unsolicited feedback. I'm taking a look at some of these
> patches to get more familiar with PSM/NSS and to free up some of bsmith's
> time. 

No problem at all, the more the merrier :) I uploaded a new version including your comments, thanks for the review and your time!
Comment on attachment 706002 [details] [diff] [review]
V2, with feedback comments included

feedback + for the new function.
Attachment #706002 - Flags: feedback+
Comment on attachment 706002 [details] [diff] [review]
V2, with feedback comments included

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

::: security/nss/lib/pkcs7/p7decode.c
@@ +1798,5 @@
> +				       SECCertUsage certusage,
> +				       const SECItem *detached_digest,
> +				       HASH_HashType digest_type,
> +				       PRBool keepcerts,
> +				       PRInt64 atTime)

s/PRInt64/PRTime/

If the caller wants to have the "signing time or now" semantics, then they can call SEC_PKCS7VerifyDetachedSignature. NSS functions that take time arguments generally default them to PR_Now() when they are zero. This function should do the same, so that we always ignore the signing time when this function is called.

One way of doing that is to insert:

  if (!atTime) {
      atTime = PR_Now();
  }

I will make this change, and update the documentation for this function, when I check in the code.
Attachment #706002 - Flags: review?(bsmith) → review+
<relyea> I was just looking at the header to make sure it was explicit about the fact that NULL time was the signing time of the jar, not PR_Now() as many users may assume. Changing to what most users would expect would be fine. Make sure you make it clear that passing NULL is *not* the same as the SEC_PKCS7VerifyDetachedSignature(), though.
<relyea> Go ahead and put this discussion in the bug.
(In reply to Jason Smith [:jsmith] from comment #1)
> Someone will ask this question in triage, so I'll ask it:
> 
> Can you explain why this has to block a bit more? It's a bit unclear from
> reading this right now.

Though we're well on our way to having the key management problems solved (how we protect the keys that sign apps on the servers), in order to reduce risk and in order to give the marketplace and ops teams as much flexibility as possible, we want to be able to use one or two short-lived End-entity certificates that chain to the final trusted roots, during the next month or so. In order to be confident that those temporary EE certs have no effect on the security of the marketplace after B2G 1.0 ships, we need to make sure we enforce the expiration time of those certs. This patch is what enables us to do that.
Whiteboard: [bsmith is going to modify the patch and check it in]
Comment on attachment 706002 [details] [diff] [review]
V2, with feedback comments included

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

After looking at the patch more closely, I see some issues that should be studied more carefully:

* In the case where the requested usage is certUsageEmailSigner or certUsageEmailRecipient, we save an S/MIME profile with the signing time, even if the caller passed a different time. Does this make sense or not? I think we should avoid parsing the signing time and avoid creating an S/MIME profile when atTime is given. We would have to call out in the documentation for each function whether it deals with the signing time or S/MIME profile stuff or not.

* Should we attempt to verify that the signingTime is sane in some way? If not, we should add a note about this to the documentation.

Also, there is an XXX comment about the use of the signing time saying that we should let the application pass the signing time if it wants to. Since we're doing that with this patch, we should remove that comment.

I'm testing out a patch that makes these changes now.
Attachment #706002 - Flags: review+ → review?(bsmith)
(In reply to Brian Smith (:bsmith) from comment #11)
> Comment on attachment 706002 [details] [diff] [review]
> V2, with feedback comments included
> 
> Review of attachment 706002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After looking at the patch more closely, I see some issues that should be
> studied more carefully:
> 
> * In the case where the requested usage is certUsageEmailSigner or
> certUsageEmailRecipient, we save an S/MIME profile with the signing time,
> even if the caller passed a different time. Does this make sense or not? I
> think we should avoid parsing the signing time and avoid creating an S/MIME
> profile when atTime is given. We would have to call out in the documentation
> for each function whether it deals with the signing time or S/MIME profile
> stuff or not.

The atTime is used only for the verification of the certificate, as it should. It should not affect at all what dates are returned on some other places. In fact, if the certificate is valid, we can still trust (for some value of trust ;) ) the date included in the signature. So, in short, I think the behavior was correct on Carmen's patch (even if I still think it would have been better just to verify the certificate ALWAYS on the current time): use the provided time to verify if the certificate (and the signature) is valid, and if it's valid use whatever time is provided on the signature for the rest of the purposes. 

> 
> * Should we attempt to verify that the signingTime is sane in some way? If
> not, we should add a note about this to the documentation.

The one included on the signature? We could verify if it falls on the certificate validity period, and that the signing time is before now. Then again, I would have thought that was already done, and if it isn't, again it should be done for ALL use cases not just this one :).
I made the following changes to the patch:

* I added a corresponding SEC_PKCS7VerifySignatureAtTime function.
* I changed the *AtTime functions such that they never deal with the signing time or the S/MIME profile stuff at all.
* I updated the metadata in smime.def to export both new functions for the NSS 3.14.2 release (the previous patch put them in the NSS 3.13.1 section).
* Marked the older functions DEPRECATED.
* Rewrote the documentation for all SEC_PKCS7Verify* functions in the header file, and removed the duplicated documentation in p7decode.c, so that we don't have to update both sets of documentation in the future.
* Changed the indention to match NSS's and Gecko's conventions consistently.
* Added the notes in security/patches that are required for private patches to NSS.

Antonio wrote:
> The atTime is used only for the verification of the certificate, as it
> should. It should not affect at all what dates are returned on some other
> places. In fact, if the certificate is valid, we can still trust (for some
> value of trust ;) ) the date included in the signature. So, in short, I
> think the behavior was correct on Carmen's patch (even if I still think it
> would have been better just to verify the certificate ALWAYS on the current
> time): use the provided time to verify if the certificate (and the
> signature) is valid, and if it's valid use whatever time is provided on the
> signature for the rest of the purposes. 

I mostly agree. However, I noticed that in the case where the certificate validation fails, we still update the S/MIME profile information for the email addresses in the certificate. That seems very wrong to me; if certificate validation failed then we shouldn't be saving *any* information from this message into the token. My concern is that this may cause some applications (including Thunderbird?) to suggest the use of an attacker's certificate when encrypting email to a recipient, even when the attacker's certificate is untrusted and even when the recipient already had good information from a message signed with a trusted certificate. Although this doesn't affect the functionality that B2G needs, I would rather fix this now so that we don't end up creating yet another function that is doing dangerous things.

By the way, I do also agree that we should change the old functions (which I marked deprecated) to use PR_Now() instead of the signing time for validation, and also change them so that they only save the S/MIME profile if the certificate validation succeeded. However, I am pretty sure that these mus-behaviors are intentional design decisions made to support Thunderbird and other email applications. If this patch gets r+d then I will file the bugs to fix Thunderbird, and if/when that work gets completed in a way that demonstrates that it is reasonable to change the behavior of the current functions, then we can make the change to the existing functions' behavior. But, I am not sure if that will happen or when it will happen.
Attachment #706314 - Flags: review?(rrelyea)
Attachment #706314 - Flags: feedback?(amac)
(In reply to Brian Smith (:bsmith) from comment #13)
> I made the following changes to the patch:

In addition to the changes listed in the previous comment, I also made the change mentioned in comment 8 so that PR_Now() is used as the default value when 0 as atTime for the new functions.
Bob, it is important that we land this change (either my patch that I requested you to review, or Carmen's) for the B2G branch ASAP. However, I saw Kai's email on nss-dev that said that we're not supposed to land new features on the NSS trunk until the NSS 3.14.2 release is done. Should I hold off on landing this on the NSS trunk? If so, I will need to land it as a private patch on mozilla-central as well as the mozilla-b2g18 branch.
Comment on attachment 706314 [details] [diff] [review]
Alternate patch that avoids signing time and S/MIME profiles completely for the new functions

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

I don't think the S/MIME profile is saved at all if the certificate validation fails... The function could use some refactoring (almost 500 lines!) but at the start it has:
1309 goodsig = PR_FALSE;

And then if the certificate validation fails, it does a goto savecert, skipping everywhere else than the goodsig variable is modified. Then in savecert it does:

1701  if (goodsig && (signerinfo->authAttr != NULL)) {
        /*
         * If the signature is good, then we can save the S/MIME profile,
         * if we have one.
         */
        SEC_PKCS7Attribute *attr;

        attr = sec_PKCS7FindAttribute (signerinfo->authAttr,
                                       SEC_OID_PKCS9_SMIME_CAPABILITIES,
                                       PR_TRUE);
        profile = sec_PKCS7AttributeValue (attr);
      }

so if the certificate validation fails then profile is NULL and so it isn't saved or added:

https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certdb/stanpcertdb.c#942

968     if (cert->slot && cert->isperm && CERT_IsUserCert(cert) &&
969 	(!emailProfile || !emailProfile->len)) {
970 	/* Don't clobber emailProfile for user certs. */
971     	return SECSuccess;
972     }

In short, I don't think this extra modification is needed. If anything you could move the goodsig validation to 

1689    if ( cert->emailAddr && cert->emailAddr[0] &&
             ( ( certusage == certUsageEmailSigner ) ||
               ( certusage == certUsageEmailRecipient ) ) ) {

so we don't do any extra steps if the signature failed. But other than using the signature date to check the certificate I think the current behavior is correct.
Comment on attachment 706314 [details] [diff] [review]
Alternate patch that avoids signing time and S/MIME profiles completely for the new functions

I will address the additional issues I fixed in this patch in a new bug.
Attachment #706314 - Flags: review?(rrelyea)
Attachment #706314 - Flags: feedback?(amac)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc89c4fedd7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9a14853c74e9

Carmen, Mercurial on Windows butchered your name when I pushed the patch to mozilla-inbound. Sorry about that. I corrected it for the mozilla-b2g18 version by pushing it in a Linux VM. If you have any idea how to get Mercurial on Windows to do the right thing with non-ASCII usernames, please let me know by email.

I made the following changes to Carmen's patch:
1. Made whitespace and line length consistent with Gecko/NSS rules.
2. Changed the handling of atTime == 0 to always default to PR_Now(), ignoring the signing time.
3. s/PRInt64/PRTime/
4. I updated the metadata in smime.def to refer to NSS 3.14.2 instead of NSS 3.13.1.
5. Added the notes in security/patches that are required for private patches to NSS.

I did not make the other changes from my alternate patch. I will do that, plus the work that enables us to use an alternate certificate verification function, in bug 808224.
Whiteboard: [bsmith is going to modify the patch and check it in]
Attachment #706314 - Attachment is obsolete: true
Comment on attachment 706002 [details] [diff] [review]
V2, with feedback comments included

r+ with the modifications that I made.
Attachment #706002 - Flags: review?(bsmith) → review+
(In reply to Antonio Manuel Amaya Calvo from comment #12)
> > * Should we attempt to verify that the signingTime is sane in some way? If
> > not, we should add a note about this to the documentation.
> 
> The one included on the signature? We could verify if it falls on the
> certificate validity period, and that the signing time is before now. Then
> again, I would have thought that was already done, and if it isn't, again it
> should be done for ALL use cases not just this one :).

FYI, I checked the spec and it doesn't seem to have any requirements that the signingTime be valid in any way. The validation of signingTime seems to be part of the application-specific security policy, so I don't think we should do anything with it.
(In reply to Brian Smith (:bsmith) from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc89c4fedd7
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/9a14853c74e9
> 
> Carmen, Mercurial on Windows butchered your name when I pushed the patch to
> mozilla-inbound. Sorry about that. I corrected it for the mozilla-b2g18
> version by pushing it in a Linux VM. If you have any idea how to get
> Mercurial on Windows to do the right thing with non-ASCII usernames, please
> let me know by email
> 
Thanks for fixing it!
Sadly I've only used Mercurial on Linux. Still, I know of an easy way to fix it: just remove the accent from the é and leave a plain e ;)
In fact I'll change my Mercurial configuration so this doesn't happen again (I thought it was on plain ascii already)
https://hg.mozilla.org/mozilla-central/rev/3bc89c4fedd7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
[From email:]
> I see that this bug is marked "tef+", but the bugfix is not landed on
> the new v1.0.0 branch. Is this intentional or will this bugfix soon
> land on the new v1.0.0 branch?

I don't know anything about the v1.0.0 branch and I haven't seen any notification that I am/was supposed to land patches anywhere other than mozilla-b2g18. But, yes, it is supposed to be in 1.0 so I suppose it must be in the 1.0.0 branch.

> Note: I ask because there's something about the commit message for
> your bugfix which is breaking our hg->git conversions. We're putting
> workaround in place for now, to unbork the replications for other
> bugfixes for v1.0.0, but if your patch *needs* to be on v1.0.0,
> please let us know before landing, while we debug this.

Apparently the fix for the character encoding issue I mentioned in comment 18 was bad. I wonder how Fabrice gets the non-ASCII characters in his commit messages to work correctly.
tracking-b2g18: --- → ?
No need to track for b2g18. This is already tef+ - you've got power to land on each branch already.
tracking-b2g18: ? → ---
Why does this patch claim that your new function was added to very old NSS version 3.13.1 ?

If this is a last-minute fix for B2G, why are you landing it into mozilla-inbound/mozilla-central at all?

By changing mozilla-central to require an NSS API that isn't available in any upstream NSS branch nor NSS release, you make it impossible for anyone building mozilla-central / Firefox 21 with separate NSS.

I see that you filed bug 808224, but is that enough to be in line with the rules? Adding a new API to mozilla-central, that hasn't been reviewed upstream by NSS, from my point of view that's a fork.

Please don't fork NSS in mozilla desktop branches. Please play by the rules and get NSS to accept your new API first, before you use it on mozilla desktop branches.

And what worries me even more, you don't even seem to intend to work on inclusion of exactly this API in upstream NSS, instead you say in bug 808224 that you intend to do future work to create a modified version of this API.

By making life easier for you (in b2g) you make life much harder for anyone in the Linux that start their work early by building early versions of the mozilla-central/mozilla-aurora/mozilla-beta branches.

I would like to see a strict promise (including approval flags) that you clean this up prior to the next Mozilla branch date, prior to this change transitioning to mozilla-aurora.

Honestly, I believe you should follow the procedure we always used in the past. Get your change reviewed and accepted as a new API, check it in to NSS, and wait for at least a NSS beta tag before picking up a new API in mozilla desktop code.
IIUC, this will need to go on the b2g18_v1_0_0 branch as well (pending comment #25 anyway)
Comment on attachment 706002 [details] [diff] [review]
V2, with feedback comments included

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

::: security/nss/lib/pkcs7/secpkcs7.h
@@ +136,5 @@
>  
> +
> +/*
> + * SEC_PKCS7VerifyDetachedSignatureAtTime
> + *      Does the same function than SEC_PKCS7VerifyDetachedSignature.

"than" => "as"?

@@ +139,5 @@
> + * SEC_PKCS7VerifyDetachedSignatureAtTime
> + *      Does the same function than SEC_PKCS7VerifyDetachedSignature.
> + *	The verification checks that the signing cert is valid at the atTime
> + *      time if nonzero or at the signing time if the pkcs7 has a signing time or
> + *      now otherwise.

This comment needs work. It may be useful to write the three cases
clearly.

I don't know what "or now otherwise" refers to because there are two
if's in this sentence.

Also, note that 0 is a valid value for PRTime. It means the Unix
epoch (1970-01-01 00:00:00 GMT). I think it is better for this
function to just use atTime as specified without such complicated
logic.

Note: I just noticed bsmith suggested the same thing.

@@ +146,5 @@
> +						     SECCertUsage certusage,
> +						     const SECItem *detached_digest,
> +						     HASH_HashType digest_type,
> +						     PRBool keepcerts,
> +						     PRInt64 atTime);

Include "prtime.h" instead of "nspr.h", and use PRTime instead of PRInt64 here.

::: security/nss/lib/smime/smime.def
@@ +266,5 @@
>  NSSSMIME_GetVersion;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSS_3.13.1 {    # NSS 3.13.1 release

As Kai pointed out, this should say NSS_3.14.3.
(In reply to Wan-Teh Chang from comment #27)

Wan-teh, please look at the version of the patch that I checked in, not the version attached to this bug. I fixed several issues before I checked it in.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9a14853c74e9

> Also, note that 0 is a valid value for PRTime. It means the Unix
> epoch (1970-01-01 00:00:00 GMT). I think it is better for this
> function to just use atTime as specified without such complicated
> logic.

I agree. (IIRC, there are other functions that accept optional times and default them to PRTime.) I will make this change in bug 808224, which is the bug for upstreaming the new API to NSS.

> Include "prtime.h" instead of "nspr.h", and use PRTime instead of PRInt64
> here.

I removed the include before checking in the patch.

> As Kai pointed out, this should say NSS_3.14.3.

I changed it to NSS_3.14.2 before checking it in. In bug 808224, I will update it to say NSS_3.14.3.

BTW, I didn't land this change on the CVS trunk because (a) According to nss-dev, we're not supposed to add new features to NSS 3.14.2, and (b) I am going to change this function's semantics, and its signature significantly in bug 808224. If others think it is important that we have this patch on the NSS CVS HEAD, then we should create a branch for NSS 3.14.2 first, because this temporary function shouldn't make it to any actual NSS release.
As mentioned in comment 26, we're still needing this on mozilla-b2g18_v1_0_0 branch to fulfill the tef+ block here.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/dc225398a795

Needs to be landed on b2g18-v1.0.1 but I don't see any 1.0.1-specific flags, so marking tracking-b2g18?.
mozilla-b2g18 = mozilla-b2g18-v1.0.1 (the v1_0_1 branch is only being used by releng for testing atm)
Blocks: 842856
Depends on: 856285
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: