1278605 - "@mozilla.org/security/certoverride;1" can no longer be implemented in JavaScript

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Selenium webdriver will totally break when 47 ships: it just causes the browser to crash on startup.

Selenium overrides the "@mozilla.org/security/certoverride;1" contract.  As of bug 1201437, this service is used directly from HandshakeCallback in security/manager/ssl/nsNSSCallbacks.cpp, which is called on random threads.  This tries to instantiate a JS component on a random thread, which at best crashes and at worst corrupts memory... and then hopefully still crashes.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Seems like nsICertOverrideService needs to be annotated as "builtinclass", but that won't entirely fix Selenium's issue. Any idea why they're overriding the default implementation?

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello David, this is a potentially critical issue for Fx47. I don't fully understand the implication or the fix. We can consider including this as a dot-release ride-along but it is unclear to me whether this needs to drive a dot release. Please let me know. Thanks!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Annotating as builtinclass will fix the crash (probably; would need to check), but leave Selenium broken, I believe.

They're overriding the default implementation so they can force Firefox to accept certain invalid certs as valid for testing purposes, looks like.  See https://github.com/SeleniumHQ/selenium/blob/master/javascript/firefox-driver/js/badCertListener.js and its history.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

Review commit: https://reviewboard.mozilla.org/r/58266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58266/

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

In the interests of not breaking Selenium or anything else that has implemented something similar, attachment 8760862 [details] is an actual fix that should maintain the functionality from bug 1201437 while not crashing when nsICertOverrideService is implemented in JS. The changes are not very intrusive, so I think this is a fairly safe patch.

Comment 6

[:Cykesiopka]

Comment on attachment 8760862 [details]
bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

https://reviewboard.mozilla.org/r/58266/#review55186

Looks good.

::: security/manager/ssl/nsNSSCallbacks.cpp:1239
(Diff revision 1)
> -    bool haveOverride;
> -    uint32_t overrideBits = 0; // Unused.
> -    bool isTemporaryOverride; // Unused.
> +  Unused << status->GetIsDomainMismatch(&domainMismatch);
> +  Unused << status->GetIsUntrusted(&untrusted);
> +  Unused << status->GetIsNotValidAtThisTime(&notValidAtThisTime);

This file should have an #include "mozilla/unused.h" for these.

::: security/manager/ssl/tests/unit/test_js_cert_override_service.js:2
(Diff revision 1)
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.

I believe new test files should use Public Domain.

