Closed Bug 682927 Opened 13 years ago Closed 13 years ago

Dis-trust DigiNotar root certificate

Categories

(NSS :: CA Certificates Code, task)

task
Not set
blocker

Tracking

(firefox6+ .1-fixed, firefox7+ verified, firefox8+ verified, firefox9+ verified, blocking1.9.2 .21+, status1.9.2 .21-fixed)

VERIFIED FIXED
3.12.12
Tracking Status
firefox6 + .1-fixed
firefox7 + verified
firefox8 + verified
firefox9 + verified
blocking1.9.2 --- .21+
status1.9.2 --- .21-fixed

People

(Reporter: gerv, Assigned: briansmith)

References

Details

(Keywords: verified-beta, Whiteboard: [sg:vector-high][qa!])

Attachments

(7 files, 12 obsolete files)

6.00 KB, text/plain
Details
77.06 KB, patch
briansmith
: review+
dveditz
: feedback+
Details | Diff | Splinter Review
76.33 KB, patch
briansmith
: review+
dveditz
: feedback+
Details | Diff | Splinter Review
1.26 KB, patch
briansmith
: review+
dveditz
: feedback+
Details | Diff | Splinter Review
10.60 KB, patch
dveditz
: review+
KaiE
: review+
Details | Diff | Splinter Review
9.05 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
380.28 KB, application/pdf
Details
Reported by Adam Langley to security-group:

"It's just gone (very) public: http://pastebin.com/ff7Yg663"

Gerv
Certificate is in the CRL:

http://service.diginotar.nl/crl/public2025/latestCRL.crl

    Serial Number: 05E2E6A4CD09EA54D665B075FE22A256
        Revocation Date: Aug 29 16:59:03 2011 GMT
        CRL entry extensions:
            Invalidity Date: 
                Aug 29 16:58:47 2011 GMT

Revoked 2 hours ago. 

I assume therefore it's in OCSP.

Gerv
There are reports that this has been used to attack Iranian users in the wild: http://www.google.co.uk/support/forum/p/gmail/thread?tid=2da6158b094b225a&hl=en

We have contacted the CA but I don't have any other pertinent information about other certificates etc.
I will create the patch that removes the CA from the builtin cert database.

Do we also want to prevent users from adding exceptions for these certificates?
I think it is clear that we want to prevent exceptions for the specific *.google.com certificate, but I don't think we want to prevent exceptions for other sites which currently have a valid certificate from a CA we no longer trust.
We need an eventual patch landed on:

* mozilla-central (default branch)
* releases/mozilla-aurora (default branch)
* releases/mozilla-beta (default branch)
* releases/mozilla-1.9.2 (default branch)
* releases/mozilla-1.9.2 (GECKO19220_2011080310_RELBRANCH branch)
It's worth noting that DigiNotar appears to have been cross signed several times:

CN=DigiNotar PKIoverheid CA Overheid en Bedrijven by CN=Staat der Nederlanden Overheid CA

CN=DigiNotar Services 1024 CA/emailAddress=info@diginotar.nl by Entrust

CN=DigiNotar Cyber CA/emailAddress=info@diginotar.nl by CyberTrust

CN=DigiNotar Root CA/emailAddress=info@diginotar.nl by Entrust (twice).

So blocking by serial number or certificate hash may be ineffective.
> I think it is clear that we want to prevent exceptions for the specific *.google.com > certificate, but I don't think we want to prevent exceptions for other sites which
> currently have a valid certificate from a CA we no longer trust.

If we both remove digitnotar and add the *.google.com cert as untrusted, that should accomplish the desired the above requested effect. The user cans till go in and explicitly trust the *.google.com cert from the certificate manager, but it would require a fair amount of sophistication to do so (and presumably that user will know what they are doing).
Bob: can we just clarify that? You are saying that, even with the cross-signing, if we simply remove the DigiNotar root from the NSS store and add explicit distrust for *.google.com, it will make all DigiNotar certs untrusted by default (but overridable) except for *.google.com, which will be non-overridable? No need for a code patch?

Gerv
> It's worth noting that DigiNotar appears to have been cross signed several times:

ouch - These don't stop blocking the leaf, but it does 

> So blocking by serial number or certificate hash may be ineffective.

Brian's proposal was to remove the certificate. That wouldn't block the cert, only not trust it. To block the intermediates, we'd have to craft a bogus DigiNotar Intermediate and explicitly not trust it. NSS would find the bogus and prefer it either the root or any of the intermediates.

If we had the code I've been waiting on a review for for several months in (and were using libpkix), we could simply mark the root as being untrusted and that would block everything.

bob
It isn't clear to what extent we want to revoke trust on DigiNotar. This patch is the nuclear option--it basically revokes all trust in DigiNotar-issued certificates, regardless of who the root CA is, and there's no user override possible.
Assignee: nobody → bsmith
Attachment #556670 - Attachment description: Blacklist all Diginotar-issued certificates based on "CN=Diginotar " in issuer → Blacklist all Diginotar-issued certificates based on "CN=DigiNotar " in issuer
DigiNotar is complex. There's the root, which can be removed, but there are sites which are "Entrust -> DigiNotar Services 1024 CA -> leaf" (www.zorgwebtwente.nl) and  "Staat der Nederlanden Root CA -> Staat der Nederlanden Overheid CA -> DigiNotar PKIoverheid CA Overheid en Bedrijven -> leaf" (https://www.wageningen.nl/).

And there are probably others. Explicit banning will be needed to knock all these paths out.
> Bob: can we just clarify that? You are saying that, even with the cross-signing,
>  if we simply remove the DigiNotar root from the NSS store and add explicit
> distrust for *.google.com, it will make all DigiNotar certs untrusted by default
> (but overridable) except for *.google.com, which will be non-overridable?

