Last Comment Bug 682927 - Dis-trust DigiNotar root certificate
: Dis-trust DigiNotar root certificate
Status: VERIFIED FIXED
[sg:vector-high][qa!]
: verified-beta
Product: NSS
Classification: Components
Component: CA Certificates (show other bugs)
: trunk
: All All
: -- blocker with 2 votes (vote)
: 3.12.12
Assigned To: Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
:
Mentors:
: 682956 683455 (view as bug list)
Depends on: 683449
Blocks: 683261 683460
  Show dependency treegraph
 
Reported: 2011-08-29 12:02 PDT by Gervase Markham [:gerv]
Modified: 2013-07-29 11:32 PDT (History)
54 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
.1-fixed
+
verified
+
verified
+
verified
.21+
.21-fixed


Attachments
Blacklist all Diginotar-issued certificates based on "CN=DigiNotar " in issuer (1.46 KB, patch)
2011-08-29 14:20 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Review
Pastebin document linked to in summary (in case it disappears) (6.00 KB, text/plain)
2011-08-29 15:16 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details
Less strict blacklisting of all Diginotar-issued certificates based on "CN=Diginotar " in issuer (5.66 KB, patch)
2011-08-29 16:49 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Review
Less strict blacklisting of all Diginotar-issued certificates based on "CN=Diginotar " in issuer (v2) (5.63 KB, patch)
2011-08-29 16:53 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Review
Blacklist all Diginotar-issued certificates, considering the whole chain, date-based hard block (10.33 KB, patch)
2011-08-29 21:30 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
dveditz: review+
Details | Diff | Review
Blacklist all Diginotar-issued certificates, considering the whole chain, Comodogate-ish hard block (12.81 KB, patch)
2011-08-29 22:04 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
dveditz: feedback-
Details | Diff | Review
patch - remove diginotar root from stable branches (NSS 3.12) (77.06 KB, patch)
2011-08-30 01:44 PDT, Kai Engert (:kaie)
brian: review+
dveditz: feedback+
Details | Diff | Review
patch - remove diginotar root from NSS trunk / mozilla-central (76.33 KB, patch)
2011-08-30 01:45 PDT, Kai Engert (:kaie)
brian: review+
dveditz: feedback+
Details | Diff | Review
patch - remove diginotar from PSM's whitelist for EV (1.26 KB, patch)
2011-08-30 02:18 PDT, Kai Engert (:kaie)
brian: review+
dveditz: feedback+
Details | Diff | Review
[v2] Blacklist all DigiNotar-issued certificates, considering the whole chain, date-based hard block (10.60 KB, patch)
2011-08-30 03:23 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
dveditz: review+
kaie: review+
Details | Diff | Review
BAD (9.76 KB, patch)
2011-08-30 07:49 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
FYI - current (old) cert in NSS (1.39 KB, application/octet-stream)
2011-08-30 07:51 PDT, Kai Engert (:kaie)
no flags Details
FYI - the override cert I created (1.39 KB, application/octet-stream)
2011-08-30 07:52 PDT, Kai Engert (:kaie)
no flags Details
FYI - text dump of old cert (4.96 KB, text/plain)
2011-08-30 07:53 PDT, Kai Engert (:kaie)
no flags Details
Patch to NSS - fixed version of (1a) (9.87 KB, patch)
2011-08-30 08:10 PDT, Kai Engert (:kaie)
rrelyea: review+
Details | Diff | Review
FYI - text dump of the override cert that I created (4.96 KB, text/plain)
2011-08-30 08:11 PDT, Kai Engert (:kaie)
no flags Details
Modifications to attachment 556791 that I did in order to make the patch apply cleanly on 1.9.2 (1.54 KB, patch)
2011-08-30 08:39 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
Modifications to attachment 556791 that I did in order to make the patch apply cleanly on 1.9.2 (9.05 KB, patch)
2011-08-30 08:42 PDT, :Ehsan Akhgari (busy, don't ask for review please)
brian: review+
Details | Diff | Review
Fox-IT Interim report: DigiNotar Certificate Authority breach “Operation Black Tulip” September 5, 2011 (380.28 KB, application/pdf)
2011-09-05 19:08 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details

Description Gervase Markham [:gerv] 2011-08-29 12:02:12 PDT
Reported by Adam Langley to security-group:

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

Gerv
Comment 1 Gervase Markham [:gerv] 2011-08-29 12:09:27 PDT
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
Comment 2 Adam Langley 2011-08-29 12:26:45 PDT
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.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 12:44:53 PDT
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?
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-08-29 13:04:08 PDT
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.
Comment 8 christian 2011-08-29 13:43:38 PDT
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)
Comment 9 Adam Langley 2011-08-29 14:04:44 PDT
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.
Comment 10 Robert Relyea 2011-08-29 14:12:59 PDT
> 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).
Comment 11 Gervase Markham [:gerv] 2011-08-29 14:17:25 PDT
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
Comment 12 Robert Relyea 2011-08-29 14:19:44 PDT
> 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
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 14:20:41 PDT
Created attachment 556670 [details] [diff] [review]
Blacklist all Diginotar-issued certificates based on "CN=DigiNotar " in issuer

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.
Comment 14 Adam Langley 2011-08-29 14:22:44 PDT
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.
Comment 15 Robert Relyea 2011-08-29 14:23:30 PDT
> 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.
Comment 16 Robert Relyea 2011-08-29 14:26:24 PDT
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.
Comment 18 Adam Langley 2011-08-29 14:29:23 PDT
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?
Comment 20 Johnathan Nightingale [:johnath] 2011-08-29 14:36:47 PDT
I wonder if Brian and Bob R should get on the phone and work this through?
Comment 21 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 14:40:34 PDT
(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.
Comment 22 Adam Langley 2011-08-29 14:50:09 PDT
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.
Comment 23 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 15:16:57 PDT
Created attachment 556684 [details]
Pastebin document linked to in summary (in case it disappears)
Comment 24 Geo Mealer [:geo] -- This account is inactive after 2015-07-07 2011-08-29 15:18:52 PDT
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.
Comment 25 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 15:46:03 PDT
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
Comment 26 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 16:49:02 PDT
Created attachment 556711 [details] [diff] [review]
Less strict blacklisting of all Diginotar-issued certificates based on "CN=Diginotar " in issuer

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 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 16:53:01 PDT
Created attachment 556716 [details] [diff] [review]
Less strict blacklisting of all Diginotar-issued certificates based on "CN=Diginotar " in issuer (v2)

Fixed typo.
Comment 28 Wan-Teh Chang 2011-08-29 18:15:12 PDT
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.
Comment 29 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 18:58:11 PDT
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.
Comment 30 Wan-Teh Chang 2011-08-29 19:08:56 PDT
Brian: you are also going to remove the root certificate from
certdata.c, aren't you?
Comment 31 Daniel Veditz [:dveditz] 2011-08-29 19:15:12 PDT
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.
Comment 32 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 20:08:59 PDT
(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.
Comment 33 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 21:30:13 PDT
Created attachment 556748 [details] [diff] [review]
Blacklist all Diginotar-issued certificates, considering the whole chain, date-based hard block

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.
Comment 34 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 21:34:03 PDT
Here is the try run for the previous patch: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=a598c12afa89
Comment 35 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-29 22:04:06 PDT
Created attachment 556755 [details] [diff] [review]
Blacklist all Diginotar-issued certificates, considering the whole chain, Comodogate-ish hard block

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.
Comment 36 christian 2011-08-29 22:56:26 PDT
(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)
Comment 37 Daniel Veditz [:dveditz] 2011-08-30 00:02:53 PDT
(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 38 Daniel Veditz [:dveditz] 2011-08-30 00:06:58 PDT
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.
Comment 39 christian 2011-08-30 00:25:22 PDT
As gerv and reed mentioned earlier we should probably open up this bug...details are all over the web.
Comment 40 Kai Engert (:kaie) 2011-08-30 00:41:25 PDT
When searching for CN=DigiNotar, should we do a string comparison that ignores upper/lowercase, just to be sure?
Comment 41 Kai Engert (:kaie) 2011-08-30 01:18:56 PDT
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 42 Daniel Veditz [:dveditz] 2011-08-30 01:36:31 PDT
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.
Comment 43 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 01:41:34 PDT
(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.
Comment 44 Kai Engert (:kaie) 2011-08-30 01:44:42 PDT
Created attachment 556774 [details] [diff] [review]
patch - remove diginotar root from stable branches (NSS 3.12)
Comment 45 Kai Engert (:kaie) 2011-08-30 01:45:33 PDT
Created attachment 556775 [details] [diff] [review]
patch - remove diginotar root from NSS trunk / mozilla-central
Comment 46 Kai Engert (:kaie) 2011-08-30 01:54:13 PDT
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?
Comment 47 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 02:12:13 PDT
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".
Comment 48 Kai Engert (:kaie) 2011-08-30 02:18:03 PDT
Created attachment 556779 [details] [diff] [review]
patch - remove diginotar from PSM's whitelist for EV
Comment 49 Daniel Veditz [:dveditz] 2011-08-30 02:28:05 PDT
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+)
Comment 50 Kai Engert (:kaie) 2011-08-30 03:10:38 PDT
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.
Comment 51 Kai Engert (:kaie) 2011-08-30 03:11:05 PDT
Who agrees with my comment 50?
Comment 52 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 03:23:47 PDT
Created attachment 556791 [details] [diff] [review]
[v2] Blacklist all DigiNotar-issued certificates, considering the whole chain, date-based hard block

Here is the updated patch that avoids blocking DigiNotar certs that chain to the Dutch government root CA, by request of the Dutch government.
Comment 53 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 03:29:14 PDT
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)
Comment 54 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 03:36:16 PDT
(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.
Comment 55 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 03:45:18 PDT
Tryserver build for the latest PSM patch: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=1c90597fb9a9
Comment 56 Daniel Veditz [:dveditz] 2011-08-30 04:02:59 PDT
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
Comment 57 Adam Langley 2011-08-30 04:21:04 PDT
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 58 Kai Engert (:kaie) 2011-08-30 05:08:03 PDT
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.
Comment 59 Johnathan Nightingale [:johnath] 2011-08-30 06:50:27 PDT
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)
Comment 60 Kai Engert (:kaie) 2011-08-30 07:17:38 PDT
(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.
Comment 61 Kai Engert (:kaie) 2011-08-30 07:33:41 PDT
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.
Comment 62 Kai Engert (:kaie) 2011-08-30 07:49:50 PDT
Created attachment 556837 [details] [diff] [review]
BAD

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.
Comment 63 Kai Engert (:kaie) 2011-08-30 07:51:16 PDT
Created attachment 556838 [details]
FYI - current (old) cert in NSS
Comment 64 Kai Engert (:kaie) 2011-08-30 07:52:17 PDT
Created attachment 556840 [details]
FYI - the override cert I created
Comment 65 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 07:52:49 PDT
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...
Comment 66 Kai Engert (:kaie) 2011-08-30 07:53:24 PDT
Created attachment 556841 [details]
FYI - text dump of old cert
Comment 67 Kai Engert (:kaie) 2011-08-30 07:54:53 PDT
Comment on attachment 556837 [details] [diff] [review]
BAD

argh I made a mistake
Comment 68 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 07:56:56 PDT
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.
Comment 69 Gervase Markham [:gerv] 2011-08-30 07:59:08 PDT
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
Comment 70 Johnathan Nightingale [:johnath] 2011-08-30 08:04:32 PDT
(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.
Comment 71 Kai Engert (:kaie) 2011-08-30 08:10:02 PDT
Created attachment 556845 [details] [diff] [review]
Patch to NSS - fixed version of (1a)

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.
Comment 72 Kai Engert (:kaie) 2011-08-30 08:11:51 PDT
Created attachment 556846 [details]
FYI - text dump of the override cert that I created
Comment 73 Kai Engert (:kaie) 2011-08-30 08:15:35 PDT
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.
Comment 74 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 08:38:25 PDT
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.
Comment 75 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 08:39:41 PDT
Created attachment 556856 [details] [diff] [review]
Modifications to attachment 556791 [details] [diff] [review] that I did in order to make the patch apply cleanly on 1.9.2
Comment 76 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 08:42:13 PDT
Created attachment 556857 [details] [diff] [review]
Modifications to attachment 556791 [details] [diff] [review] that I did in order to make the patch apply cleanly on 1.9.2

Oops, wrong patch
Comment 77 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 09:28:20 PDT
(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.
Comment 78 Daniel Veditz [:dveditz] 2011-08-30 09:31:29 PDT
(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.
Comment 79 Daniel Veditz [:dveditz] 2011-08-30 09:35:10 PDT
(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.
Comment 80 Daniel Veditz [:dveditz] 2011-08-30 10:17:20 PDT
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.
Comment 81 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 10:25:01 PDT
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 82 Wan-Teh Chang 2011-08-30 10:39:02 PDT
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.
Comment 83 Robert Relyea 2011-08-30 11:02:35 PDT
> 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 84 Robert Relyea 2011-08-30 11:04:47 PDT
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.
Comment 85 Kai Engert (:kaie) 2011-08-30 12:40:05 PDT
(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
Comment 86 Kai Engert (:kaie) 2011-08-30 13:07:26 PDT
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.
Comment 87 Reed Loden [:reed] (use needinfo?) 2011-08-30 22:49:04 PDT
*** Bug 682956 has been marked as a duplicate of this bug. ***
Comment 88 Huzaifa Sidhpurwala 2011-08-30 23:44:13 PDT
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.
Comment 89 Kevin Brosnan [:kbrosnan] 2011-08-31 01:14:04 PDT
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.
Comment 90 Mathijs Kwik 2011-08-31 01:31:19 PDT
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
Comment 91 Alex Elderson 2011-08-31 05:28:24 PDT
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?
Comment 92 Gervase Markham [:gerv] 2011-08-31 05:37:30 PDT
Alex: bug 683449.

Gerv
Comment 93 Teun van Eijsden 2011-08-31 09:00:59 PDT
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.
Comment 94 Robert Relyea 2011-08-31 10:09:56 PDT
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.
Comment 95 Kai Engert (:kaie) 2011-08-31 10:14:41 PDT
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
Comment 96 Kai Engert (:kaie) 2011-08-31 11:26:40 PDT
(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.
Comment 97 Daniel Veditz [:dveditz] 2011-08-31 11:33:12 PDT
(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.
Comment 98 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-31 11:43:35 PDT
*** Bug 682956 has been marked as a duplicate of this bug. ***
Comment 99 Robert Relyea 2011-08-31 14:02:07 PDT
<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
Comment 100 Johnathan Nightingale [:johnath] 2011-08-31 14:08:38 PDT
(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?
Comment 101 Kai Engert (:kaie) 2011-08-31 14:27:09 PDT
testing now firefox beta with date changed to 2009
Comment 102 Kai Engert (:kaie) 2011-08-31 15:48:31 PDT
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.
Comment 103 christian 2011-08-31 17:41:54 PDT
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!
Comment 104 Kai Engert (:kaie) 2011-09-01 06:49:39 PDT
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.
Comment 105 Gervase Markham [:gerv] 2011-09-01 07:27:26 PDT
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
Comment 106 Kai Engert (:kaie) 2011-09-01 07:39:32 PDT
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).
Comment 107 Paul Wouters 2011-09-01 18:22:47 PDT
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
Comment 108 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-05 19:08:29 PDT
Created attachment 558368 [details]
Fox-IT Interim report: DigiNotar Certificate Authority breach “Operation Black Tulip” September 5, 2011
Comment 109 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-09 14:40:54 PDT
QA needs to track this bug as it still needs verification on Firefox 8 and 9
Comment 110 tnzzbugs 2011-11-17 16:12:43 PST
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?
Comment 111 Kai Engert (:kaie) 2011-11-29 05:56:29 PST
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
Comment 112 Vlad [QA] 2011-12-15 00:06:35 PST
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
Comment 113 Vlad [QA] 2011-12-28 02:09:30 PST
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.
Comment 114 Gervase Markham [:gerv] 2013-07-29 11:32:25 PDT
*** Bug 683455 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.