683883 - Improve DigitNotarGate handling in PSM

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

This is a follow-up bug to the PSM level changes done in bug 682927.

See bug 682927 comment 102 (and surrounding comments) for information why this is needed.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

With Bug 683261, NSS checks for known DigiNotar certs, and will report them  as untrusted.

This patch attempts to do the following in addition, but note, this is done ONLY FOR SSL connections, not for other uses of certs.

if NSS said "good",
  then call PSM_SSL_BlacklistDigiNotar

else == NSS said "bad or knockout cert"
  then call PSM_SSL_DigiNotarErrorUpgradeToRevoked


PSM_SSL_DigiNotarErrorUpgradeToRevoked
--------------------------------------
We look at all certs of the chain.
Two conditions must be true:
- at least one cert in the chain must be issued by "CN=DigiNotar Root CA"
- at least one of the certs in the chain must be issued >= 2011-07-01

If both conditions are true, then we upgrade the error to "revoked".


PSM_SSL_BlacklistDigiNotar
--------------------------
Remember this is only called after NSS decides the chain looks good - and was not knocked out.
We look at all certs of the chain.
If the bottom-most entry in the chain is one of the two whitelisted Dutch gov. roots, then we continue with the good state from NSS.

Else, and at least one cert in the chain was issued by someone named like "CN=DigiNotar",
then we'll change NSS's good decision into a "bad cert" decision.

We'll say at least "untrusted". However, in this case we'll also call into PSM_SSL_DigiNotarErrorUpgradeToRevoked, and potentially upgrade the error to revoked.


I believe this implements what was originally intended by bug 682927.

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 557489 [details] [diff] [review]
Patch v1

this needs some fixes

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Only comments have changed in this updated patch.

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 557490 [details] [diff] [review]
Patch v2

>+// Call this if a cert has been reported by NSS as INVALID

-> // Call this only if a cert has been reported by NSS as INVALID

