917380 - (CVE-2013-6673) Extended validation root certificates are trusted even if the user has explicitly removed their trust

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Chris]

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."

Comment 1

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

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?

Comment 2

[Chris]

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.

Comment 3

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

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?

Comment 4

[Chris]

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.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Camilo Viecco (:cviecco)]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Camilo Viecco (:cviecco)]

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.

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: filter-ev-roots (v1)

Comment 10

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

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.

Comment 11

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

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.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) 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.

Comment 13

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

(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.

Comment 14

[Alex Keybl [:akeybl]]

"Not exploitable in default config" probably means not worth tracking for the upcoming release.

Comment 15

[Camilo Viecco (:cviecco)]

Attachment: filter-ev-roots (v2)

Comment 16

[Camilo Viecco (:cviecco)]

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

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

Tests would require landing 912155.

Comment 17

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

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.

Comment 18

[Camilo Viecco (:cviecco)]

Attachment: filter-ev-roots (v3)

Comment 19

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

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.

Comment 20

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

Is there any reason this bug needs to stay hidden? I don't think so.

Comment 21

[Daniel Veditz [: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?

Comment 22

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

(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.

Comment 23

[Camilo Viecco (:cviecco)]

Attachment: test (depends on 927016)

Comment 24

[Camilo Viecco (:cviecco)]

Now with tests.

Comment 25

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

clearing needinfo since the review flag is set instead.

Comment 27

[Daniel Veditz [:dveditz]]

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.

Comment 28

[Chris]

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 30

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

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.

Comment 31

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

Comment on attachment 815564 [details] [diff] [review]
filter-ev-roots (v3)

Will review when the tests get updated.

Comment 32

[Camilo Viecco (:cviecco)]

Attachment: test (v2)

Comment 33

[Camilo Viecco (:cviecco)]

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.

Comment 34

[Camilo Viecco (:cviecco)]

Attachment: test (v2.1)

cleanup pnly from v2

Comment 35

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

Clearing needinfo?brian since we already have review?brian.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 37

[Daniel Veditz [:dveditz]]

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.

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

(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)

Comment 39

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

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).

Comment 40

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

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).

Comment 41

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

(In reply to [e-mail] [: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.

Comment 42

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=fe84ae081054

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d666051d1d7
https://hg.mozilla.org/mozilla-central/rev/5af82266fa35

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate this for uplift asap in order to make our final FF26 beta.

Comment 45

[Camilo Viecco (:cviecco)]

Attachment: filter-ev-roots

Final patch in m-c

Comment 46

[Camilo Viecco (:cviecco)]

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

Comment 47

[Camilo Viecco (:cviecco)]

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.

Comment 48

[Camilo Viecco (:cviecco)]

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 49

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

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

Approving for beta and aurora based on Lukas' comments.

Comment 50

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

Thanks for taking care of this, cviecco.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/986074bde47d
https://hg.mozilla.org/releases/mozilla-beta/rev/05b552b99409

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/05b552b99409

Comment 53

[Andrei Vaida [:avaida]]

We successfully verified the fix for this bug using the certificates available for the following websites: https://etherpad.mozilla.org/Fx26b10-TLS-SSL.

Comment 54

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

Based on comment 38 and comment 41, did we intend this to go into 24.2.0esr or not?

Comment 55

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

IOW, I expected to see the status flag for esr24 set to fixed.

Comment 56

[Lukas Blakk [:lsblakk] use ?needinfo]

Camilo can you nominate this patch for ESR24 branch uplift asap (hopefully this patch applies cleanly to that branch).

Comment 57

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 58

[Camilo Viecco (:cviecco)]

Thank you lukas. Got interrupted on my way to write the request :)

Comment 59

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 60

[Camilo Viecco (:cviecco)]

RyanVM. I am sorry about this. I missed the bustage and went to sleep. Will not happen again. Thanks for the backout.

Comment 61

[Cameron Kaiser [:spectre]]

Is this shipping for 24.2.0?

Comment 62

[Camilo Viecco (:cviecco)]

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.

Comment 63

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

So did this issue make it onto ESR24 or has it missed the release because of the backout?

Comment 64

[Daniel Veditz [:dveditz]]

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.

Comment 65

[Daniel Veditz [:dveditz]]

actually sounds like we haven't done build 2 for esr yet, there's a chance

Comment 66

[Nick Thomas [:nthomas] (UTC+12)]

This relanded at http://hg.mozilla.org/releases/mozilla-esr24/rev/9ee70aadb3fd, right ?

Comment 67

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

(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.

Comment 68

[Daniel Veditz [:dveditz]]

Looks like this did re-land on ESR-24
http://hg.mozilla.org/releases/mozilla-esr24/rev/9ee70aadb3fd

Comment 69

[Andrei Vaida [:avaida]]

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