Closed Bug 1278605 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 + verified
firefox48 + fixed
firefox49 + fixed
relnote-firefox --- 47+
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: keeler)

References

Details

(Keywords: regression, Whiteboard: [psm-assigned])

Attachments

(1 file)

[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.
Flags: needinfo?(nhnt11)
Flags: needinfo?(dkeeler)
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?
Flags: needinfo?(dkeeler)
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!
Flags: needinfo?(david.burns)
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.
Assignee: nobody → dkeeler
Component: Security → Security: PSM
Whiteboard: [psm-assigned]
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.
Attachment #8760862 - Flags: review?(cykesiopka.bmo) → review+
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.
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 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/
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)
Flags: needinfo?(david.burns)
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
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|
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a724267cef4
ensure that nsICertOverrideService can be implemented in JS r=Cykesiopka
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!
Flags: needinfo?(dburns)
Flags: needinfo?(bzbarsky)
(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.
(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".
https://hg.mozilla.org/mozilla-central/rev/6a724267cef4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: needinfo?(bzbarsky)
(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.
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.
Flags: needinfo?(dburns)
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 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
Attachment #8760862 - Flags: approval-mozilla-release?
Attachment #8760862 - Flags: approval-mozilla-beta?
Attachment #8760862 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1278924
Duplicate of this bug: 1278969
Duplicate of this bug: 1279127
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.
Attachment #8760862 - Flags: approval-mozilla-beta?
Attachment #8760862 - Flags: approval-mozilla-beta+
Attachment #8760862 - Flags: approval-mozilla-aurora?
Attachment #8760862 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1280854
Duplicate of this bug: 1279280
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?
Flags: qe-verify+
Flags: needinfo?(dburns)
(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!
Flags: needinfo?(dburns)
(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).
Flags: needinfo?(dburns)
(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?
Flags: needinfo?(dburns)
(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 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.
Attachment #8760862 - Flags: approval-mozilla-release? → approval-mozilla-release+
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/
Flags: needinfo?(dburns)
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.
(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)`
This has been fixed. There has to be a release from the Selenium for all users as the maxVersion doesnt allow minor versions.
Flags: needinfo?(dburns)
Many thanks David! Since this fix landed into release and got verified by you, I'm assuming it's safe to mark it verified.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1281711
Duplicate of this bug: 1281420
Duplicate of this bug: 1278300
You need to log in before you can comment on or make changes to this bug.