No, I just posted my response to Adam's comments... We'll have to craft a special fake intermediate.

> No need for a code patch?

No, we can still do it with the patch to builtins, but we will probably loose the ability to override SSL for other DigiNotar certs.
re comment 14.

Adam, All those intermediates need to have the same subject just before the leaf, otherwise we are effectively talking about different CA's (even if they are run from the same company). The leaf chains to it's intermediate through it's subject.
re #16

So, are you considering blocking just certificates that chain to the DigiNotar root, or (as Brian's patch does), anything which appears to be controlled by DigiNotar?
I wonder if Brian and Bob R should get on the phone and work this through?
(In reply to Johnathan Nightingale [:johnath] from comment #20)
> I wonder if Brian and Bob R should get on the phone and work this through?

We are on the phone already.
We (Google) have decided to take action against DigiNotar as well. I'm currently waiting to see what you decide to do about the root vs intermediates issue.
QA will probably need a hint as to how to quickly test this fix once it's landed, if at all possible. It'll save time if that comes in with the patch.
Adam Langley wrote:
> [intermediates of the DigiNotar root:]
>    CN=DigiNotar Extended Validation
>    CN=DigiNotar Services
>    CN=DigiNotar Public CA 2025/emailAddress=info@diginotar.nl
>    CN=DigiNotar Qualified
>    CN=Nederlandse Orde van Advocaten - Dutch Bar Association
>
> Issued from the intermediates:
> 
> Extended Validation: 33
> Services: 150
> Public: 0
> Qualifed: 0
Here is a patch that is less strict than the previous patch. It lets the user add a certificate override for DigiNotar-issued certificates, that are otherwise valid, and that have an notBefore date prior to July 1, 2011. It prevents the user from adding a certificate override for any DigiNotar-issued certificates with a notBefore date after July 1, 2011, which would include the *.google.com certificate.
Comment on attachment 556716 [details] [diff] [review]
Less strict blacklisting of all Diginotar-issued certificates based on "CN=Diginotar " in issuer (v2)

>+SECErrorCodes
>+PSM_SSL_BlacklistDigiNotar(CERTCertificate * serverCert)
...
>+  return static_cast<SECErrorCodes>(0);
>+}

Nit: this function should return either int or PRErrorCode
(which is a typedef of PRInt32).

This allows you to remove the static_cast at the end of the
function.

>+extern SECErrorCodes PSM_SSL_BlacklistDigiNotar(CERTCertificate * serverCert);

There should be a header where you can declare this function.
Perhaps Kai can suggest one.
Thanks for the review comments Wan-Teh.

Important notes:

(1) My patches are conservative about matching the DN of the issuer, by requiring the CN of the issuing cert starting with "DigiNotar". It might make more sense to block issuers where the name contains DigiNotar anywhere within the DN, or anywhere within the CN= or O= fields. With my current patch, a CN such as "New DigiNotar ..." will not patch.

(2) My patches only check the CN of the immediate issuer of the EE cert. DigiNotar could still issue certificates by inserting an intermediate that doesn't have a "DigiNotar" prefix in the CN.
Brian: you are also going to remove the root certificate from
certdata.c, aren't you?
I was just going to ask about scenario (1)... Your patch doesn't actually remove or distrust the DigiNotar root itself (no patch to certdata) and we don't validate roots: wouldn't it be trivial for DigiNotar to issue themselves a new intermediate with a different CN= string?

1) We should remove or distrust the DigiNotar root explicitly in certdata.

2) We should block the intermediates they are currently using that are cross-signed or chain to other roots. I'm relatively OK with the "CN=DigiNotar" string match if their own root is gone. If other CAs issue them new intermediates we can talk to those CAs about it.

3) We should block those intermediates when found anywhere in the chain.
(In reply to Wan-Teh Chang from comment #30)
> Brian: you are also going to remove the root certificate from
> certdata.c, aren't you?

Yes.

(In reply to Daniel Veditz from comment #31)
> I was just going to ask about scenario (1)... Your patch doesn't actually
> remove or distrust the DigiNotar root itself (no patch to certdata) and we
> don't validate roots: wouldn't it be trivial for DigiNotar to issue
> themselves a new intermediate with a different CN= string?

Yes. But, unless all the cross-signed intermediate certificates they've ever been issued have a path length constraint of zero, they will be able to do the same from one of their cross-signed intermediate certificates.

> 1) We should remove or distrust the DigiNotar root explicitly in certdata.
> 
> 2) We should block the intermediates they are currently using that are
> cross-signed or chain to other roots. I'm relatively OK with the
> "CN=DigiNotar" string match if their own root is gone. If other CAs issue
> them new intermediates we can talk to those CAs about it.
> 
> 3) We should block those intermediates when found anywhere in the chain.

