825022 - Deal with TURKTRUST mis-issued *.google.com certificate

Status: RESOLVED FIXED

(NSS :: CA Certificates Code, task)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 410821, bug 411299, and bug 768547 which deal with adding TURKTRUST.

As you may have been informed by Google by now, we received an email from Google at 17:38 on December 26, 2012 indicating that somebody has issued a "*.google.com” certificate chaining to one of TURKTRUST's Roots.

Upon our immediate investigation, we found that the "*.google.com” certificate has been issued by the below certificate which has the extensions of an intermediate CA certificate, although it was totally intended to be an end user SSL certificate:

<CN = *.EGO.GOV.TR, OU = EGO BILGI ISLEM, O = EGO, L = ANKARA, S = ANKARA, C = TR>

After having completed an initial control on our vital system components, it has been noticed that the target certificate had been issued with false extensions. Thus, we immediately revoked the related SSL certificate for the server “*.ego.gov.tr” at 18:08 on December 26, 2012.

It was found out that the certificate had been issued as a result of a valid certification request from a governmental agency and the related paperwork has been crosschecked from the archive. The related SSL certificate subscriber is one of the government bodies in Turkey namely “EGO Genel Müdürlüğü (Ankara Electricity, Gas and Bus Operation)” under the Head of Ankara Metropolitan
Municipality.

EGO is an end-user (not a sub-CA) and the application is for an end-user SSL certificate that should have a lifetime of 2 years. Yet the extensions for this certificate had been incorrectly set. It has been observed that the target certificate had been issued within our knowledge and there existed no certificate that has been issued without our control. This is because TURKTRUST uses no intermediate CAs for SSL certificate issuance and end-user SSL certificates are only issued by TURKTRUST SSL subroot certificates upon requests of end-user clients.

This incident is to be treated as a fault due to a software error occurred during profile based certificate generation on our systems. We have not met any clues for a security breach or problem during our investigation which may have been exploited for any fraudulent purposes.

This specific certificate is one of only two instances happened due to a faulty SSL certificate generation that took place at the same date and time, i.e. August 8, 2011 - 10:07:44. The whole TURKTRUST certificate database has been thoroughly searched and examined to find out whether there are other instances of such faulty certificate generation. However, only two certificates were found, one being the EGO certificate above and the other one was for another organization (was issued at the same time via the same production batch). The second certificate was revoked on September 13, 2011. The revoked one had also been issued as a result of a valid certification request and the related paperwork had been checked as well.

We do have a log of all certificates ever issued from any roots or subroots. The issuance process is in many ways designed to be secured as it is physically and electronically isolated from the outside world (no need to mention the use of separation of duties, cross checking roles, etc.). For a first level sub-root, the process is even more heavily implemented as only a couple of roots or first level subroots have been issued since the first day of our operation, in accordance with our CPS and internal procedures. As regards to the issuance of a sub-root under a sub-root (which happened to occur twice on the same day and exact time of issuance of an SSL end entity certificate), this is certainly not allowed. A query that searched the database ensured that this happened only twice in tens of thousands of certificates issuance.

Nevertheless, a few months after this faulty certificate generation case occured in August 2011, our CA management software had major improvements and upgrades during Fall 2011 due to our preparations for an external ETSI TS 102 042 audit. At the same time, our CA infrastructure had undergone several improvements including hardware and network based upgrades as well. The
audit took place on October 31 to November 2, 2011 by KPMG auditors and after passing the audit successfully TURKTRUST was granted ETSI TS 102 042 compliance certificate for NCP and EVCP by British Standards Institution (BSI). You can find the related ETSI certificate attached. This year, we had our continuous assessment audit again by KPMG on December 11-13, 2012 and again passed the audit successfully. We believe that no such instances will occur again as all possible factors are eliminated to the largest possible extent after the above mentioned system upgrade.

Although our systems were upgraded and further improved before the successful November 2011 ETSI 102 042 audit, and under continuous audit of our established quality assurance system, the last event is to be taken very seriously and will certainly lead us to tighten our audit procedures. The system shall definitely be amended with additional controls (to the already existing ones) to detect if and when any such unexpected system error is occurring right before committing the action.

After being informed about the case on December 26, 2012, we contacted our client EGO in the evening at about 21:00 local time and discussed this issue with the related server administrator. We learned that they carried their SSL certificate key to a Checkpoint firewall. As far as we understood, they tried to make a firewall configuration that would regulate the access to “google.com” from the
internal machines. It is revealed that they very recently exported their old certificate to a firewall environment where a new certificate is somehow issued.

This morning, December 27, 2012 at 10:00 we visited EGO to investigate the case in more detail on our client systems. Our findings are as follows:

• On the day of fake "*.google.com” certificate issuance by the “*.ego.gov.tr” certificate (to repeat again, not an intended subCA certificate, should have been an end-user SSL certificate) our client EGO performed a firewall change operation via its subcontractor
company (the firewall brand is changed).

• The EGO SSL certificate is installed on the newly installed Checkpoint firewall.

• MITM proxy configurations were made in order to be able to search HTTPS packages.

• After completing these changes, it was observed by EGO representatives that the external services could not be reached especially through Chrome.

EGO representatives told us that they did not generate a "*.google.com” certificate deliberately. Before leaving EGO site, we warned EGO representatives about the risks caused by the relevant PFX file. Currently, there is no evidence of an attack that compromised the private key.

There has not been any suspicious increase in the number of OCSP requests coming for EGO SSL certificate. Above 99% of such requests has come from the IPs belonging to EGO.

We are sharing the above information with all major browser vendors as well and keep going for further investigations about this case.

Best Regards,

