Bug 917380 (CVE-2013-6673)

Extended validation root certificates are trusted even if the user has explicitly removed their trust

VERIFIED FIXED in Firefox 26

Status

()

defect
P1
major
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: xiasijie12345, Assigned: cviecco)

Tracking

({regression, sec-moderate})

26 Branch
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox24 wontfix, firefox25- wontfix, firefox26+ verified, firefox27+ verified, firefox28+ verified, firefox-esr17 unaffected, firefox-esr2426+ verified, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

Details

(Whiteboard: [adv-main26+][adv-esr24.2+] Doesn't affect most users, high for those under targeted attack)

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (Stop!Hacker!; rv:24.0) Gecko/20130524 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130917030214

Steps to reproduce:

This is in firefox nightly build 26.0a1. I tried to open a website that has a secure connection. However, something strange happened. 


Actual results:

When I tried to open a website signed by Entrust, Inc. Although I edit the CA settings in the certificate manager that this certificate is not trusted, the connection will still go through without showing “The connection is untrusted". Also, when I opened the certificate manager and change a CA settings, I closed it, firefox will crash after I reopen certificate manager.

Test Website: https://order.dominos.com/en/pages/order/menu.jsp#/menu/category/all/
With "Entrust Inc" Root certificate untrusted, the certificate is still trusted.


Expected results:

It should appear "The connection is untrusted, because the root certificate is untrusted."
Reporter

Updated

6 years ago
Severity: normal → critical
Component: Untriaged → Security
OS: Other → Windows XP
Chris, it appears that you have two issues here.

One: after explicitly untrusting a cert, the content loads anyway.
Two: crash after reopening the cert manager.

For the first one, try clearing your browser cache, restarting browser, and see if the cert exception dialog shows.

For the second, we'd need more explicit steps plus a crash log. If you get it to crash again, go to about:crashes and paste the URL of the crash here.

Lastly, in your user agent string, what does "Stop! Hacker!" mean?
Reporter

Comment 2

6 years ago
Hi Matt,
I tried the first step, after I cleared the cache, cookies and check all options to clear, but the website still loads. The two certificates I encountered is "COMODO Certification Authority" and another one is Entrust when I try to log in to Dominos.com online ordering system. The trust settings of the certificates do nothing on the secure connections.. (Also, it seems that the root certificate updates itself every time I open the browser)
For the second, I already submitted the crash report through the browser, hope it will fix soon.

Thanks.
Is this new behavior for Firefox 26 only? Or does it happen in other branches as well?

Also, is this just on one web site, or on any site that you alter the CA settings?
Reporter

Comment 4

6 years ago
From the tests I made, I upgraded to firefox 27.0a1 today, I still encounter this problem.
I have not noticed about this problem while in firefox 25.0a1 and before..
I think there are some certificates or websites have this problem, but I believe not all of them.
I can reproduce this -- removed trust from all Entrust roots and the domino page still loads. Even tried clearing the cache and restarting the browser.

If we're really failing to check the trust settings on roots that would be really bad. It's possible that the Entrust root is cross-signed, and in that case this is not a problem at all. There's no way for users to mark roots as "bad", simply as no known or trusted. The browser will try to find another way to construct a trusted path and it's possible that's what has happened here. Our UI, however, shows the untrusted path so there's AT LEAST a UI bug.

Or, as first mentioned, a really bad trust failure if Entrust is not cross-signed.
Flags: needinfo?(cviecco)
Assignee

Comment 6

6 years ago
I think I know. This happens on ev verification as the code to get the ev roots does not check for the trust flags of the certs.
I believe this is affecting release. This is a side effect of the centralize certificate verification (only eval EV once).
http://mxr.mozilla.org/mozilla-central/ident?i=getRootsForOid does not check for the trust flags of the roots. 

This is my initial diagnosis. Writing test patch now.
Flags: needinfo?(cviecco)
Blocks: 813418
Component: Security → Security: PSM
Keywords: regression
Product: Firefox → Core
Camilo: so this bug affects certs which
 - have the EV OID
 - are correctly signed, not expired, etc
 - chain up to a built-in root
 - that we have granted EV powers to
 - but that the user has removed the SSL trust-bit from

Are all of those conditions required? What if the server supplies a full chain including the root (as dominos appears to)? Is revocation checked/enforced on the EE and intermediate certs?

What if the user has installed their own root (corporate or whatever)? I suppose that wouldn't be allowed for EV, but would we accept it anyway? (that is, assume it's valid but simply not grant the EV UI at the end)