>+PRErrorCode
>+PSM_SSL_DigiNotarErrorUpgradeToRevoked(CERTCertificate * serverCert,
>+                                       CERTCertList * serverCertChain)
>+{
>+  // If cert was issued by DigiNotar, 
>+  // and any involved cert was issued after 01-JULL-2011,

Nit: JUL

>+    if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) {

Are we sure this is going to catch everything, including certs chaining up via cross-signing, which do not chain up to the DigiNotar Root CA?

>+// Call this if a cert has been reported by NSS as VALID

Again, add the "only".

>+    // By request of the Dutch government

Please change this comment; it's misleading people. I suggest simply removing it.

Gerv

Comment 5

[Kai Engert [:KaiE:]]

Attachment: patch v3

(In reply to Gervase Markham [:gerv] from comment #4)
> 
> >+// Call this if a cert has been reported by NSS as INVALID
> 
> -> // Call this only if a cert has been reported by NSS as INVALID

Actually, this function is called from two places.
Now I think the following would be a clearer comment:

// Call this if we have already decided that a cert should be treated as INVALID,
// in order to check if we to upgrade the error to REVOKED.


> >+  // and any involved cert was issued after 01-JULL-2011,
> 
> Nit: JUL

ok fixed.



> >+    if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) {
> 
> Are we sure this is going to catch everything, including certs chaining up
> via cross-signing, which do not chain up to the DigiNotar Root CA?

No, this doesn't catch everything.
This is my attempt to do the same as we had initially intended.
I'd be OK to extend this to any CA certs from DigiNotar, including the cross-signed intermediates.

If we want to do so (still excluding Dutch Gov.),
we can shorten the comparison string to "CN=DigiNotar"

Because of your question, I presume you're in favor of this, so I'll make this change.


> >+// Call this if a cert has been reported by NSS as VALID
> 
> Again, add the "only".

Ok, added.


> >+    // By request of the Dutch government
> 
> Please change this comment; it's misleading people. I suggest simply
> removing it.

removed

Comment 6

[Robert Relyea]

> >+    if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) {

> Are we sure this is going to catch everything, including certs chaining up via
> cross-signing, which do not chain up to the DigiNotar Root CA?

The stated goal of the original patch was to do this test for cert only under the DigiNotar Root CA. This code should work even in the presence of cross-signed certs for the Root CA. It would not elevate certs issued after July 1 in the other DigiNotar intermediates.

I'm OK with either, as long as we know what we are trying to elevate.

Comment 7

[Robert Relyea]

Comment on attachment 557506 [details] [diff] [review]
patch v3

r+ rrelyea..

Two optiona changes:


+    if (CERT_GetCertTimes(serverCert, &notBefore, &notAfter) != SECSuccess ||
+        notBefore >= cutoff) {
+      onOrAfter20110701 = PR_TRUE;
+    }

serverCert is passed in, and isn't changed by the loop, so you can simply check it outside the loop. If it's not before the cutoff, you can simply return 0 without looking at the loop.

This means you can return SEC_ERROR_REVOKED_CERTIFICATE inside the loop, since you can only get to the loop if the cert has been issued after Jul 1.

My original description was assuming you only had the chain and you didn't know the chain order.

+        CERT_LIST_END(CERT_LIST_NEXT(node), serverCertChain)) {

This appears to be verifying that one of the Dutch CA's is the root cert that issued this chain. The CERT_LIST_END() check is correct only if serverCertChain is ordered leaf to root. I believe that is the case, but a less brittle check would be to see if the cert is selfsigned SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject)


NOTE: This patch implements the semantic that any certificate in the DigiNotar infrastructure issued after Jul 1 would have the elevated error code. That's a mozilla policy decision whether that is the correct semantic.


bob

Comment 8

[Kai Engert [:KaiE:]]

OK, I'll attach an updated patch in a couple of minutes.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v4

This patch addresses Bob's proposed changes.

Comment 10

[Robert Relyea]

Comment on attachment 557592 [details] [diff] [review]
Patch v4

r+ rrelyea

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 557592 [details] [diff] [review]
Patch v4

Asking for additional feedback, as suggested during NSS conf call.

Comment 12

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #7)
> 
> NOTE: This patch implements the semantic that any certificate in the
> DigiNotar infrastructure issued after Jul 1 would have the elevated error
> code. That's a mozilla policy decision whether that is the correct semantic.


Yes, any certificate in the DigiNotar infrastructure, except the ones that the Dutch Government requested to exclude.

The code will change all such certificates, issued July 1 or afterwards, to be reported as "revoked", not overridable by the user.

Our earlier patch had already done this for everything chaining up to the "DigiNotar Root CA". We know that Verizon and Entrust have revoked the respective intermediates.

I think we want to treat certs from any potentially existing, but not yet reported intermediates, in the very same way. (with the exception of Staat der Nederlanden).

I believe Mozilla wants this behaviour.
That's how I understand Gerv's question in comment 4.

Comment 13

[Paul van Brouwershaven]

(In reply to Kai Engert (:kaie) from comment #12)
> I think we want to treat certs from any potentially existing, but not yet
> reported intermediates, in the very same way. (with the exception of Staat
> der Nederlanden).

I think a good idea to hold this patch until a final decision by the Dutch government or to review the statement about to keep the DigiNotar intermediates under the root of Staat der Nederlanden.

The Dutch government recently advised in preparation for the possible outcomes of the Fox-IT report to "Identify what the consequences are if PKIoverheid DigiNotar certificates can not be trusted."

It looks like they tried (with success) to keep the DigiNotar PKI Overheid root online as long as possible. This e-mail confirms that they are definitely not sure that the intermediate root is not compromised.

http://dewinter.com/2011/09/01/logius-adviseert-om-gevolgen-uitval-pki-overheidscertificaten-te-benoemen/

Comment 14

[Kai Engert [:KaiE:]]

Paul, please don't misunderstand this patch.

This patch is a code fix. It introduces better/stricter handling for everything except the Dutch Government CAs.

The handling for the Dutch Gov certs was added elsewhere. As you can see, this patch simply moves that code around, for code correctness purposes.

I don't see a need to hold this patch, because it doesn't change the current handling of the Dutch Government CA certificates.

Comment 15

[Kai Engert [:KaiE:]]

Note to people who will land the patch in this bug. You may have to revert the patch from bug 683449, to make the patch in this bug apply cleanly.

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 557592 [details] [diff] [review]
Patch v4

Just to be prepared, if Mozilla should decide to stop excluding the PKIoverheid CAs from the blacklist, I would land a modified patch, that excludes the following block of code.

>+    // If it's issued by one of the "Staat der Nederlanden Root"s,
>+    // then don't blacklist. Compare names, and ensure it's a self-signed root.
>+    if ((!strcmp(node->cert->issuerName,
>+                "CN=Staat der Nederlanden Root CA,O=Staat der Nederlanden,C=NL") ||
>+         !strcmp(node->cert->issuerName,
>+                "CN=Staat der Nederlanden Root CA - G2,O=Staat der Nederlanden,C=NL")) &&
>+        SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject)
>+        ) {
>+      // keep as valid
>+      return 0;
>+    }

Bob, if needed, would you be OK with this modification?

Comment 17

[Robert Relyea]

> Bob, if needed, would you be OK with this modification?

Yes, sounds good.

This will definately stop whitelisting the Overheid certificates.

bob

Comment 18

[Wan-Teh Chang]

Comment on attachment 557592 [details] [diff] [review]
Patch v4

r=wtc.  I suggest some changes.

>+// Call this if we have already decided that a cert should be treated as INVALID,
>+// in order to check if we to upgrade the error to REVOKED.

Nit: I find "upgrade the error to REVOKED" a little hard to understand.
"Upgrade" usually means upgrading to something better, such as from
economy class to business class.  But REVOKED is the worse certificate
error.

>+  // If cert was issued by DigiNotar,
>+  // and any involved cert was issued after 01-JUL-2011,

IMPORTANT: This comment does not match your code.  To match your code,
the comment should say:

  // If any involved cert was issued by DigiNotar,
  // and serverCert was issued after 01-JUL-2011,
 
>+  if (status != PR_SUCCESS) {
>+    NS_ASSERTION(status == PR_SUCCESS, "PR_ParseTimeString failed");
>+    // be safe, assume it's afterwards, keep going
>+  }
>+  else {

Nit: put '}' and 'else {' on the same line?

>+      // no upgrade for certs older than the cutoff date

Nit: change "older than" to "issued before".

>+    if (!node->cert->issuerName)
>+      continue;
>+    if (strstr(node->cert->issuerName, "CN=DigiNotar")) {
>+      return SEC_ERROR_REVOKED_CERTIFICATE;

Nit: you can combine these two if statements into one:

  if (node->cert->issuerName &&
      strstr(node->cert->issuerName, "CN=DigiNotar")) {
    return SEC_ERROR_REVOKED_CERTIFICATE;
  }

>+    // If it's issued by one of the "Staat der Nederlanden Root"s,
>+    // then don't blacklist. Compare names, and ensure it's a self-signed root.
>+    if ((!strcmp(node->cert->issuerName,
>+                "CN=Staat der Nederlanden Root CA,O=Staat der Nederlanden,C=NL") ||
>+         !strcmp(node->cert->issuerName,
>+                "CN=Staat der Nederlanden Root CA - G2,O=Staat der Nederlanden,C=NL")) &&
>+        SECITEM_ItemsAreEqual(&node->cert->derIssuer,&node->cert->derSubject)
>+        ) {
>+      // keep as valid
>+      return 0;
>+    }

The comment is slightly inaccurate because node->cert is one of
the roots, but the comment is talking about the issuer.  I would
change the comment to say:

  // If it's one of the "Staat der Nederlanden Root"s,
  // then don't blacklist. Compare names, and ensure it's a self-signed root.

i.e., remove "issued by".

>+    certList = CERT_GetCertChainFromCert(serverCert, PR_Now(), certUsageSSLCA);
>+    if (!certList) {
>+      rv = SECFailure;
>+    }
>+    else {

Nit: put '}' and 'else {' on the same line?

>+      PRErrorCode blacklistErrorCode;
>+      if (rv == SECSuccess) { // PSM_SSL_PKIX_AuthCertificate said "valid cert"
>+        blacklistErrorCode = PSM_SSL_BlacklistDigiNotar(serverCert, certList);
>+      } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert"
>+        blacklistErrorCode = PSM_SSL_DigiNotarErrorUpgradeToRevoked(serverCert, certList);
>+      }

IMPORTANT: in the 'else' case, you may need to save the original
error code, so that you can restore it if blacklistErrorCode is 0.
The reason is that some of the NSS functions called by
PSM_SSL_DigiNotarErrorUpgradeToRevoked internally may change
the error code.

You may be able to inspect PSM_SSL_DigiNotarErrorUpgradeToRevoked
to prove that it won't change the error code if it returns 0, but
it may be more robust to save the original error code.

Comment 19

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #18)
> 
> IMPORTANT: in the 'else' case, you may need to save the original
> error code, so that you can restore it if blacklistErrorCode is 0.
> The reason is that some of the NSS functions called by
> PSM_SSL_DigiNotarErrorUpgradeToRevoked internally may change
> the error code.

Thanks for catching this. I indeed just ran into this bug.

I will address your comments and attach an improved patch soon.

Comment 20

[Kai Engert [:KaiE:]]

Attachment: Patch v5

(In reply to Wan-Teh Chang from comment #18)
> 
> Nit: I find "upgrade the error to REVOKED" a little hard to understand.
> "Upgrade" usually means upgrading to something better, such as from
> economy class to business class.  But REVOKED is the worse certificate
> error.

Ok, I'm changing comments and variables to use "worsen" instead of upgrade.


> >+      } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert"
> >+        blacklistErrorCode = PSM_SSL_DigiNotarErrorUpgradeToRevoked(serverCert, certList);
> >+      }
> 
> IMPORTANT: in the 'else' case, you may need to save the original
> error code, so that you can restore it if blacklistErrorCode is 0.

I've changed it to 

  PRErrorCode revokedCode = PSM_SSL_DigiNotarTreatAsRevoked(serverCert, certList);
  if (revokedCode != 0)
    blacklistErrorCode = revokedCode;


This patch addresses all the changes that Wan-Teh requested, and I fixed the single bug (I have a test case for this), therefore I'm carrying forward the r+.

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 557831 [details] [diff] [review]
Patch v5

No, although my test case worked, I din't fix the bug correctly.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: Patch v6

Ok, most of this patch has r=rrelyea and r=wtc, except for the following piece:

+      PRErrorCode blacklistErrorCode;
+      if (rv == SECSuccess) { // PSM_SSL_PKIX_AuthCertificate said "valid cert"
+        blacklistErrorCode = PSM_SSL_BlacklistDigiNotar(serverCert, certList);
+      } else { // PSM_SSL_PKIX_AuthCertificate said "invalid cert"
+        PRErrorCode savedErrorCode = PORT_GetError();
+        // Check if we want to worsen the error code to "revoked".
+        blacklistErrorCode = PSM_SSL_DigiNotarTreatAsRevoked(serverCert, certList);
+        if (blacklistErrorCode == 0) {
+          // we don't worsen the code, let's keep the original error code from NSS
+          PORT_SetError(savedErrorCode);
+        }
+      }


Wan-Teh, could you please review this section, to make sure I fixed it in the way you prefer?

Thanks!

Comment 23

[LegNeato]

We'll wanted this landed everywhere once it is reviewed and tested:

* mozilla-central (default branch)
* releases/mozilla-aurora (default branch)
* releases/mozilla-beta (default branch)
* releases/mozilla-release (default branch)
* releases/mozilla-1.9.2 (default branch)

I can take care of transplanting to whatever relbranches we need. Thanks!

Comment 24

[Robert Relyea]

Comment on attachment 557837 [details] [diff] [review]
Patch v6

r+ rrelyea

Comment 25

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/db763ec38c6a
http://hg.mozilla.org/releases/mozilla-aurora/rev/23e0b7ab56dc
http://hg.mozilla.org/releases/mozilla-beta/rev/e5b13332a307
http://hg.mozilla.org/releases/mozilla-beta/rev/4e3c2f671ef2
http://hg.mozilla.org/releases/mozilla-release/rev/32c201b7f160
http://hg.mozilla.org/releases/mozilla-release/rev/0ca9edcca253
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/88a72e8554a7
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/49b5a69ceeeb

Comment 26

[Wan-Teh Chang]

Comment on attachment 557837 [details] [diff] [review]
Patch v6

r=wtc.

>+// Call this if we have already decided that a cert should be treated as INVALID,
>+// in order to check if we to worsen the error to REVOKED.

Nit: change "we to" to "we have to" or "we need to".

Nit: "worsen" is correct but sounds awkward.  I'll shut up now :-)

>     if (strstr(node->cert->issuerName, "CN=DigiNotar")) {
>       isDigiNotarIssuedCert = PR_TRUE;

Nit: Add a break statement.  You can break out of the 'for' loop as soon as you set
isDigiNotarIssuedCert to true.

Comment 27

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

QA would like to verify this behavior prior to signing off, but it's not 100% clear what we should be using as testcases and what the final behavior should be. Any hints would be very appreciated.

Comment 28

[Gervase Markham [:gerv]]

See bug 683261 for verification steps.

Gerv

Comment 29

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Hey, thanks tons for the followup. Sounds like we picked the right strategy (much of our test matrix are the URLs you pointed out) but the confirmation makes me breathe easier.

Comment 30

[juan becerra [:juanb]]

QA verififed this against 3.6.22(build2), 6.0.2(build2, and 7.0b4(build2) as well as the latest nightly. We used URLs that should now not be trusted as well as trusted ones and marked our results in the following spreadsheet:

https://docs.google.com/spreadsheet/ccc?key=0AnEJ45MnstYjdGVLc0hQV3M0RE1ENDlZOTJqX204ZXc&hl=en_US#gid=0

Comment 31

[juan becerra [:juanb]]

QA tracking for verification in Aurora.