Mert Özarar
TURKTRUST Inc.

Comment 1

[Gervase Markham [:gerv]]

[Coped from s-g message]

The explanation quoted above makes sense to me - I can imagine the possibility that a firewall admin sticks any old cert into their shiny new firewall to get it to work, and it happens to be the one cert TurkTrust has issued which can do MITM.

TurkTrust claim they didn't deliberately issue an intermediate; it's an interesting but perhaps eventually irrelevant question to wonder whether EGO actually did specifically request one. I would guess not, given that it's taken them 18 months to use it in that way.

However, that leaves the question of how TurkTrust managed to issue it in the first place. The total replacement of all their systems may make that a hard question to answer.

Given that there has been no system compromise, and no uncontrolled or open-ended issuance, I would suggest this is not chemspill-worthy. But I'm open to counter-argument on that point. 

Gerv

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Certificates received from Google (ZIP file)

Comment 3

[Adam Langley]

(Hopefully you received my message yesterday that contains the intermediates in question.)

Currently there are a couple of intermediates, issued from TURKTRUST, where the private key has essentially escaped. Until just a few hours ago one of the private keys was on an IIS server at https://posta.ego.gov.tr.

The private keys have not been published at least, and we don't have any evidence of any further exploitation. Chrome has performed pushes to revoke both of the `mistaken' intermediate certificates.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: *.google.com certificate mis-issued by *.EGO.GOV.TR (Binary DER encoding)

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: *.EGO.GOV.TR certificate with isCA=true issued by TÜRKTRUST Elektronik Sunucu Sertifikası Hizmetleri (Binary DER encoding)

These are the same certs, just converted to binary DER encoding.

Comment 6

[Adam Langley]

Attachment: Second mistaken intermediate

This is the second mistaken CA certificate, as provided by TURKTRUST.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 696103 [details]
*.EGO.GOV.TR certificate with isCA=true issued by TÜRKTRUST Elektronik Sunucu Sertifikası Hizmetleri (Binary DER encoding)

Sorry, there was some confusion here. This is actually the "Second mistaken intermediate" and not the "*.EGO.GOV.TR" certificate," which we don't have here.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Distrust both currently-known-bad intermediates [correct but incomplete]

I am still testing this patch.

Google sent us two PEM files: turktrust-google.pem and turktrust-intermediate-2.pem.

turktrust-google.pem contains this cert chain, which I split into four files and converted to DER format:

   turktrust-google-1.der (EE): Bad *.google.com certificate
   turktrust-google-2.der: Mis-issued *.EGO.GOV.TR intermediate
   turktrust-google-3.der: Internal(?) Turktrust intermediate
   turktrust-google-4.der: Turktrust root, already in NSS

I also converted turktrust-intermediate-2.pem to turktrust-intermediate-2.der.

The PEM-to-DER conversions were done using:

   openssl -inform pem -outform der < $INFILE > $OUTFILE

From there, I followed the work that Kai, Wan-Teh, and Bob did in bug 724929 for Trustwave's mis-issued certifiate. I used the vfychain tool to verify that the certs were trusted before, and are dis-trusted after.

+--------+
| Before |
+--------+

# Note that "-u 1" means "usage: SSL server"
$ vfychain -u 1 turktrust-google-1.der \
                turktrust-google-2.der \
		turktrust-google-3.der
Chain is good!

# Note that "-u 3" means "usage: SSL Server CA"
$ vfychain -u 3 turktrust-intermediate-2.der \
                turktrust-google-3.der
Chain is good!

+----------+
| Distrust |
+----------+

$ addbuiltin -D -n "TURKTRUST Mis-issued Intermediate CA 1" \
	        -i turktrust-google-2.der >> lib/ckfw/builtins/certdata.txt
$ addbuiltin -D -n "TURKTRUST Mis-issued Intermediate CA 2" \
	        -i turktrust-intermediate-2.der >> lib/ckfw/builtins/certdata.txt
		
At this point, I modified certdata.txt to add ", Bug 825022" to the comments
of both distrust records.

+-------+
| After |
+-------+

$ vfychain -u 1 turktrust-google-1.der \
                turktrust-google-2.der \
		turktrust-google-3.der
Chain is bad!
PROBLEM WITH THE CERT CHAIN:
CERT 1. CN=*.EGO.GOV.TR,OU=EGO BILGI ISLEM,O=EGO,L=ANKARA,ST=ANKARA,C=TR [Certificate Authority]:
  ERROR -8172: Peer's certificate issuer has been marked as not trusted by the user.
    O=T+£RKTRUST Bilgi -¦leti+ƒim ve Bili+ƒim G++venli-ƒi Hizmetleri A...,C=TR,CN=T+£RKTRUST Elektronik Sunucu Sertifika
s-¦ Hizmetleri

$ vfychain -u 3 turktrust-intermediate-2.der \
                turktrust-google-3.der
chain is bad!
ROBLEM WITH THE CERT CHAIN:
ERT 0. [e-mail] :
 ERROR -8171: Peer's certificate has been marked as not trusted by the user.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #8)
>    openssl -inform pem -outform der < $INFILE > $OUTFILE

openssl x509 -inform pem -outform der < $INFILE > $OUTFILE

Comment 10

[Huzaifa Sidhpurwala]

Any plans for a chemspill?

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Per security-group discussion, there are no current plans to chemspill. I would encourage us to discuss it there, with wider viewing and comments, than in this bug.

Comment 12

[Robert Kaiser]