::: security/manager/ssl/tests/unit/test_js_cert_override_service.js:14
(Diff revision 1)
> +// a specific host ("expired.example.com") has a matching override (ERROR_TIME).
> +// Connections to that host should succeed.
> +
> +// Mock implementation of nsICertOverrideService
> +const gCertOverrideService = {
> +  rememberValidityOverride: function(){},

Nit: Looks like we can use the nicer ES6 object method syntax here:
> rememberValidityOverride() {},

::: security/manager/ssl/tests/unit/test_js_cert_override_service.js:14
(Diff revision 1)
> +  rememberValidityOverride: function(){},
> +  rememberTemporaryValidityOverrideUsingFingerprint: function(){},

ESLint complains about these lines - see your try push.

::: security/manager/ssl/tests/unit/test_js_cert_override_service.js:18
(Diff revision 1)
> +const gCertOverrideService = {
> +  rememberValidityOverride: function(){},
> +  rememberTemporaryValidityOverrideUsingFingerprint: function(){},
> +
> +  hasMatchingOverride: function(hostname, port, cert, overrideBits,
> +                                isTemporary) {

Nit: This should probably be set just so the JS implementation is clearly correct.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/58266/#review55186

Thanks for the quick review!

> This file should have an #include "mozilla/unused.h" for these.

Ok - added.

> I believe new test files should use Public Domain.

Fixed.

> Nit: Looks like we can use the nicer ES6 object method syntax here:
> > rememberValidityOverride() {},

Sounds good.

> ESLint complains about these lines - see your try push.

Fixed.

> Nit: This should probably be set just so the JS implementation is clearly correct.

Good call.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8760862 [details]
bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58266/diff/1-2/

Comment 9

[David Burns :automatedtester]

Guessing the needinfo was meant for me...

(In reply to Ritu Kothari (:ritu) from comment #2)
> Hello David, this is a potentially critical issue for Fx47. I don't fully
> understand the implication or the fix. We can consider including this as a
> dot-release ride-along but it is unclear to me whether this needs to drive a
> dot release. Please let me know. Thanks!

We can ask users to downgrade to Firefox 46 or ESR 45 or they can use Marionette as a workaround (Mn doesnt have support for visiting sites with self-signed certs)

Comment 10

[David Burns :automatedtester]

I have tried this patch with selenium and am getting a startup crash still (havent got symbols generation in mozconfig) though I built against inbound instead of release so will rebuild and retest tomorrow morning UTC

Comment 11

[David Burns :automatedtester]

if someone wanted to do this before tomorrow you can clone do

git clone https://github.com/SeleniumHQ/selenium.git
cd selenium

Edit https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/common/desired_capabilities.py#L55 and add "binary":"/full/path/to/your/firefox_binary" to the dict

run |./go test_py method=clear_test|

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a724267cef4
ensure that nsICertOverrideService can be implemented in JS r=Cykesiopka

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Relnote wording "Selenium WebDriver extension crashes Firefox on startup. Please use Marionette WebDriver extension as a workaround. For more info go to https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/WebDriver."

Hi Bz, David, does this relnote wording seem OK? I would like to add this to Fx47 release notes soon. Thanks!

Comment 14

[:Cykesiopka]

(In reply to David Burns :automatedtester from comment #11)
> if someone wanted to do this before tomorrow you can clone do
> 
> git clone https://github.com/SeleniumHQ/selenium.git
> cd selenium
> 
> Edit
> https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/
> common/desired_capabilities.py#L55 and add
> "binary":"/full/path/to/your/firefox_binary" to the dict
> 
> run |./go test_py method=clear_test|

I did this on my development Linux VM that doesn't have Firefox installed at all except for my local m-i builds, and I get this (simplified to just show general structure):
>     def _get_firefox_start_cmd(self):
>         [...]
>         if platform.system() == "Darwin":
>             [...]
>         elif platform.system() == "Windows":
>             [...]
>         elif platform.system() == 'Java' and os._name == 'nt':
>             [...]
>         else:
>             for ffname in ["firefox", "iceweasel"]:
>                 start_cmd = self.which(ffname)
>                 if start_cmd is not None:
>                     break
>             else:
>                 # couldn't find firefox on the system path
>                 raise RuntimeError("Could not find firefox in your system PATH." +
> >                   " Please specify the firefox binary location or install firefox")
> E               RuntimeError: Could not find firefox in your system PATH. Please specify the firefox binary location or install firefox
... so I'm not entirely convinced the instructions given actually cause the correct Firefox build to be run.

Anyways, I made a symbolic link so that just plain |firefox| would work as well, and this was what I got:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a724267cef4: Firefox launches, and I see it navigates to a test page at least.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c039c5822c2 (rev just before the one above): Firefox doesn't launch at all and I get: "WebDriverException: Message: Can't load the profile." etc.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Relnote wording "Selenium WebDriver extension crashes Firefox on startup.
> Please use Marionette WebDriver extension as a workaround. For more info go
> to https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/WebDriver."

Modified relnote proposal: "Selenium WebDriver may cause Firefox to crash on startup, use Marionette WebDriver instead"

Link to the MDN link for "Marionett WebDriver".

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a724267cef4

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> Modified relnote proposal: "Selenium WebDriver may cause Firefox to crash on
> startup, use Marionette WebDriver instead"
> 
> Link to the MDN link for "Marionett WebDriver".

If Marionette WebDriver is our path forward (and I think it is), we should include this "known issue" in place for a few releases.

Comment 18

[David Burns :automatedtester]

I have retested this with a better build and (after fixing some selenium bugs) we no longer crash and have the past experience. If this could be in the future dot release for 47 that would be great.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 release notes.(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> > Modified relnote proposal: "Selenium WebDriver may cause Firefox to crash on
> > startup, use Marionette WebDriver instead"
> > 
> > Link to the MDN link for "Marionett WebDriver".
> 
> If Marionette WebDriver is our path forward (and I think it is), we should
> include this "known issue" in place for a few releases.

Thanks Lawrence! I've added it to Fx47 release notes.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8760862 [details]
bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

Approval Request Comment
[Feature/regressing bug #]: bug 1201437
[User impact if declined]: Selenium webdriver crashes Firefox
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: low - the change is self-contained, the feature has tests, and a new test was added to make sure the issue has been addressed
[String/UUID change made/needed]: none

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8760862 [details]
bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

Fix for major regression, please uplift to aurora and beta. 
We should verify this on beta once it lands.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a585c67e88e
https://hg.mozilla.org/releases/mozilla-beta/rev/2539cf53e85d

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Can someone verify that it is fixed? 
Especially on beta as we are considering to take it in a potential dot release.
David, can you help?

Comment 29

[David Burns :automatedtester]

(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> Can someone verify that it is fixed? 
> Especially on beta as we are considering to take it in a potential dot
> release.
> David, can you help?

This is working well, no more crashes!

Comment 30

[Paul Silaghi, QA [:pauly]]

(In reply to David Burns :automatedtester from comment #29)
> This is working well, no more crashes!
On what version of Firefox did you try?
I'm not able to run a simple webdriver with junit test on FX 48b2.
" org.openqa.selenium.firefox.NotConnectedException: Unable to connect to host 127.0.0.1 on port 7055 after 45000 ms. Firefox console output:
fd}","syncGUID":"Px1EIWLThJW1","location":"app-global","version":"48.0","type":"theme","internalName":"classic/1.0","updateURL":null,"updateKey":null,"optionsURL":null,"optionsType":null,"aboutURL":null,"icons":{"32":"icon.png","48":"icon.png"},"iconURL":null,"icon64URL":null,"defaultLocale":{"name":"Default","description":"The default theme.","creator":"Mozilla","homepageURL":null,"contributors":["Mozilla Contributors"]},"visible":true,"active":true,"userDisabled":false,"appDisabled":false,"descriptor":"C:\\Program Files (x86)\\Mozilla Firefox\\browser\\extensions\\{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi","installDate":1466452269000,"updateDate":1466452269000,"applyBackgroundUpdates":1,"skinnable":true,"size":21899,"sourceURI":null,"releaseNotesURI":null,"softDisabled":false,"foreignInstall":false,"hasBinaryComponents":false,"strictCompatibility":true,"locales":[],"targetApplications":[{"id":"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}","minVersion":"48.0","maxVersion":"48.0"}],"targetPlatforms":[],"seen":true} "

It works fine though on 50.0a1 (2016-06-21).

Comment 31

[David Burns :automatedtester]

(In reply to Paul Silaghi, QA [:pauly] from comment #30)
> (In reply to David Burns :automatedtester from comment #29)
> > This is working well, no more crashes!
> On what version of Firefox did you try?
> I'm not able to run a simple webdriver with junit test on FX 48b2.
> " org.openqa.selenium.firefox.NotConnectedException: Unable to connect to
> host 127.0.0.1 on port 7055 after 45000 ms. Firefox console output:
> fd}","syncGUID":"Px1EIWLThJW1","location":"app-global","version":"48.0",
> "type":"theme","internalName":"classic/1.0","updateURL":null,"updateKey":
> null,"optionsURL":null,"optionsType":null,"aboutURL":null,"icons":{"32":
> "icon.png","48":"icon.png"},"iconURL":null,"icon64URL":null,"defaultLocale":
> {"name":"Default","description":"The default
> theme.","creator":"Mozilla","homepageURL":null,"contributors":["Mozilla
> Contributors"]},"visible":true,"active":true,"userDisabled":false,
> "appDisabled":false,"descriptor":"C:\\Program Files (x86)\\Mozilla
> Firefox\\browser\\extensions\\{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi",
> "installDate":1466452269000,"updateDate":1466452269000,
> "applyBackgroundUpdates":1,"skinnable":true,"size":21899,"sourceURI":null,
> "releaseNotesURI":null,"softDisabled":false,"foreignInstall":false,
> "hasBinaryComponents":false,"strictCompatibility":true,"locales":[],
> "targetApplications":[{"id":"{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",
> "minVersion":"48.0","maxVersion":"48.0"}],"targetPlatforms":[],"seen":true} "
> 
> It works fine though on 50.0a1 (2016-06-21).

I was trying with 48b2 that I built locally because I don't know if there was one already available in the beta branch.

Did you get a crash or did you get a can not connect. Has something from addon signing landed again?

Comment 32

[Paul Silaghi, QA [:pauly]]

(In reply to David Burns :automatedtester from comment #31)
> Did you get a crash or did you get a can not connect.
No crash, just unable to connect

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8760862 [details]
bug 1278605 - ensure that nsICertOverrideService can be implemented in JS

We are planning to do a 47.0.1 just to address this issue. Let's uplift to moz-release.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/5e4a96d2062b

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

David, since our team is caught up with 2 builds today, could you please confirm this is properly fixed in 47.0.1? 

https://archive.mozilla.org/pub/firefox/candidates/47.0.1-candidates/build1/

Comment 36

[user277536]

I can confirm that this is NOT fixed in 47.0.1 released via auto update today. I'm getting the same unable to connect error as Paul.

Comment 37

[Vlad Filippov (:vladikoff)]

(In reply to user277536 from comment #36)
> I can confirm that this is NOT fixed in 47.0.1 released via auto update
> today. I'm getting the same unable to connect error as Paul.

It seems like another issue where you have to disable add-on compat check with something like this:

`setPreference('extensions.checkCompatibility.47.0', false)`

Comment 38

[David Burns :automatedtester]

This has been fixed. There has to be a release from the Selenium for all users as the maxVersion doesnt allow minor versions.

Comment 39

[Cornel Ionce [:noni] [Hubs QA]]

Many thanks David! Since this fix landed into release and got verified by you, I'm assuming it's safe to mark it verified.