If it has to be a built-in EV-trusted root then this is probably only sec-moderate. Not nice to ignore the user's lack of trust but at least Mozilla trusts the root. If it's worse than that this is probably sec-high.
Assignee: nobody → cviecco
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty?
Flags: needinfo?(cviecco)
Assignee

Comment 8

6 years ago
Thu(In reply to Daniel Veditz [:dveditz] from comment #7)
> Camilo: so this bug affects certs which
>  - have the EV OID
>  - are correctly signed, not expired, etc
>  - chain up to a built-in root
>  - that we have granted EV powers to
>  - but that the user has removed the SSL trust-bit from
Removed trust bits from the EV root.
> 
> Are all of those conditions required?
Yes.
 What if the server supplies a full
> chain including the root (as dominos appears to)?
 Is revocation
> checked/enforced on the EE and intermediate certs?
Yes as it is EV.



> What if the user has installed their own root (corporate or whatever)? 

This only affects built in roots.
Flags: needinfo?(cviecco)
Assignee

Comment 9

6 years ago
Posted patch filter-ev-roots (v1) (obsolete) — Splinter Review
Assignee

Updated

6 years ago
Attachment #813317 - Flags: review?(brian)
I agree with Dan's assessment of the severity in comment 7. This bug is basically "Firefox runs in its default configuration even when I have tried to configure it to run in a non-default configuration." Badness, and it should get uplifted to mozilla-beta so it appears in the next release, but trusting this root *is* the default things to do. So, I suggest we don't rate the bug sec-anything and open the bug up to the public as it isn't exploitable. Since Camilo is working on a fix, we may want wait to do that until we've tested Camilo's fix to ensure that we've diagnosed the issue correctly.
Severity: critical → major
OS: Windows XP → All
Priority: -- → P1
Hardware: Other → All
Target Milestone: --- → mozilla27
Comment on attachment 813317 [details] [diff] [review]
filter-ev-roots (v1)

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

I think this is the right idea.

Please put the duplicate code into a function that gets used twice. Document GetRootsForOid is responsible for verifying that the given roots should really be trusted. Also, include a test.

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1085,5 @@
> +    if (entry.oid_tag == oid_tag){
> +      if (entry.cert->trust && entry.cert->trust->sslFlags == 0) {
> +        CERT_AddCertToListTail(certList, CERT_DupCertificate(entry.cert));
> +      }
> +    }

Use CERT_GetCertTrust. Reading the definition of CERT_GetCertTrust shows that it is acquiring/releasing the cert trust lock so presumably that is important to do.

See http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/certvfy.c?rev=06e3d7e19a09#532 for an example of how to use CERT_GetCertTrust.
Attachment #813317 - Flags: review?(brian) → review-
Summary: Security Certificate → Extended validation root certificates are trusted even if the user has explicitly removed their trust
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #10)
> trusting this root *is* the default things to do. So, I suggest we don't
> rate the bug sec-anything and open the bug up to the public as it isn't
> exploitable.

I agree it's not severe for the vast majority of our users, but for those who do remove trust bits we don't know why or how important it is that those decisions be honored. FirefoxOS, for example, relies on the removal of code-signing trust to make the marketplace work correctly. If a bug like this had affected that it would be pretty bad, whose to say some subset of users haven't created some derivative product or installation that relies on having only a single or small number of trusted SSL roots active?

I don't want to over-rate this bug, but we do need to treat it as a security bug.
Keywords: sec-moderate
Whiteboard: Not exploitable in default config
(In reply to Daniel Veditz [:dveditz] from comment #12)
> I agree it's not severe for the vast majority of our users, but for those
> who do remove trust bits we don't know why or how important it is that those
> decisions be honored. FirefoxOS, for example, relies on the removal of
> code-signing trust to make the marketplace work correctly. If a bug like
> this had affected that it would be pretty bad, whose to say some subset of
> users haven't created some derivative product or installation that relies on
> having only a single or small number of trusted SSL roots active?

My understanding is that our ratings are for our products only. If somebody builds a derivative product then their security team needs to rate all our bugs. Often they can use our ratings, but not always.

> I don't want to over-rate this bug, but we do need to treat it as a security
> bug.

Sure. I think it is a security bug, not not a very severe one.

My own recommendation would be not to rate it, because this trust configuration is what we ship as the default because we believe the browser is as secure as we intend it to be this way. All that said, I know neither of us want to debate severity ratings too much so I am happy to defer to you about the actual rating.
"Not exploitable in default config" probably means not worth tracking for the upcoming release.
Assignee

Comment 15

6 years ago
Posted patch filter-ev-roots (v2) (obsolete) — Splinter Review
Attachment #813317 - Attachment is obsolete: true
Assignee

Comment 16

6 years ago
Comment on attachment 814390 [details] [diff] [review]
filter-ev-roots (v2)

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

Tests would require landing 912155.
Attachment #814390 - Flags: review?(brian)
Comment on attachment 814390 [details] [diff] [review]
filter-ev-roots (v2)

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

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1019,5 @@
>    return false;
>  }
>  
> +static void
> +addToCertListIfTrusted(CERTCertList* certList, CERTCertificate *cert){

whitespace.

@@ +1027,5 @@
> +  if(srv != SECSuccess) {
> +    return;
> +  }
> +  nsNSSCertTrust trust(&nsstrust);
> +  if (trust.HasTrustedCA(true, false, false)) {

It is hard to understand what nsNSSCertTrust does and nsNSSCertTrust seems overcomplicated and too indirect.

Here is the code that I'm currently using in NSSCertDBTrustDomain:

  // XXX: CERT_GetCertTrust's result is ambiguous. Does SECFailure mean that
  // it failed, or that there is no trust record?
  CERTCertTrust trust;
  if (CERT_GetCertTrust(candidateCert, &trust) == SECSuccess) {
    PRUint32 flags = SEC_GET_TRUST_FLAGS(&trust, mCertDBTrustType);

    // For DISTRUST, we use the CERTDB_TRUSTED or CERTDB_TRUSTED_CA bit,
    // because we can have active distrust for either type of cert.
    PRUint32 relevantTrustBit = endEntityOrCA == MustBeCA ? CERTDB_TRUSTED_CA
                                                          : CERTDB_TRUSTED;
    if (((flags & (relevantTrustBit|CERTDB_TERMINAL_RECORD)))
            == CERTDB_TERMINAL_RECORD) {
      *trustLevel = ActivelyDistrusted;
      return SECSuccess;
    }

    // For TRUST, we only use the CERTDB_TRUSTED_CA bit, because Gecko hasn't
    // needed to consider end-entity certs to be their own trust anchors since
    // Gecko implemented nsICertOverrideService.
    if (flags & CERTDB_TRUSTED_CA) {
      *trustLevel = TrustAnchor;
      return SECSuccess;
    }
  }

It seems like it would be better to use the exact same logic in both places.
Attachment #814390 - Flags: review?(brian) → review-
Assignee

Comment 18

6 years ago
Posted patch filter-ev-roots (v3) (obsolete) — Splinter Review
Attachment #814390 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #815564 - Flags: review?(brian)
Comment on attachment 815564 [details] [diff] [review]
filter-ev-roots (v3)

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

Please re-request review when there is a test.
Attachment #815564 - Flags: review?(brian)
Is there any reason this bug needs to stay hidden? I don't think so.
Flags: needinfo?(dveditz)
No, I guess not. In fact since the only people at risk are the people who changed their trust settings perhaps we should issue an advisory for them now so they can take steps to protect themselves. Not sure what steps those would be, though. Drop back to ESR 17 for the last supported release until this is fixed in Fx26? Is there a pref that turns off EV handling so these certs would be validated through the normal path?
Flags: needinfo?(dveditz)
Whiteboard: Not exploitable in default config → Not exploitable in default config, still needs advisory
(In reply to Daniel Veditz [:dveditz] from comment #21)
> No, I guess not. In fact since the only people at risk are the people who
> changed their trust settings perhaps we should issue an advisory for them
> now so they can take steps to protect themselves. Not sure what steps those
> would be, though. Drop back to ESR 17 for the last supported release until
> this is fixed in Fx26?

If we think advice like this is warranted then we should just respin Fx26. If we don't think it is worth respinning Fx26 then I don't think advising users to do something so drastic is a good idea.

> Is there a pref that turns off EV handling so these
> certs would be validated through the normal path?

No.
Group: core-security
Assignee

Updated

6 years ago
Depends on: 927016
No longer depends on: 912155
Assignee

Comment 23

6 years ago
Posted patch test (depends on 927016) (obsolete) — Splinter Review
Assignee

Comment 24

6 years ago
Now with tests.
Flags: needinfo?(brian)
Assignee

Updated

6 years ago
Attachment #815564 - Flags: review?(brian)
Assignee

Updated

6 years ago
Attachment #823672 - Flags: review?(brian)
clearing needinfo since the review flag is set instead.
Flags: needinfo?(brian)
Flags: sec-bounty? → sec-bounty-
Flags: sec-bounty- → sec-bounty+
Brian points out that we were going to be making more use of PKIX validation in the future which would make this impact more users and that the reporter does deserve a security thank-you.
Reporter

Comment 28

6 years ago
Based on my point of view, I think it is vulnerable in some cases. For example, the certificate "CNNIC Root", if I have a certificate that is the EV Certificate from CNNIC Root, firefox will still trust the page even if remove the trust bits of "CNNIC Root".
Comment on attachment 823672 [details] [diff] [review]
test (depends on 927016)

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

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +277,5 @@
> +     let errorList = {};
> +     let chain = certdb.getChainForUsage(cert, usage, 0, hasEVPolicy,  errorList);
> +     do_check_null(chain);
> +     certdb.setCertTrust(evrootca, nsIX509Cert.CA_CERT, Ci.nsIX509CertDB.TRUSTED_SSL);
> +  }

See bug 927016 comment 8. I think you are going to update this patch after updating bug 927016.

I would like to see two additions to this test:

1. Test what happens when you verify the cert after you call setCertTrust(evrootca, nsIX509Cert.CA_CERT, Ci.nsIX509CertDB.TRUSTED_SSL);

2. Test that we get the expected (different) results in the !isDebugBuild case.
Attachment #823672 - Flags: review?(brian)
Comment on attachment 815564 [details] [diff] [review]
filter-ev-roots (v3)

Will review when the tests get updated.
Attachment #815564 - Flags: review?(brian)
Assignee

Comment 32

6 years ago
Posted patch test (v2) (obsolete) — Splinter Review
Attachment #823672 - Attachment is obsolete: true
Assignee

Comment 33

6 years ago
Comment on attachment 8336269 [details] [diff] [review]
test (v2)

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

(noticed now that the const and the eq(0,error) should be moved
to the other bug.
Attachment #8336269 - Flags: review?(brian)
Assignee

Updated

6 years ago
Attachment #8336269 - Flags: review?(brian)
Assignee

Comment 34

6 years ago
Posted patch test (v2.1)Splinter Review
cleanup pnly from v2
Attachment #8336269 - Attachment is obsolete: true
Attachment #8336480 - Flags: review?(brian)
Assignee

Updated

6 years ago
Flags: needinfo?(brian)
Clearing needinfo?brian since we already have review?brian.
Flags: needinfo?(brian)
Not sure what's happened to the flags here but this isn't a tracking-worthy bug at this point, it's also too late for uplift to FF26 as a non sec-critical bug, and I'm setting esr24 nomination back to ? but at sec-moderate this doesn't meet the criteria for ESR so if it's to land there we'd need to hear why an exception should be made for this fix.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
I'd ask you to reconsider taking this on older branches. It's a small uncomplicated patch. We have rated this "moderate" on the basis that most users don't actually use these controls or have a need to make different trust decisions than we do for our root program. But for the users who _do_ this is a sec-high or critical "abandon ship" kind of bug.

Go read the voluminous newsgroups and bug comments about adding CNNIC's root. "opt out by removing CNNIC trust yourself" was our answer at the time. This has press-incident written all over it if we don't address it in a reasonable time frame.

Users have been broken since Firefox 23 (Aug 6). If this misses Firefox 26 it leaves those users unprotected for 6 months until Firefox 27.

On the ESR this functionality is even more important because making trust decisions for their organization is exactly the kind of tweak enterprises make for their users.

B2G1.2 is technically affected, but since there's no mechanism for users to remove trust bits we don't have to fix this on that branch.
Flags: needinfo?(dveditz)
Whiteboard: Not exploitable in default config, still needs advisory → Doesn't affect most users, high for those under targeted attack
Flags: needinfo?(abillings)
(In reply to Daniel Veditz [:dveditz] from comment #37)

> Users have been broken since Firefox 23 (Aug 6). If this misses Firefox 26
> it leaves those users unprotected for 6 months until Firefox 27.


That would be 6 weeks, not months, but if this is so serious to affected users we can reconsider taking it.


> On the ESR this functionality is even more important because making trust
> decisions for their organization is exactly the kind of tweak enterprises
> make for their users.

In that case we can take this along on the ESR that goes with the gecko version we ship - that can only be 26 if this gets nominated with a risk assessment and so putting a ni? on Brian to get this ready before our last FF26 Beta (next Monday)
Flags: needinfo?(brian)
Comment on attachment 8336480 [details] [diff] [review]
test (v2.1)

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

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +100,5 @@
> +function check_cert_err(cert_name, expected_error) {
> +    let cert = certdb.findCertByNickname(null, cert_name);
> +    let hasEVPolicy = {};
> +    let verifiedChain = {};
> +    let error = certdb.verifyCertNow(cert, 0x02, 0, verifiedChain, hasEVPolicy);

0x02 should be a named constant.
Does the zero mean "no flags"? If so, please add a NO_FLAGS named constant to head_psm.js.

@@ +146,5 @@
>    check_ee_for_ev("non-ev-root", false);
>    run_next_test();
>  });
>  
> +//bug 917380 //Does not check cross signed certs

Wrong bug number? (bug 917380 is this bug.)

Fix whitespace and remove redundant "//" in the middle.

@@ +148,5 @@
>  });
>  
> +//bug 917380 //Does not check cross signed certs
> +add_test(function () {
> +  const nsIX509Cert = Components.interfaces.nsIX509Cert;

NIT: Ci.

@@ +149,5 @@
>  
> +//bug 917380 //Does not check cross signed certs
> +add_test(function () {
> +  const nsIX509Cert = Components.interfaces.nsIX509Cert;
> +  let evrootca = certdb.findCertByNickname(null, evrootnick);

NIT: camelCase (evRootCA or rootCA or similar).
Attachment #8336480 - Flags: review?(brian) → review+
Comment on attachment 815564 [details] [diff] [review]
filter-ev-roots (v3)

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

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1019,5 @@
>    return false;
>  }
>  
> +static void
> +addToCertListIfTrusted(CERTCertList* certList, CERTCertificate *cert) {

brace on its own line.

@@ +1020,5 @@
>  }
>  
> +static void
> +addToCertListIfTrusted(CERTCertList* certList, CERTCertificate *cert) {
> +  CERTCertTrust nsstrust;

NIT: camelCase (nssTrust).

@@ +1022,5 @@
> +static void
> +addToCertListIfTrusted(CERTCertList* certList, CERTCertificate *cert) {
> +  CERTCertTrust nsstrust;
> +  SECStatus srv;
> +  srv = CERT_GetCertTrust(cert, &nsstrust);

NIT: you can do away with the srv variable:
  if GERT_GetCertTrust(cert, &nssTrust) != SECSuccess) {

@@ +1023,5 @@
> +addToCertListIfTrusted(CERTCertList* certList, CERTCertificate *cert) {
> +  CERTCertTrust nsstrust;
> +  SECStatus srv;
> +  srv = CERT_GetCertTrust(cert, &nsstrust);
> +  if(srv != SECSuccess) {

space after if.

@@ +1026,5 @@
> +  srv = CERT_GetCertTrust(cert, &nsstrust);
> +  if(srv != SECSuccess) {
> +    return;
> +  }
> +  PRUint32 flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustSSL);

unsigned int. (See the definition of SEC_GET_TRUST_FLAGS).
Attachment #815564 - Flags: review+
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #38)
> In that case we can take this along on the ESR that goes with the gecko
> version we ship - that can only be 26 if this gets nominated with a risk
> assessment and so putting a ni? on Brian to get this ready before our last
> FF26 Beta (next Monday)

Let's land this (with the test) today. Then cviecco can do the uplift request. I believe this is very low risk and also I think it is important to uplift to all affected products.
Flags: needinfo?(brian)
Please nominate this for uplift asap in order to make our final FF26 beta.
Flags: needinfo?(cviecco)
Flags: needinfo?(brian)
Assignee

Comment 45

6 years ago
Final patch in m-c
Attachment #815564 - Attachment is obsolete: true
Attachment #8341134 - Flags: review+
Assignee

Comment 46

6 years ago
Comment on attachment 8341134 [details] [diff] [review]
filter-ev-roots

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 813418 
User impact if declined: Users that explicitly disable some EV roots will still be using those certificates for verification.
Testing completed (on m-c, etc.): Landed on M-C on Nov-27 with tests.
Risk to taking this patch (and alternatives if risky):  None that I can think.
String or IDL/UUID changes made by this patch: None
Attachment #8341134 - Flags: approval-mozilla-beta?
Attachment #8341134 - Flags: approval-mozilla-aurora?
Assignee

Comment 47

6 years ago
This has a test suite. I have requested the uplift of the fix and not the tests as the tests have a set of dependencies that we do no need to uplift.
Flags: needinfo?(cviecco)
Assignee

Comment 48

6 years ago
Forgot: In absence of tests. Manual testing is: 
1. go to any EV enable site (such as bugzilla). 
2. Verify you get EV
3. go to the side details and get the certiicate chain
4. Note the name of the signing root.
5. Go to the certificate manager and set that root as untrusted.
6. restart FF, goto site on step 1. You should get a failed connection or a non-ev connection (non-ev will be caused by cross signing of root certs, ie the root you chain is different, but it will still travese a cert with the same public key info).
Comment on attachment 8341134 [details] [diff] [review]
filter-ev-roots

Approving for beta and aurora based on Lukas' comments.
Attachment #8341134 - Flags: approval-mozilla-beta?
Attachment #8341134 - Flags: approval-mozilla-beta+
Attachment #8341134 - Flags: approval-mozilla-aurora?
Attachment #8341134 - Flags: approval-mozilla-aurora+
Thanks for taking care of this, cviecco.
Flags: needinfo?(brian)
Target Milestone: mozilla27 → mozilla28
Keywords: verifyme
Target Milestone: mozilla28 → mozilla27
Target Milestone: mozilla27 → mozilla28
We successfully verified the fix for this bug using the certificates available for the following websites: https://etherpad.mozilla.org/Fx26b10-TLS-SSL.
Whiteboard: Doesn't affect most users, high for those under targeted attack → [adv-main26+][adv-esr24.2+] Doesn't affect most users, high for those under targeted attack
Alias: CVE-2013-6673
Based on comment 38 and comment 41, did we intend this to go into 24.2.0esr or not?
IOW, I expected to see the status flag for esr24 set to fixed.
Camilo can you nominate this patch for ESR24 branch uplift asap (hopefully this patch applies cleanly to that branch).
Flags: needinfo?(cviecco)
Comment on attachment 8341134 [details] [diff] [review]
filter-ev-roots

Actually, preemptively approving for ESR24, sheriff can put branch-patch-needed in if that's required.
Attachment #8341134 - Flags: approval-mozilla-esr24+
Assignee

Comment 58

6 years ago
Thank you lukas. Got interrupted on my way to write the request :)
Flags: needinfo?(cviecco)
Camilo landed this on esr24. I've backed it out for debug bustage.
https://hg.mozilla.org/releases/mozilla-esr24/rev/ffd35a39ebc0

https://tbpl.mozilla.org/php/getParsedLog.php?id=31463950&tree=Mozilla-Esr24

Camilo - tree rules state that *you* are to watch your pushes to release branches. This backout is coming 13 hours after the fact *and* with another push on top of it.

Brian - tree rules state that you aren't to land on top of bustage. If you needed to push the NSS update, you should have backed Camilo out first.
Flags: needinfo?(cviecco)
Flags: needinfo?(brian)
Assignee

Comment 60

6 years ago
RyanVM. I am sorry about this. I missed the bustage and went to sleep. Will not happen again. Thanks for the backout.
Flags: needinfo?(cviecco)
Is this shipping for 24.2.0?
Assignee

Comment 62

6 years ago
The issue mas a missing ';' that I missed when my patch did not 'qpushed' cleanly and that unfortunately did not complained on my local(linux) box. I remade the patch and pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=ba9a3e92c3bf. And I am waitin to ensure I dont make the same mislanding again. I dont think will be an issue to land the new version on esr24 but I dont want to break the build again for a similar issue.
So did this issue make it onto ESR24 or has it missed the release because of the backout?
Whiteboard: [adv-main26+][adv-esr24.2+] Doesn't affect most users, high for those under targeted attack → [adv-main26+] Doesn't affect most users, high for those under targeted attack
I doubt we will convince anyone to respin ESR 24.2 again at this point, but please land when it's ready just in case. And if you do please send us mail so we can put it back in the advisory.
actually sounds like we haven't done build 2 for esr yet, there's a chance
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #59)
> Brian - tree rules state that you aren't to land on top of bustage. If you
> needed to push the NSS update, you should have backed Camilo out first.

I agree. I simply didn't notice the bustage. Too used to mozilla-inbound.
Flags: needinfo?(brian)
Looks like this did re-land on ESR-24
http://hg.mozilla.org/releases/mozilla-esr24/rev/9ee70aadb3fd
Whiteboard: [adv-main26+] Doesn't affect most users, high for those under targeted attack → [adv-main26+][adv-esr24.2+] Doesn't affect most users, high for those under targeted attack
I was able to verify the fix on Windows 7 x64, using the instructions provided by Camilo in Comment 48 with:
- Firefox 27.0b1 (BuildID: 20131209204824): Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
- Aurora 28.0a2 (BuildID: 20131211004000): Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
- ESR 24.2.0 (BuildID: 20131205180928): Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.