Also note that the regular release of 18 and ESR updates is due on Jan 8 and a number of people are fully or partially on holiday until close to that (on our side as well as users), so if there's no absolute need for a chemspill, it's best for everyone, including users, to only get this with the regular update.
We're still trying to find out the real extent of this and the measures we need to take, though, so a chemspill could still come around. That said, every day we get nearer to the regular release, the more likely is that we don't chemspill and ship with 18 and friends instead.

Comment 13

[Kai Engert (:KaiE:)]

I independently created the same patch baed on the attached intermediate certificates, so I confirm it's what we want to technically knockout the affected intermediates.

But the patch is slightly incomplete, because of bureaucracy.

We must release a new version of the "root list module" contained in NSS (aka nssckbi).
The complete patch must include a change to upgrade the version number to CKBI 1.93

NSS 3.14 and NSS 3.14.1 include CKBI 1.92

NSS 3.13.6 (used by Firefox 17) includes CKBI 1.91

I will attach two new patches:

- a patch for Firefox 18 and all later branches, which includes Brian's patch
  and increases the version number.

- a patch for Firefox that includes the upgrade from CKBI 1.91 to CKBI 1.92
  plus the new changes.


The upgrade on FF 17 is for consistency reasons, it adds new roots, but it doesn't add any new code.