If we block "CN=DigiNotar" anywhere in the chain, then AFAICT this will subsume the removal of the root from the NSS certdata. This is what I am working on now.
This patch checks every cert in the chain. If any of the CAs in the chain has "CN=Diginotar" then we block the cert. If the cert is chained to any (intermediate or root) cert with "CN=Diginotar Root CA" and the cert was issued since July 1, 2011, we hard-block (no user override) it; otherwise we soft-block (user can override) it.
Attachment #556670 - Attachment is obsolete: true
Attachment #556716 - Attachment is obsolete: true
Attachment #556748 - Flags: review?(kaie)
Attachment #556748 - Flags: review?(dveditz)
Attachment #556748 - Flags: feedback?(wtc)
Attachment #556748 - Flags: feedback?(agl)
Here is my WIP on a weaker version of the previous patch, which only hard-blocks for known-bad certificates (in this case, just the Google one). Setting up testcase for it now.
(In reply to Christian Legnitto [:LegNeato] from comment #8)
> We need an eventual patch landed on:
> 
> * mozilla-central (default branch)
> * releases/mozilla-aurora (default branch)
> * releases/mozilla-beta (default branch)
> * releases/mozilla-1.9.2 (default branch)
> * releases/mozilla-1.9.2 (GECKO19220_2011080310_RELBRANCH branch)

Also:

* releases/mozilla-release (default branch)
* releases/mozilla-release (SEAMONKEY_2_3_RELBRANCH branch)
(In reply to Brian Smith (:bsmith) from comment #32)
> If we block "CN=DigiNotar" anywhere in the chain, then AFAICT this will
> subsume the removal of the root from the NSS certdata.

Maybe in practical effect, but there is symbolic importance in removing the root itself (as well as being a user-visible change).
Comment on attachment 556755 [details] [diff] [review]
Blacklist all Diginotar-issued certificates, considering the whole chain, Comodogate-ish hard block

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

I know you haven't asked for reviews on this one, but given that we don't know any details of the attack and the reasonable assumption that a successful hacker would want more than a single cert (cf. comodogate) this doesn't seem like a safe approach.
Attachment #556755 - Flags: feedback-
As gerv and reed mentioned earlier we should probably open up this bug...details are all over the web.
When searching for CN=DigiNotar, should we do a string comparison that ignores upper/lowercase, just to be sure?
Given this affect multiple validation pathes, given there appear to be multiple certs in the wild, I believe we cannot currently trust this CA. There are too many combinations, and producing code that covers each of the scenarios individually is difficult. We don't have the time, we need an emergency reaction, now, and potentially relax it later.

I propose to:
- remove the root CA cert from NSS
- change NSS cert verification functions
- in NSS, if we attempt to verify a cert, and the cert's name starts with DigiNotar,
  then NSS should abort the verification with error code REVOKED
Comment on attachment 556748 [details] [diff] [review]
Blacklist all Diginotar-issued certificates, considering the whole chain, date-based hard block

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

This looks like it accomplishes what we want so r=dveditz for that, but I don't know PSM details and made some assumptions that macros and such did what their names indicated. Needs that second r+ from Kai to cover that.

Also want the root removed from certdata

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1071,5 @@
> +
> +  if (isDigiNotarIssuedCert)
> +    return SEC_ERROR_UNTRUSTED_ISSUER; // user can override this
> +  else
> +    return 0; // No DigiNotor cert => carry on as normal

wouldn't it make more sense for this to be SECSuccess? If you change this also change the line below that checks the return value.
Attachment #556748 - Flags: review?(dveditz) → review+
(In reply to Daniel Veditz from comment #42)
> ::: security/manager/ssl/src/nsNSSCallbacks.cpp
> @@ +1071,5 @@
> > +
> > +  if (isDigiNotarIssuedCert)
> > +    return SEC_ERROR_UNTRUSTED_ISSUER; // user can override this
> > +  else
> > +    return 0; // No DigiNotor cert => carry on as normal
> 
> wouldn't it make more sense for this to be SECSuccess? If you change this
> also change the line below that checks the return value.

SECSuccess has type SECStatus, not PRErrorCode.
Is everybody in agreement, that removing the DigiNotar root from the trusted CA list in NSS is a common denominator and can be done immediately, whatever additional actions might be decided later?
I verified that Kai's patch to mozilla-central causes Firefox to stop trusting the DigiNotar root, by applying the patch and visiting https://www.diginotar.nl, resulting in error code sec_error_unknown_issuer.

I also verified that the certdata.c file in the patch is the result of "gmake -C security/nss/lib/ckfw generate".
Whiteboard: [sg:high] keep bug hidden for now
Comment on attachment 556779 [details] [diff] [review]
patch - remove diginotar from PSM's whitelist for EV

not a PSM peer, but I agree with this change (pseudo-r+)
Attachment #556779 - Flags: feedback+
Attachment #556775 - Flags: feedback+
Attachment #556774 - Flags: feedback+
Here is my personal proposal for immediate action.

We should immediately create releases of NSS and Mozilla that:
- remove the trust for the DigiNotar root (NSS patch)
- remove DigiNotar from the EV list

This, at least, gives all users chance to notice that there is something wrong.

Yes, I understand, it would be better to have that "make it impossible to override".
But obviously, at this time, we don't have the exact list of certs that should be included or included in the "do not allow to override".

I propose to start with the obvious common denominator now, and start release processes.

This gives us more time to think about additional cleanup, and additional hardening, and we can do these additional steps in a second round of Firedrill.
Who agrees with my comment 50?
Here is the updated patch that avoids blocking DigiNotar certs that chain to the Dutch government root CA, by request of the Dutch government.
Attachment #556748 - Attachment is obsolete: true
Attachment #556748 - Flags: review?(kaie)
Attachment #556748 - Flags: feedback?(wtc)
Attachment #556748 - Flags: feedback?(agl)
Attachment #556791 - Flags: review?(kaie)
Attachment #556791 - Flags: review?(dveditz)
Test sites for the previous patch:

https://diginotar.nl (chains to DigiNotar root CA through its EV intermediary, should be blocked)
https://zga-tag.zorggroep-almere.nl (chains to Entrust root, should be blocked)
https://zoek.officielebekendmakingen.nl	(chains to Dutch government root, should NOT be blocked)
(In reply to Kai Engert (:kaie) from comment #50)
> Yes, I understand, it would be better to have that "make it impossible to
> override".

I agree with you, except it must not be possible to add an exception for the known-bad *.google.com cert. Unless my patch is problematic, we should include it too. If my patch is problematic, let's fix it.
Summary: *.google.com certificate being abused in the wild → Dis-trust DigiNotar root certificate
Comment on attachment 556791 [details] [diff] [review]
[v2] Blacklist all DigiNotar-issued certificates, considering the whole chain, date-based hard block

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

Looks good to me, r=dveditz
Attachment #556791 - Flags: review?(dveditz) → review+
Attachment #556755 - Attachment is obsolete: true
Comment on attachment 556791 [details] [diff] [review]
[v2] Blacklist all DigiNotar-issued certificates, considering the whole chain, date-based hard block

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

LGTM
Comment on attachment 556791 [details] [diff] [review]
[v2] Blacklist all DigiNotar-issued certificates, considering the whole chain, date-based hard block

>+{
>+  PRBool isDigiNotarIssuedCert = PR_FALSE;
>+
>+  for (CERTCertListNode *node = CERT_LIST_HEAD(serverCertChain);
>+       !CERT_LIST_END(node, serverCertChain);
>+       node = CERT_LIST_NEXT(node)) {
>+    if (!node->cert->issuerName)

if you're paranoid, we could use
    if (!node->cert || !node->cert->issuerName)


>+      continue;
>+
>+    if (strstr(node->cert->issuerName, "CN=DigiNotar")) {
>+      isDigiNotarIssuedCert = PR_TRUE;
>+      // Do not let the user override the error if the cert was
>+      // chained from the "DigiNotar Root CA" cert and the cert was issued
>+      // within the time window in which we think the mis-issuance(s) occurred.

add a comment that explains why you use the time based cutoff for this subset only?


>+      if (strstr(node->cert->issuerName, "CN=DigiNotar Root CA")) {
>+        PRTime cutoff = 0, notBefore = 0, notAfter = 0;
>+        PRStatus status = PR_ParseTimeString("01-JUL-2011 00:00", PR_TRUE, &cutoff);
>+        NS_ASSERTION(status == PR_SUCCESS, "PR_ParseTimeString failed");
>+        if (status != PR_SUCCESS ||
>+           CERT_GetCertTimes(serverCert, &notBefore, &notAfter) != SECSuccess ||
>+           notBefore >= cutoff) {
>+          return SEC_ERROR_REVOKED_CERTIFICATE;
>+        }
>+      }
>+    }
>+


>+    // By request of the Dutch government
>+    if (!strcmp(node->cert->issuerName,
>+                "CN=Staat der Nederlanden Root CA,O=Staat der Nederlanden,C=NL") &&
>+        CERT_LIST_END(CERT_LIST_NEXT(node), serverCertChain)) {

At least add a comment that you assume that the list is ordered.

Add a comment that clarifies, the dutch exception only relates to intermediates,
which were issued by by this toplevel (Nederlanden) CA.

Remainder of the patch looks good.

I'd prefer if you addressed these nits before checking in, but if there is no time, you can do that later.
r=kaie


Please note your patch doesn't help against code signing certs or S/MIME certs,
it only covers S/MIME certs.

This means, this patch still requires the NSS patch to remove the trust, too.
I assume this patch is checked in together with the removal (or blocking) patch in NSS.


Now, after I have finished this review, I will attempt to do an alternative patch for the NSS removal - something similar to 471715, but I need to investigate how to do that.
Attachment #556791 - Flags: review?(kaie) → review+
I note (as does Kai in comment 58) that Brian's attachment 556791 [details] [diff] [review] doesn't include Kai's removals from NSS and PSM's EV list.

Kai or Brian:

 - can we clarify what the complete set of patches is that should land, assuming we go with the approach in Brian's patch? Can we produce a rollup patch to that effect to reduce the risk of mislandings?

 - can we clarify that Brian's latest patch allows overrides for certs other than the actual known broken one?

Kai - I know you're working on the alternate, but in case it isn't done in time, it would be very helpful to have your assistance in the hours before brian wakes up. (And thank you so much for your help thus far - you too Dan and Bob and Adam)
(In reply to Johnathan Nightingale [:johnath] from comment #59)
> I note (as does Kai in comment 58) that Brian's attachment 556791 [details] [diff] [review]
> [details] [review] doesn't include Kai's removals from NSS and PSM's EV list.

Yes, because NSS and Firefox/PSM live in different source code repositories.
I'm used to clarify which piece belongs into which bowl.


>  - can we clarify what the complete set of patches is that should land,
> assuming we go with the approach in Brian's patch?

For the stable branches of Mozilla, which are using NSS 3.12.x,
the answer is, the following 3 patches:

(1) attachment 556774 [details] [diff] [review]  [ NSS ]
                       (don't read the .c part,
                        it's automatically generated from the .txt part)

(2) attachment 556779 [details] [diff] [review]  [ PSM ]

(3) attachment 556791 [details] [diff] [review]  [ PSM ]


> Can we produce a rollup
> patch to that effect to reduce the risk of mislandings?

Sure.
I can do that.
How long are you willing to wait?

If you give me 30 minutes, I should be able to produce the alternative (1a).


>  - can we clarify that Brian's latest patch allows overrides for certs other
> than the actual known broken one?

Sorry, this question cannot be simply answered with a yes or no, because the logic is more complicated.

Also, I don't have a list of the known broken ones - so we cannot compare.

If the name of an issuer of a cert starts with "DigiNotar Root CA",
then {
  If the cert was issued on or after 01-JUL-2011 00:00,
  then
    { treat as revoked, no override possible }
  else
    { default to untrusted (error page), user can override }
} 
else { // toplevel DigiNotar root CA was not involved in creating a cert
  if any other CA with a name that starts with "DigiNotar" was involved,
  then {
    if the cert chains up to the Staat der Nederlanden Root CA
    then {
      skip all DigiNotar blacklisting, and just use all the other usual checks
    }
    else {
      treat as untrusted (error page), user can override
    }
  }
}


I hope I correctly translated Brian's patch into pseudo code.
I have a cert that is crafted equivalently to what we did for the rogue CA cert.
I'm working on the patch that adds it to NSS, instead of the old one.
Will be ready in 5 minutes.
Attached patch BAD (obsolete) — Splinter Review
This is my attempt to produce the equivalent of what we did in the past for the rogue md5 collision CA.

I binary edited the cert.
I changed the serial number to 0xfffffffffffff
I increased the validity times by one second,
so NSS will prefer these over the older one.

I haven't tested it.

I added this cert to NSS.
This patch removes the old root, and replaces it with my manually crafted root.

This patch is for the NSS 3.12 branch, so it should apply fine to all stable branches of Mozilla.
(does not apply to mozilla-central)

Unfrotunately I must board a plane in a couple of minutes.
I will attach multiple versions of the certs, FYI,
so can see what I did.
Attached file FYI - current (old) cert in NSS (obsolete) —
Attached file FYI - the override cert I created (obsolete) —
So, here's the current status.  We decided to take the existing fix on our branches, pending any possible alternate approach patch that Kai is working on right now.  I landed on mozilla-central and mozilla-aurora:

http://hg.mozilla.org/mozilla-central/rev/04a58ba1ce1e
http://hg.mozilla.org/mozilla-central/rev/365bd397fa9b
http://hg.mozilla.org/mozilla-central/rev/fcca99426576
http://hg.mozilla.org/releases/mozilla-aurora/rev/23a4e07cd174
http://hg.mozilla.org/releases/mozilla-aurora/rev/73dd41230d4e
http://hg.mozilla.org/releases/mozilla-aurora/rev/8587a9c1b25b

(I used the NSS 3.12 patch on Aurora)

But that patch does not apply cleanly on Beta:

ehsanakhgari:~/moz/beta [10:48:12]$ hg trans -s ../aurora/ 23a4e07cd174
searching for changes
filtering /var/folders/68/68h0xk+-GQaZP1NVs8kSmE+++TI/-Tmp-/hg-transplant-IU5cH6
applying 23a4e07cd174
patching file security/nss/lib/ckfw/builtins/certdata.c
Hunk #1 FAILED at 29
Hunk #2 FAILED at 1033
Hunk #71 FAILED at 16752
Hunk #137 FAILED at 21224
Hunk #138 FAILED at 21342
Hunk #139 FAILED at 21368
Hunk #140 FAILED at 21501
Hunk #141 FAILED at 21527
Hunk #142 FAILED at 21621
Hunk #143 FAILED at 21649
Hunk #144 FAILED at 21733
Hunk #145 FAILED at 22091
12 out of 145 hunks FAILED -- saving rejects to file security/nss/lib/ckfw/builtins/certdata.c.rej
patching file security/nss/lib/ckfw/builtins/nssckbi.h
Hunk #1 FAILED at 71
1 out of 1 hunks FAILED -- saving rejects to file security/nss/lib/ckfw/builtins/nssckbi.h.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue


According to security/nss/TAG-INFO, Aurora is using NSS_3_12_11_RTM, while Beta, Release are using NSS_3_12_10_RTM, and 1.9.2 is probably using a different NSS version (or one that doesn't include the aforementioned file at least).

I don't know how to get the NSS fix on Beta, Release and 1.9.2...
Attached file FYI - text dump of old cert (obsolete) —
Comment on attachment 556837 [details] [diff] [review]
BAD

argh I made a mistake
Attachment #556837 - Attachment description: alternative to 556774 - (1a) - override builtin diginotar with explicitly untrusted one → BAD
Attachment #556837 - Attachment is obsolete: true
I have triggered a local build with attachment 556837 [details] [diff] [review] to test the sites mentioned in comment 53.  Will post the results shortly.
Ehsan: I believe certdata.c is auto-generated, so perhaps you can rerun the auto-generation? That would deal with most of the conflict.

Gerv
(In reply to Gervase Markham [:gerv] from comment #69)
> Ehsan: I believe certdata.c is auto-generated, so perhaps you can rerun the
> auto-generation? That would deal with most of the conflict.
> 
> Gerv

Yep - we got there too. :) Ehsan's working on applying it across the board while we also test locally and then push.
Ok, I have another couple of minutes.

I believe the override cert I have created is correct.
I just forgot one piece when creating the patch for NSS.

This one should be right.
So, you have two choices:

- either you go with the set of patches, as in comment 60

- or you use 
    (1a) attachment 556845 [details] [diff] [review]
    (2) attachment 556779 [details] [diff] [review]
    (3) attachment 556791 [details] [diff] [review]

But because (1a) is untested, I understand if you don't want to take it now.
We took the set of patches in comment 60 across all branches.  Here are the additional landings on top of the ones in comment 65.

http://hg.mozilla.org/releases/mozilla-beta/rev/d0188c7a996b
http://hg.mozilla.org/releases/mozilla-beta/rev/ebe065dc7ce6
http://hg.mozilla.org/releases/mozilla-beta/rev/19d07c45a5ff
http://hg.mozilla.org/releases/mozilla-release/rev/b5f28acb61c0
http://hg.mozilla.org/releases/mozilla-release/rev/1cb931c6824a
http://hg.mozilla.org/releases/mozilla-release/rev/43636529bf9d
http://hg.mozilla.org/releases/mozilla-release/rev/078215b4d425 (SEAMONKEY_2_3_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-release/rev/d17210697973 (SEAMONKEY_2_3_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-release/rev/bb85f35f416a (SEAMONKEY_2_3_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f729fcf0eb50
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cbe67e60d8c0
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b01c24423221
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/581657ce604e (GECKO19220_2011080310_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c7cbd666461f (GECKO19220_2011080310_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6ef4cc2f67cf (GECKO19220_2011080310_RELBRANCH)

Also, I landed a header change that Kai requested as a follow-up:
http://hg.mozilla.org/releases/mozilla-beta/rev/edf2f673b882
http://hg.mozilla.org/releases/mozilla-release/rev/791d19e33b2a
http://hg.mozilla.org/releases/mozilla-release/rev/53192d5b30ef (SEAMONKEY_2_3_RELBRANCH)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e2829e84c046
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/064d4408b160 (GECKO19220_2011080310_RELBRANCH)

I had to modify Brian's patch for 1.9.2 in order to make it apply cleanly.  Will attach the patch right now for his review.
Oops, wrong patch
Attachment #556856 - Attachment is obsolete: true
Attachment #556856 - Flags: review?(bsmith)
Attachment #556857 - Flags: review?(bsmith)
Attachment #556857 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #53)
> Test sites for the previous patch:
> 
> https://diginotar.nl (chains to DigiNotar root CA through its EV
> intermediary, should be blocked)
> https://zga-tag.zorggroep-almere.nl (chains to Entrust root, should be
> blocked)
> https://zoek.officielebekendmakingen.nl	(chains to Dutch government root,
> should NOT be blocked)

In order to verify the fix, I just got a local build from mozilla-release and tested it against these websites, in addition to paypal and AMO, and all sites were working except for https://diginotar.nl and https://zga-tag.zorggroep-almere.nl.  When I added an exception for these two sites, I could successfully navigate to them.
(In reply to Kai Engert (:kaie) from comment #58)
> >+      // Do not let the user override the error if the cert was
> >+      // chained from the "DigiNotar Root CA" cert and the cert was issued
> >+      // within the time window in which we think the mis-issuance(s) occurred.
> 
> add a comment that explains why you use the time based cutoff for this
> subset only?

Didn't he do that? We use the time-based cutoff because the cert falls in "the window in which we think the mis-issuance(s) occurred". The function as a whole references this bug in case that's not enough detail.
(In reply to Ehsan Akhgari [:ehsan] from comment #77)
> In order to verify the fix [....] [A]ll sites were working except for
> https://diginotar.nl and https://zga-tag.zorggroep-almere.nl.  When I added
> an exception for these two sites, I could successfully navigate to them.

It'd be nice if we knew of a site with a cert issued after July 1. Failing that we'll have to do personal or try builds with an earlier hardcoded date to test the mechanism. bsmith said he did test that, but not on each of those branches.
Based on comment 65 and comment 74 this is fixed everywhere, right?

trunk (mozilla 9)
aurora (mozilla 8)
beta (mozilla 7)
release (mozilla 6.0.1)
release-relbranch (seamonkey 2.3.2)
1.9.2 (3.6.22)
1.9.2-relbranch (3.6.21)

The bug is in the NSS product, but changes weren't checked into the NSS CVS repository yet. Should we move the bug to "Core::Security:PSM" and close it, or leave it open for the NSS part?

Any alternate "better"/permanent solutions should happen in a new bug linked to this one.
blocking1.9.2: --- → .21+
Let's leave this bug open until Kai's patches to NSS can be reviewed by other NSS and landed on the NSS trunk.
Comment on attachment 556845 [details] [diff] [review]
Patch to NSS - fixed version of (1a)

>-# Certificate "DigiNotar Root CA"
>+# Certificate "Explicity Disable DigiNotar Root CA"

For a root CA certificate, it should suffice to simply
remove it from certdata.txt and certdata.c.  (See bug
622719 for an example.)  We only need to use the technique
in this patch to distrust an intermediate CA certificate.

Note: in bug 530853 you used another technique to distrust
a root CA certificate, by changing its trust flags.  That
should also work.
> We only need to use the technique
> in this patch to distrust an intermediate CA certificate.

Except this is another intermediate effectively. Entrust has signed 2 cross certs for digiNotar, so this is my preferred patch for NSS.... mostly because I need to turn off DigiNotar in other NSS clients besides mozilla.

bob
Comment on attachment 556845 [details] [diff] [review]
Patch to NSS - fixed version of (1a)

r+

But it should be tested to make sure it does not prevent FF or TB from overriding.
Attachment #556845 - Flags: review+
Blocks: 683261
(In reply to Daniel Veditz from comment #80)
> The bug is in the NSS product, but changes weren't checked into the NSS CVS
> repository yet. Should we move the bug to "Core::Security:PSM"

Given that we had changes in NSS, too (which for example gives at least basic coverage for code signing and S/MIME certs), which are required because the PSM patch only covers SSL, it belongs to both areas.


> and close it,
> or leave it open for the NSS part?

Ok, we have assigned
  NSS_BUILTINS_LIBRARY_VERSION "1.85"
for "removed DigiNotar".

I should check this in to both NSS trunk and NSS 3.12 branch,
for tracking purposes I should create a NSS CVS release tag
  "NSS 3.12.11 with CKBI 1.85"

After I do, I'll close this bug.


> Any alternate "better"/permanent solutions should happen in a new bug linked
> to this one.

I filed bug 683261
trunk:

cvs commit: Examining .
Checking in certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.79; previous revision: 1.78
done
Checking in certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.76; previous revision: 1.75
done
Checking in nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.30; previous revision: 1.29
done


3.12 branch:

cvs commit: Examining .
Checking in certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.67.2.11; previous revision: 1.67.2.10
done
Checking in certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.64.2.11; previous revision: 1.64.2.10
done
Checking in nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.24.2.6; previous revision: 1.24.2.5
done


I've created tag NSSCKBI_1_85_RTM on 3.12 branch.

Do we need to create
  NSS_3_12_11_WITH_CKBI_1_85_RTM
?

Resolving fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Group: core-security
Whiteboard: [sg:high] keep bug hidden for now → [sg:vector-high]
Do we have better test sites somewhere?


>https://diginotar.nl
Seems to redirect https connections to http now.

>https://zga-tag.zorggroep-almere.nl 
Is password protected?

Thanks.
The following Google doc has a list of sites that may have certs that are affected https://docs.google.com/spreadsheet/ccc?pli=1&key=0AtLNtYDDyKsudG1lc2xmRDZRNTBkdXR1M0gzelZ3MkE&hl=en_GB#gid=0

I would be interested in seeing a site that fails the cert issued between July 1, 2011 and now case.
Really? An exception is made by request of the dutch government?
Damn, the Iranian government should have just requested a few exceptions too, then this whole circus wouldn't have happened.

Joking aside, the current state of affairs is that DigiNotar doesn't even know the impact of the hack yet, even though it took place over a month ago. They won't guarantee anything and only talk in terms of "no indication so far" and "probably not affected". A company like that is clearly incapable of operating a root CA, and any certificate issued by them is contaminated.
Sure, the impact might be huge. But making an exception just because a government asks is just irresponsible. Especially given their track record on security and privacy. 
Please realize that highly confidential information like taxes, debts, family composition, are all handled via (mandatory) government-operated websites using certificates that are now clearly untrustworthy, because DigiNotar won't guarantee no rogue "copies" exist.

The dutch government are just trying to save their faces here. Diginotar isn't the first IT partner of the dutch government that proved to be incapable, and news of it would further lower the trust people have in government-IT.

I would expect mozilla to see through these attempts and just do the right thing, so I'm disappointed by this exception patch. It seems even microsoft is taking the hard stance in this by just blocking all DigiNotar issued certificates.

On a sidenote:
DigiNotar is currently very busy moving affected customers (non governmental) to the "PKI Overheid" root, which I think is very peculiar, since "PKI Overheid" is meant for governmental sites only, and given the current status of "probably not compromised". Source (in dutch): http://webwereld.nl/nieuws/107764/diginotar--mogelijk-nog-valse-certificaten-in-omloop-.html
Blocks: 683460
Attachment #556838 - Attachment is obsolete: true
Attachment #556840 - Attachment is obsolete: true
Attachment #556841 - Attachment is obsolete: true
Attachment #556846 - Attachment is obsolete: true
Attachment #556845 - Attachment is obsolete: true
The exeption in the patch for the dutch goverment isn't covering all cases. The dutch govement use 2 root ca's. "Staat der Nederlanden Root CA" and "Staat der Nederlanden Root CA - G2", the last one will fail now.
Looking for examples, but not found yet. Users have reported that overheid.nl use the second CA, but after checking this the site use the first one.

The exeption can be changed maybe into a start string compare instead of a whole string compare?
Alex: bug 683449.

Gerv
Depends on: 683449
I think it is a bad decision to make the exception for "Staat der Nederlanden Root CA".

DigiNotar can not guarantee that other certificates have not been falsified.

The core of SSL is about trust and we cannot trust DigiNotar anymore.

The fact that the Dutch government is too lazy to get a new certificate at a reputable Root CA is not Mozilla's problem imho.

The exception made in Firefox decreases the trust users can have in SSL and in Firefox.
Has any body tested the July 1 can't override part of the code. It appears to me that it will only work if the cert is issued directly from the Root CA, and not from an intermediate (say "DigiNotar Public CA 2025").

The quick fix is to check for DigiNotar Public CA 2025.

The longer fix would be to keep track of over seeing 
    1) any cert issued after July 1 and 
    2) the DigiNotar Root CA is found as an issuer of any certificate.
Then return the hard failure error code if both conditions become true.
The good news is, everything I did for the stable branch of NSS was correct.

The news that need a correction is, for NSS trunk, I made a mistake. I didn't remove it, I landed an attempt of an override cert.

Let me clean this up.
This is simple, I will remove the DigiNotar CA cert - that's the same that we did on the stable branches.

cvs commit: Examining .
Checking in certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.80; previous revision: 1.79
done
Checking in certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.77; previous revision: 1.76
done
(In reply to Robert Relyea from comment #94)
> Has any body tested the July 1 can't override part of the code. It appears
> to me that it will only work if the cert is issued directly from the Root
> CA, and not from an intermediate (say "DigiNotar Public CA 2025").
> 
> The quick fix is to check for DigiNotar Public CA 2025.
> 
> The longer fix would be to keep track of over seeing 
>     1) any cert issued after July 1 and 
>     2) the DigiNotar Root CA is found as an issuer of any certificate.
> Then return the hard failure error code if both conditions become true.



Bob, I apologize, but I have trouble understanding your question.

Is your question
  "Have we tested that the July 1 date code works correctly"?

Yes, I think Brian has tested this code, using an earlier hardcoded date, and was able to confirm that it works in general.
(In reply to Robert Relyea from comment #94)
> Has any body tested the July 1 can't override part of the code. It appears
> to me that it will only work if the cert is issued directly from the Root
> CA, and not from an intermediate (say "DigiNotar Public CA 2025").

I don't think anyone has found a site with a legitimately-issued post-July cert, but bsmith said:

<bsmith> By the way, this is how I tested this patch:
<bsmith> I took the patch, and modified the hard-coded date from "2011" to "2001"
<bsmith> that way, I can use https://www.diginotar.nl as a testcase
<bsmith> When the date is coded as 2001, it will be hard-blocked
<bsmith> when the date is 2011, it will be soft-blocked

The cert for www.diginotar.nl is not issued directly off the root.
<bsmith> By the way, this is how I tested this patch:
<bsmith> I took the patch, and modified the hard-coded date from "2011" to "2001"
<bsmith> that way, I can use https://www.diginotar.nl as a testcase

We should rerun the test win 2009... the entire diginotar infrastructure starts at 2007, so the intermedate certificate that signs www.diginotar.nl would be issued after 2001, so the could would give you a false positive.

bob
(In reply to Robert Relyea from comment #99)
> <bsmith> By the way, this is how I tested this patch:
> <bsmith> I took the patch, and modified the hard-coded date from "2011" to
> "2001"
> <bsmith> that way, I can use https://www.diginotar.nl as a testcase
> 
> We should rerun the test win 2009... the entire diginotar infrastructure
> starts at 2007, so the intermedate certificate that signs www.diginotar.nl
> would be issued after 2001, so the could would give you a false positive.
> 
> bob

Oof - I don't know if bsmith is around a build environment right now. Can we get someone to triple check this post-haste?
testing now firefox beta with date changed to 2009
The code to "upgrade to revoked" doesn't work, for two reasons.

First reason:
Because we removed the cert from the trust list, certs which chain up to "DigiNotar Root CA" are now reported as "not trusted" by NSS.
This means, we fail early in PSM, and function PSM_SSL_BlacklistDigiNotar is never reached.

Second reason (found by Bob):
PSM_SSL_BlacklistDigiNotar only checks date's of certs that are issued directly from the Root. This are probably only the intermediate certs, but not the server certs.
This explains why Brian's testing appeared to work (all intermediates are issued after 2001), but it doesn't actually work, because all intermediates were probably issued before July 2011.
I should have noticed that during code review.

While we don't upgrade to revoked, the good news is that we should treat certs as untrusted in all scenarios.

Bob has suggested to produce a NSS level patch that implements the upgrade-to-revoked, which would cover all uses of the certs, not just SSL, but also S/MIME and code signing.

I can't do this today. I can try to look into that tomorrow.
Let's discuss this in a separate new bug.
Ok, nice catch! We decided to hold off on the rebuilds and instead wait for the new patch. Please let me know when you wake up if it is not possible to get the patch within a day. Thanks!
My recommendation is, for a respin, take at least the patch in bug 682927 (NSS).

To continue working on PSM code, I've filed bug 683883.
I will attach a patch to bug 683883 shortly.
Kai: are you sure your last comment (comment #104) is correct? It says to take an NSS patch from this bug, but there is no NSS patch in this bug.

Gerv
I apologize I will reword and update comment 104:


My recommendation is, for a respin, take at least the patch in bug 683261 (NSS).
(attachment 557313 [details] [diff] [review] - needs "gmake generate" after applying)

In order to fix the PSM code, I've filed bug 683883.
I have already attached a patch to bug 683883
(attachment 557490 [details] [diff] [review] - needs review).
FYI,

The big issue for the Dutch government here is that the DigiD system is used within The Netherlands for various interactions between citizens and government computers, which includes online tax form submissions. However, as far as I know, no SSL certificates are handed out or signed for individuals - it is mostly still user/password based.

Note however, that the timing constrains discussed here might not be strong enough, as F-Secure has found traces of Diginotar compromises going back as far as May 2009

http://www.f-secure.com/weblog/archives/00002228.html
QA needs to track this bug as it still needs verification on Firefox 8 and 9
Whiteboard: [sg:vector-high] → [sg:vector-high][qa+]
The 'Target Milestone' field is empty for this bug, but I understand that it's fixed in 3.13.  Any reasons why this is not reflected in the target milestone?  If I pickup the NSS_3_13_1_RTM tag, will I get the fix?
NSS contains various modules. One of them is the "root module" named nssckbi.

Sometimes, like we did for this bug, only a newer root module is produced.

This bug was fixed on the old 3.12.x branch, however, in order to obtain the fix, you must pull:
- NSS_3_12_11_RTM
and on top of that, you must pull the directory containing the root module from tag
- NSSCKBI_1_85_RTM


Because we don't have target milestone values for such "in-between-versions" combinations, I set the target milestone to 3.12.12 (even though that version was not released (yet)).


Regarding the new major version 3.13.x, all such versions have the DigiNotar fix, both 3.13 and 3.13.1
Target Milestone: --- → 3.12.12
I have tried this using https://www.diginotar.nl/ and I get:
"The connection to www.diginotar.nl was interrupted while the page was loading."

The site is blocked. Setting resolution to Verified Fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0 beta 6
Keywords: verified-beta
Whiteboard: [sg:vector-high][qa+] → [sg:vector-high][qa+][qa!:9]
Verified on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0 beta 1
Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0 beta 1
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0 beta 1

The site from comment112 is blocked.
Whiteboard: [sg:vector-high][qa+][qa!:9] → [sg:vector-high][qa+][qa!:9][qa!:10]
Status: RESOLVED → VERIFIED
Whiteboard: [sg:vector-high][qa+][qa!:9][qa!:10] → [sg:vector-high][qa!]
You need to log in before you can comment on or make changes to this bug.