(I'm proposing this approach because that's what we've always done in the path, an older branch might contain an older version of CKBI, but it should never contain its own mix of trust.)

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 696159 [details] [diff] [review]
Distrust both currently-known-bad intermediates [correct but incomplete]

technically correct, but incomplete (version number changes needed, and additional changes needed for FF 17 branch)

r=kaie on the precondition, that this change only gets used in combination with the additional required changes

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Remove recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı" root certificate [incomplete, for review purposes]

Recently, in bug 768547 and bug 768547, another new Turktrust root CA certificate got added to NSS.

While that new root had been released as part of NSS, it has never been shipped in any final release of Firefox yet (only as part of test releases).

This is a proposal to temporarily hold back the addition of this new root to Firefox, and (at least) temporarily remove it from the NSS root store.

I am attaching a patch (the technical subset) that could perform this removal, should it be decided that such a removal is desired.

The patch can be trivially reviewed, by comparing it with the patch v5 contained in bug 795355, and by verifying that the removal here is identical to the addition there.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: patch v3 - proposed complete patch for checkin - only Firefox 18 and later [checked in to upstream NSS 3.14.x and 3.13.x branches]

This patch combines:
- attachment 696159 [details] [diff] [review] (distrust the intermediates)
- attachment 696303 [details] [diff] [review] (remove the recently new root)
- the version number increase

This patch is appropriate for any branches that use Firefox 18 or any later version (essentially, any branch that uses at least NSS 3.14)

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 696305 [details] [diff] [review]
patch v3 - proposed complete patch for checkin - only Firefox 18 and later [checked in to upstream NSS 3.14.x and 3.13.x branches]

I will r+ this patch once both individual patches have been reviewed.

The version number change is trivial and obvious.

Comment 18

[Kai Engert (:KaiE:)]

Because of bug 683266, branches using NSS 3.14 and NSS 3.14.1 will require a slightly different landing.

On a branch that still runs NSS 3.14, without the fix for bug 683266, it is necessary to:
  cd mozilla/security/nss/lib/ckfw/builtins/
  gmake generate
in order to regenerate the certdata.c file

Comment 19

[Gervase Markham [:gerv]]

The suspension of addition of the TurkTrust root is a policy decision for our CA team; once it's been made, I will r+ or r- the patch as appropriate. Please let me know if at any time we are holding things up.

Gerv

Comment 20

[Kai Engert (:KaiE:)]

Attachment: patch v4-ESR - proposed complete patch for checkin - only Firefox 17

This patch is appropriate for FF 17 ESR, which still uses NSS 3.13.6

This patch v4-ESR changes the trust store of FF 17 to the same contents as achieved using patch v3 on FF 18 and later branches.

Comment 21

[Kai Engert (:KaiE:)]

Comment on attachment 696312 [details] [diff] [review]
patch v4-ESR - proposed complete patch for checkin - only Firefox 17

Once the individual patches get reviewed, I'll mark this patch as r+, too, because it's simply a merger of patches, for easier landing.

Comment 22

[Kai Engert (:KaiE:)]

Comment on attachment 696305 [details] [diff] [review]
patch v3 - proposed complete patch for checkin - only Firefox 18 and later [checked in to upstream NSS 3.14.x and 3.13.x branches]

Ok, luckily, we don't have any branches using the older NSS 3.14 - only branches using NSS 3.14.1 - which means, this patch is fine (no need to regenerate certdata.c)

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Gervase Markham [:gerv] from comment #19)
> The suspension of addition of the TurkTrust root is a policy decision for
> our CA team; once it's been made, I will r+ or r- the patch as appropriate.
> Please let me know if at any time we are holding things up.
> 
> Gerv

We are considering to spin FF18 beta 7, to include patches in this bug which goes to build early morning ET Monday .We would need these patches reviewed/checked in before that.

Comment 24

[u279076]

Assigning Matt Wobensmith as the QA contact so he can regress and verify this when fixed.

Comment 25

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #23)
> (In reply to Gervase Markham [:gerv] from comment #19)
> > The suspension of addition of the TurkTrust root is a policy decision for
> > our CA team; once it's been made, I will r+ or r- the patch as appropriate.
> > Please let me know if at any time we are holding things up.
> > 
> > Gerv
> 
> We are considering to spin FF18 beta 7, to include patches in this bug which
> goes to build early morning ET Monday .We would need these patches
> reviewed/checked in before that.

Gerv , has there been any progress on the policy decision yet ? Else can we please get an ETA ? Considering these patches need to go in our Firefox 18 beta 7,its preferred to have them landed by Sunday night, to get the builds kicked off early Morning ET time Monday.

Thanks in advance for the info. With the weekend around and getting to land patches in time for beta 7, this info will be very helpful for various teams working to generate/QA the build's.

Comment 26

[Kathleen Wilson]

(In reply to bhavana bajaj [:bajaj] from comment #25)
> (In reply to bhavana bajaj [:bajaj] from comment #23)
> > (In reply to Gervase Markham [:gerv] from comment #19)
> > > The suspension of addition of the TurkTrust root is a policy decision for
> > > our CA team; once it's been made, I will r+ or r- the patch as appropriate.
> > > Please let me know if at any time we are holding things up.
> > > 
> > > Gerv
> 
> Gerv , has there been any progress on the policy decision yet ? 

There is agreement among the CA team that the new TurkTrust root should not be added at this time. Please proceed with removing the recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı" root certificate (Fingerprint (SHA1): F1:7F:6F:B6:31:DC:99:E3:A3:C8:7F:FE:1C:F1:81:10:88:D9:60:33)

Thanks,
Kathleen

Comment 27

[Kai Engert (:KaiE:)]

Comment on attachment 696303 [details] [diff] [review]
Remove recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı" root certificate [incomplete, for review purposes]

(a)
The next step is for Gerv (or from anyone else from the CA team) to mark this patch as r+ in order to confirm the correctness of the removal.

As soon as that is done, I will land the change into mozilla-central and into NSS (which effectively will make public that we are having an issue with Turktrust).

The fingerprint mentioned by Kathleen is identical to the fingerpring mentioned in bug 768547, which confirms we are referring to the same root (the one added over there is to be removed here).


(b)
Please give this bug the necessary approval flag, allowing us to land the patch for Firefox 18.


(c)
Please clarify on which branch the change needs to land for Firefox 18 beta 7 - is it the standard mozilla-beta branch? If the patch is supposed to land on multiple branches, please list all of them.

As soon as I see the approval flags, I can help with the landing.

Please don't wait until the last minute with the approval flags. I'm NOT sitting next to the computer all the time, and because of time zone differences, and because of some travel on Dec 30, I will need some time to react.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Kai Engert (:kaie) from comment #27)
> Comment on attachment 696303 [details] [diff] [review]
> Remove recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı"
> root certificate [incomplete, for review purposes]
> 
> (a)
> The next step is for Gerv (or from anyone else from the CA team) to mark
> this patch as r+ in order to confirm the correctness of the removal.
> 
> As soon as that is done, I will land the change into mozilla-central and
> into NSS (which effectively will make public that we are having an issue
> with Turktrust).
> 
> The fingerprint mentioned by Kathleen is identical to the fingerpring
> mentioned in bug 768547, which confirms we are referring to the same root
> (the one added over there is to be removed here).
> 
> 
> (b)
> Please give this bug the necessary approval flag, allowing us to land the
> patch for Firefox 18.

Beta landings should take care of this.

> 
> 
> (c)
> Please clarify on which branch the change needs to land for Firefox 18 beta
> 7 - is it the standard mozilla-beta branch? If the patch is supposed to land
> on multiple branches, please list all of them.
> 
> As soon as I see the approval flags, I can help with the landing.
> 
> Please don't wait until the last minute with the approval flags. I'm NOT
> sitting next to the computer all the time, and because of time zone
> differences, and because of some travel on Dec 30, I will need some time to
> react.

I will Pre-approve all the flags needed here.Please email me if I have missed an approval on any patch that you need .

Below are the branches where this patch needs to go in :

a) mozilla-beta(default) - https://hg.mozilla.org/releases/mozilla-beta/graph/
b) esr 10(default) - https://hg.mozilla.org/releases/mozilla-esr10/graph
c) esr 17(default) - https://hg.mozilla.org/releases/mozilla-esr17/graph

Comment 29

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #28)
> (In reply to Kai Engert (:kaie) from comment #27)
> > Comment on attachment 696303 [details] [diff] [review]
> > Remove recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı"
> > root certificate [incomplete, for review purposes]
> > 
> > (a)
> > The next step is for Gerv (or from anyone else from the CA team) to mark
> > this patch as r+ in order to confirm the correctness of the removal.
> > 
> > As soon as that is done, I will land the change into mozilla-central and
> > into NSS (which effectively will make public that we are having an issue
> > with Turktrust).
> > 
> > The fingerprint mentioned by Kathleen is identical to the fingerpring
> > mentioned in bug 768547, which confirms we are referring to the same root
> > (the one added over there is to be removed here).
> > 
> > 
> > (b)
> > Please give this bug the necessary approval flag, allowing us to land the
> > patch for Firefox 18.
> 
> Beta landings should take care of this.
> 
> > 
> > 
> > (c)
> > Please clarify on which branch the change needs to land for Firefox 18 beta
> > 7 - is it the standard mozilla-beta branch? If the patch is supposed to land
> > on multiple branches, please list all of them.
> > 
> > As soon as I see the approval flags, I can help with the landing.
> > 
> > Please don't wait until the last minute with the approval flags. I'm NOT
> > sitting next to the computer all the time, and because of time zone
> > differences, and because of some travel on Dec 30, I will need some time to
> > react.
> 
> I will Pre-approve all the flags needed here.Please email me if I have
> missed an approval on any patch that you need .
> 
> Below are the branches where this patch needs to go in :
> 
> a) mozilla-beta(default) -
> https://hg.mozilla.org/releases/mozilla-beta/graph/
> b) esr 10(default) - https://hg.mozilla.org/releases/mozilla-esr10/graph
> c) esr 17(default) - https://hg.mozilla.org/releases/mozilla-esr17/graph

It will also need to land on aurora and b2g branches as "status-firefox19/status-b2g18 " are affected .(sherrifs can help with the below landings, if needed)

d) aurora(default) - https://hg.mozilla.org/releases/mozilla-aurora/graph/
e) b2g(default) - https://hg.mozilla.org/releases/mozilla-b2g18/graph/

Comment 30

[bhavana bajaj [:bajaj]]

Comment on attachment 696303 [details] [diff] [review]
Remove recently added "TÜRKTRUST Elektronik Sertifika Hizmet Sağlayıcısı" root certificate [incomplete, for review purposes]

[Triage Comment]




Please make sure to land on m-c before branch landings.

Comment 31

[bhavana bajaj [:bajaj]]

Comment on attachment 696159 [details] [diff] [review]
Distrust both currently-known-bad intermediates [correct but incomplete]

[Triage Comment]





[Triage Comment]

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 696305 [details] [diff] [review]
patch v3 - proposed complete patch for checkin - only Firefox 18 and later [checked in to upstream NSS 3.14.x and 3.13.x branches]

[Triage Comment]





[Triage Comment]

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 696312 [details] [diff] [review]
patch v4-ESR - proposed complete patch for checkin - only Firefox 17

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

When we check in this patch to CVS, will we use the tag NSSCKBI_1_93_RTM?

This patch is only upgrading *part* of nssckbi to 1.93. We should instead upgrade *all* of nssckbi to version 1.93 in Firefox 10 ESR and Firefox 17 ESR. In particular, instead of checking in the generated certdata.c, we should be checking in the updated build files that generate certdata.c from certdata.txt at build time and that remove certdata.c from version control.

In other words, instead should instead have 3 patches:

1. patch v3, above, for checking into NSS CVS.
2. The Firefox patch that is the result of "python client.py update_nssckbi NSSCKBI_1_93_RTM" for m-c, m-a, and m-b.
3. The Firefox patch that is the result of "python client.py update_nssckbi NSSCKBI_1_93_RTM && hg rm security/nss/lib/ckfw/builtins/certdata.c" for ESR 10 and ESR 17.

Patches #2 and #3 should update security/coreconf/coreconf.dep and should update security/nss/TAG-INFO-CKBI.

Even then, there is a significant problem with updating the builtins package version number without updating the NSS version number: configure.in doesn't check the minimum version of the NSS builtins package; it only checks the minimum version for the NSS package. We should modify configure.in to check the version number of the system NSS builtins package if we're trying to support it being versioned separately from the rest of NSS. Note that previously some Linux distros have been tripped up by this, where they were merrily running Firefox against an old builtins package without the distrust records when we distrusted TrustWave or Diginotar (can't remember which). We should correct this in a separate patch.

Comment 34

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:bsmith) from comment #33)
> 
> When we check in this patch to CVS, will we use the tag NSSCKBI_1_93_RTM?

Yes, it was my intention to tag directory mozilla/security/nss/lib/ckfw/builtins with that tag.


> This patch is only upgrading *part* of nssckbi to 1.93. We should instead
> upgrade *all* of nssckbi to version 1.93 in Firefox 10 ESR and Firefox 17
> ESR. In particular, instead of checking in the generated certdata.c, we
> should be checking in the updated build files that generate certdata.c from
> certdata.txt at build time and that remove certdata.c from version control.

I disagree. Those changes haven't been landed yet on the NSS 3.13.x branch and are untested on that branch. They aren't necessary to fix this bug.


> In other words, instead should instead have 3 patches:
> 
> 1. patch v3, above, for checking into NSS CVS.
> 2. The Firefox patch that is the result of "python client.py update_nssckbi
> NSSCKBI_1_93_RTM" for m-c, m-a, and m-b.
> 3. The Firefox patch that is the result of "python client.py update_nssckbi
> NSSCKBI_1_93_RTM && hg rm security/nss/lib/ckfw/builtins/certdata.c" for ESR
> 10 and ESR 17.
> 
> Patches #2 and #3 should update security/coreconf/coreconf.dep and should
> update security/nss/TAG-INFO-CKBI.

Yes, I had intended to update this not-part-of-the-build-file with the tag information as part of the landing.


> Even then, there is a significant problem with updating the builtins package
> version number without updating the NSS version number: configure.in doesn't
> check the minimum version of the NSS builtins package; it only checks the
> minimum version for the NSS package. We should modify configure.in to check
> the version number of the system NSS builtins package if we're trying to
> support it being versioned separately from the rest of NSS. Note that
> previously some Linux distros have been tripped up by this, where they were
> merrily running Firefox against an old builtins package without the distrust
> records when we distrusted TrustWave or Diginotar (can't remember which). We
> should correct this in a separate patch.

In my opinion, this isn't the right time to fix that problem. Making configure smarter shouldn't be part of a chemspill. I agree it's a good idea and it should be done, but in a separate bug, such work shouldn't hold up this fix. Yes, we have released CKBI-only updates in the past. Whenever we did, we have also released new tarballs that combined latest NSS with latest CKBI, and I intend to do so this time, too. Our release notes that can clearly indicate that source code consumers must pick up the updated NSS package, if they build nss independently.

Comment 35

[Kai Engert (:KaiE:)]

Given that Brian requests a more complicated approach, unnecessarily, I cannot yet land into the ESR-10 and ESR-17 branches.

Note that we have used the simpler approach in the past (update only the root list without updating the rest of the root module).

Assuming that Brian is away most of the time:

If you want to support my proposal to go the simpler approach, then please give ESR-10+ and ESR-17+ approval to bug 795355 attachment 670464 [details] [diff] [review].

That will enable me to land the approved patch v3 inside this bug on the ESR branches, too.

Comment 36

[Kai Engert (:KaiE:)]

Comment on attachment 696305 [details] [diff] [review]
patch v3 - proposed complete patch for checkin - only Firefox 18 and later [checked in to upstream NSS 3.14.x and 3.13.x branches]

Checked in to NSS trunk (will be contained in a future NSS 3.14.2 release).

certdata.txt cvs file version 1.87
nssckbi.h cvs file version 1.39

Tagged trunk directory mozilla/security/nss/lib/ckfw/builtins with NSS_3_14_CKBI_1_93_RTM for separate pickup.



Also checked in to NSS_3_13_4_BRANCH (would be contained in a potential future NSS 3.13.7 release).

Checking in certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/Attic/certdata.c,v  <--  certdata.c
new revision: 1.85.2.4; previous revision: 1.85.2.3
done
Checking in certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.82.2.4; previous revision: 1.82.2.3
done
Checking in nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.35.2.3; previous revision: 1.35.2.2
done


Tagged NSS branch directory mozilla/security/nss/lib/ckfw/builtins with NSS_3_13_CKBI_1_93_RTM for separate pickup.

Comment 37

[Kai Engert (:KaiE:)]

Now that I've tagged things in NSS CVS, here are simpler landing instructions.

For FF 18 and later:
  No patch necessary.
  execute:
    python update_nssckbi NSS_3_14_CKBI_1_93_RTM
  commit
    (diff looks similar to patch v3, plus one static file with the name of the tag)

For ESR-17:
  No patch necessary.
  execute
    (careful, note that this tag is different, so copy&paste from here)
    python update_nssckbi NSS_3_13_CKBI_1_93_RTM

ESR-17 landing pending the approvals I have requested in comment 35.

ESR-10: Probably the same as ESR-17, but I need a bit more time to double-check.


I will do the FF-18-and-later landings very soon (within the next 2 hours).

Comment 38

[Alex Keybl [:akeybl]]

(In reply to Kai Engert (:kaie) from comment #35)
> Assuming that Brian is away most of the time:
> 
> If you want to support my proposal to go the simpler approach, then please
> give ESR-10+ and ESR-17+ approval to bug 795355 attachment 670464 [details] [diff] [review]
> [diff] [review].
> 
> That will enable me to land the approved patch v3 inside this bug on the ESR
> branches, too.

Let's make these ESR10/17 landings on Wednesday - I'd like to hear the opinion of others before making a final decision. There are no pre-release ESR users, so the landing will be "blind" regardless (and don't require more bake time).

Comment 39

[Kai Engert (:KaiE:)]

I just learned that b2g18 still uses the same older NSS that is used on Firefox 17.

Whatever decisions you want to wait for ESR-10/ESR-17, this will also delay landing into b2g18

Comment 40

[Kai Engert (:KaiE:)]

Once again, regarding Brian's proposal, which I don't like.

ESR-10, ESR-17, B2G18 all use NSS from an older major release branch - NSS 3.13.x

I compared the changes in the directory that Brians proposes to upgrade from current NSS 3.13.x to another major release NSS 3.14.x

Changes on NSS 3.14.x:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Fckfw%2Fbuiltins%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2012-04-01&maxdate=&cvsroot=%2Fcvsroot

Changes on NSS 3.13.x:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=NSS_3_13_4_BRANCH&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Fckfw%2Fbuiltins%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2012-04-01&maxdate=&cvsroot=%2Fcvsroot

Today I have landed all of the root CAs into the stable NSS 3.13.x branch - because the patches had sufficient reviews (the patch from today, plus the batch from September). The queries reflect the state with those additions.

The only difference, the only thing missing on the older NSS 3.13.x branch, is:

(a) bug 683266, which is irrelevant for fixing this bug, and which uses code, that has never been tested together with the remainder of the NSS 3.13.x code. If you go with Brian's proposal, you get that unnecessary patch.

(b) the MPL 2 version upgrade. If you go with Brian's proposal, you will get the guts of our important change mixed up with all of those license changes, also mixed removal of 20000 autogenerated lines.

I reconfirm my request to not go Brian's path, and use my path for simplicity.

Please approve bug 795355 attachment 670464 [details] [diff] [review]
for
 - aurora
 - beta
 - b2g18

and I will be able to synchronize all branches to contain the same trust list, and that's all we need.

Thanks

Comment 41

[bhavana bajaj [:bajaj]]

(In reply to Kai Engert (:kaie) from comment #40)
> Once again, regarding Brian's proposal, which I don't like.
> 
> ESR-10, ESR-17, B2G18 all use NSS from an older major release branch - NSS
> 3.13.x
> 
> I compared the changes in the directory that Brians proposes to upgrade from
> current NSS 3.13.x to another major release NSS 3.14.x
> 
> Changes on NSS 3.14.x:
> http://bonsai.mozilla.org/cvsquery.
> cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fsecu
> rity%2Fnss%2Flib%2Fckfw%2Fbuiltins%2F&file=&filetype=match&who=&whotype=match
> &sortby=Date&hours=2&date=explicit&mindate=2012-04-
> 01&maxdate=&cvsroot=%2Fcvsroot
> 
> Changes on NSS 3.13.x:
> http://bonsai.mozilla.org/cvsquery.
> cgi?treeid=default&module=all&branch=NSS_3_13_4_BRANCH&branchtype=match&dir=m
> ozilla%2Fsecurity%2Fnss%2Flib%2Fckfw%2Fbuiltins%2F&file=&filetype=match&who=&
> whotype=match&sortby=Date&hours=2&date=explicit&mindate=2012-04-
> 01&maxdate=&cvsroot=%2Fcvsroot
> 
> Today I have landed all of the root CAs into the stable NSS 3.13.x branch -
> because the patches had sufficient reviews (the patch from today, plus the
> batch from September). The queries reflect the state with those additions.
> 
> The only difference, the only thing missing on the older NSS 3.13.x branch,
> is:
> 
> (a) bug 683266, which is irrelevant for fixing this bug, and which uses
> code, that has never been tested together with the remainder of the NSS
> 3.13.x code. If you go with Brian's proposal, you get that unnecessary patch.
> 
> (b) the MPL 2 version upgrade. If you go with Brian's proposal, you will get
> the guts of our important change mixed up with all of those license changes,
> also mixed removal of 20000 autogenerated lines.
> 
> I reconfirm my request to not go Brian's path, and use my path for
> simplicity.
> 
> Please approve bug 795355 attachment 670464 [details] [diff] [review]
> for
>  - aurora
>  - beta
>  - b2g18
> 
> and I will be able to synchronize all branches to contain the same trust
> list, and that's all we need.
> 
> Thanks

Can someone from the NSS or security team please help review the latest patch , attachment 670464 [details] [diff] [review]  ?

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #39)
> I just learned that b2g18 still uses the same older NSS that is used on
> Firefox 17.
>
> ESR-10, ESR-17, B2G18 all use NSS from an older major release branch - NSS
> 3.13.x

NSS 3.14.1 was merged into mozilla-b2g18 in bug 822682 comment 15:
https://hg.mozilla.org/releases/mozilla-b2g18/file/7def9849a8a1/security/nss/TAG-INFO

> The only difference, the only thing missing on the older NSS 3.13.x branch,
> is:
> 
> (a) bug 683266, which is irrelevant for fixing this bug, and which uses
> code, that has never been tested together with the remainder of the NSS
> 3.13.x code. If you go with Brian's proposal, you get that unnecessary patch.
> 
> (b) the MPL 2 version upgrade. If you go with Brian's proposal, you will get
> the guts of our important change mixed up with all of those license changes,
> also mixed removal of 20000 autogenerated lines.

Technically aren't all my patches, including this one, licensed under MPL 2.0 since I am a MoCo employee? I honestly don't know how that works, so I was hoping we could avoid that issue.

My concern is that we'll make future maintenance on ESR 10 and ESR 17 unnecessarily more difficult by keeping it forked. And, we'd have two different versions of CKBI called version 1.93, that compile to the same binary code, but where the source code is different (but kind of the same, as the difference is just that parts are built at different times) which is confusing.

(In reply to Kai Engert (:kaie) from comment #34)
> In my opinion, this isn't the right time to fix that problem. Making
> configure smarter shouldn't be part of a chemspill. I agree it's a good idea
> and it should be done, but in a separate bug, such work shouldn't hold up
> this fix. Yes, we have released CKBI-only updates in the past. Whenever we
> did, we have also released new tarballs that combined latest NSS with latest
> CKBI, and I intend to do so this time, too. Our release notes that can
> clearly indicate that source code consumers must pick up the updated NSS
> package, if they build nss independently.

It is fine to do that in an additional patch (perhaps in a follow-up bug), especially since this patch doesn't affect how RelEng will build the edition of NSS that we distribute as part of Gecko.

Comment 43

[Kai Engert (:KaiE:)]

(In reply to bhavana bajaj [:bajaj] from comment #41)
> 
> Can someone from the NSS or security team please help review the latest
> patch , attachment 670464 [details] [diff] [review]  ?


Is is already! That was done in September.

That patch has been landed already into both NSS branches, because it has the necessary code reviews.

It simply hasn't been yet in the FF 17 and earlier branches, because nobody had asked for an update. But now that we need to upgrade to a newer version of the root certificate list, that chunk must land, too, for consistency reasons.

Comment 44

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:bsmith) from comment #42)
> (In reply to Kai Engert (:kaie) from comment #39)
> > I just learned that b2g18 still uses the same older NSS that is used on
> > Firefox 17.
> >
> > ESR-10, ESR-17, B2G18 all use NSS from an older major release branch - NSS
> > 3.13.x
> 
> NSS 3.14.1 was merged into mozilla-b2g18 in bug 822682 comment 15:
> https://hg.mozilla.org/releases/mozilla-b2g18/file/7def9849a8a1/security/nss/
> TAG-INFO

Ok, my mistake, my local b2g18 tree was stuck at some older revision.

I had to use "hg up -C" to get past it, and now I can see that b2g18 uses NSS 3.14.1

Good. That means I can handle b2g18 landing the same as mozilla-central, aurora, beta, using the existing approvals.



> Technically aren't all my patches, including this one, licensed under MPL
> 2.0 since I am a MoCo employee? I honestly don't know how that works, so I
> was hoping we could avoid that issue.

Please let's not have a discussion and run in circles. I said that patch is unnecessary. Even if you point out that the patch isn't wrong, it's still unnecessary to land that change on that branch.


> My concern is that we'll make future maintenance on ESR 10 and ESR 17
> unnecessarily more difficult by keeping it forked.

Please don't exaggerate. We aren't talking about forking. We are talking about landing some static data into an older branch and into a newer branch. That's it.

Firefox 10 will be dead anyway in 2 months.

Firefox 17 still uses NSS 3.13.6 and can be trivially fixed by landing that static data. It's not a fork. We'll have to answer that question more generally at the time when Firefox 17 needs actual code fixes during the maintenance year, and I'd expect that sooner or later we'll upgrade that to NSS 3.14.1 at some point in the future. And instead of doing a partial upgrade now, and another partial upgrade later, why not simply land the trivial sufficient change, and deal with a clean upgrade later? I don't understand why I have to fight so hard for the trival and sufficient fix.

Comment 45

[Kai Engert (:KaiE:)]

And BTW, we have done exactly the same thing in the past for a similar scenario, landing the static root list data into two branches, and creating two snapshots. It's not a problem. It's trivial.

(The example for the old tags were
 NSS_3_13_CKBI_1_88_RTM and 
 NSS_3_12_CKBI_1_88_RTM)

Comment 46

[Kai Engert (:KaiE:)]

Fixed on mozilla-central for FF 20.
https://hg.mozilla.org/mozilla-central/rev/9dc85c3d98ae

Comment 47

[Kai Engert (:KaiE:)]

Fixed on mozilla-aurora for Firefox 19
https://hg.mozilla.org/releases/mozilla-aurora/rev/3dc2182a6b50

Comment 48

[Kai Engert (:KaiE:)]

Fixed on mozilla-beta for Firefox 18
https://hg.mozilla.org/releases/mozilla-beta/rev/d5935d0242bc

Comment 49

[Kai Engert (:KaiE:)]

Fixed in mozilla-b2g18
https://hg.mozilla.org/releases/mozilla-b2g18/rev/905b696c998d

Comment 50

[Kai Engert (:KaiE:)]

also setting the fixed-20 flag, as it has been landed into mozilla-central.

Comment 51

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #40)
> Please approve bug 795355 attachment 670464 [details] [diff] [review]
> for
>  - aurora
>  - beta
>  - b2g18
> 
> and I will be able to synchronize all branches to contain the same trust
> list, and that's all we need.


And just in case it isn't clear yet.

We need that approval anyway, regardless whether you prefer my simpler approach (land only the static data) or whether you prefer Brian's approach (copy more stuff to the older branch).

Both approaches will add that patch from bug 795355 to ESR-10/ESR-17, combined with patch v3 in this bug on top of it.

Comment 52

[Kai Engert (:KaiE:)]

I've set the approval flags in bug 795355.

We need approval for ESR-10 and ESR-17.

Comment 53

[Kai Engert (:KaiE:)]

Comment on attachment 696312 [details] [diff] [review]
patch v4-ESR - proposed complete patch for checkin - only Firefox 17

Thank you for the approvals in bug 795355, this makes it possible to fix the ESR branches without requiring this patch, marking obsolete.

Comment 54

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/1992da03a607

Comment 55

[Kai Engert (:KaiE:)]

Landing on esr-10 requires some additional changes to the root CA list.
I hope you don't mind if I interpret your approval in bug 795355, which pulls in some CA certificates, to mean approval to land all missing certs.

The additional required bugs are 757189 (zero-effect cleanup) and 757197 (may 2012 batch of new CAs).

Comment 56

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/8514e65d50ec

all branches are done now, as I understand it.

Comment 57

[Matt Wobensmith [:mwobensmith][:matt:]]

I am verifying - using Brian's example in comment 8 - that the certs have been successfully distrusted. Will update bug shortly when I have confirmation.

Comment 58

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified 2012-1-2, m-c and beta.

Comment 59

[Mike Hommey [:glandium]]

I wonder, why aren't there CKA_CERT_SHA1_HASH and CKA_CERT_MD5_HASH data for these intermediates?

Comment 60

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Hommey [:glandium] from comment #59)
> I wonder, why aren't there CKA_CERT_SHA1_HASH and CKA_CERT_MD5_HASH data for
> these intermediates?

Those fields are not useful for distrust records, AFAICT.

Comment 61

[Robert Relyea]

> Those fields are not useful for distrust records, AFAICT.

To be most accurate, they are not needed. For trust, these fields guarrentee that someone can't create their own cert with the same issuer/SN as a trusted one and have it trusted.

In the distrust case, however, any cert with the given issuer/SN is either this incorrectly issued cert, or an imposter. In those cases we want to distrust all those certs, not just the original. This also allows us to create distrust records for certs that we only know from CRLs, which was the main reason we bothered to skip the hash check for distrusted certificates.

Comment 62

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified 2012-1-4, esr10, esr17 and Aurora.

Comment 63

[Al Billings [:abillings - ex-MoCo]]

Removing advisory tags since we don't normally do advisories for these as non-code related bugs.

Comment 64

[Al Billings [:abillings - ex-MoCo]]

Changing my mind based on security-group discussing and adding the CVE back. We will be doing an advisory for this issue.

Comment 65

[Daniel Veditz [:dveditz]]

unhiding per s-g discussion

Comment 66

[u279076]

Assuming you didn't mean to change all the status- flags.

Comment 67

[Al Billings [:abillings - ex-MoCo]]

Per discussion with Mitre, who issues CVE numbers, this issue does not merit its own CVE number. Because of this, I am removing it from the issue and our advisory for it.

Comment 68

[bri.eli.dun]

I don't mean to butt in but I see 4 of these certificates in my Apple Keychain Access. This bug report seems to be old but I need to know if this is okay to leave alone or if I need to delete them. I'm not sure how to package them but they are attached as a p7b bundle. I don't even know what that is.

Comment 69

[bri.eli.dun]

Attachment: Certificates.p7b

4 of these certificates in my Keychain Access on my Mac. Please advise if they need to be deleted. Thank you.

Comment 70

[u279076]

(In reply to bri.eli.dun from comment #68)
> I don't mean to butt in but I see 4 of these certificates in my Apple Keychain Access.

I'm sure someone over at support.mozilla.org would be more than happy to help you. Bugzilla isn't really the right place for this. Thanks.