1306801 - Remove Necko IPC security

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[(no longer active)]

This was used for app isolation on b2g.  Jason, Patrick, can we remove this now?

Comment 1

[Patrick McManus [:mcmanus]]

fine by me if its fine by jason

Comment 2

[Jason Duell]

So I'm guessing it would still be useful to enforce that all channels from the child have a valid LoadContext and/or LoadInfo.  That has caught lots of bugs in the past. :ckerschb, does that sound correct?

If so we can rip out all the appid, etc validation, but we'd keep the necko security pref (so we can enforce LoadContext for child channels in the browser, but disable the pref for xpcshell tests which don't have a loadContext).

Comment 3

[Jason Duell]

Valentin, I'm giving you this and the rest of the nukeB2G necko work (bug 1307491, bug 1307467, and probably a new bug you should file to get rid of the infrastructure we added for packaged apps).  Let me know if that's not OK.

Comment 4

[Jason Duell]

Heard back from ckerschb.

So we'll want to rip out the code here (that validates that the child actually has the appID, etc that it says it has):
  
   https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/netwerk/ipc/NeckoParent.cpp#134

But we'll keep the parts in that function where we enforce that the channel has a loadContext (unless neckosecurity is off).  Also, let's enforce that the channel has a loadInfo, and that it matches the loadContext (by calling NS_CompareLoadInfoAndLoadContext():  we might only need to do that in debug mode to save cycles?)

Finally, network.disable.ipc.security is currently pref'ed off in browser/app/profile/firefox.js.  That seems bad--we want the loadContext/Info checks to happen in Firefox.  I'm fine with changing the name of the pref, but I'm guessing we can just turn it back on (and change the comment) and see what happens.  Maybe try that before you change a lot of other code (in case it turns out that Firefox has valid use cases for not providing a loadContext for some channels).

Comment 5

[(no longer active)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #4)
> Heard back from ckerschb.
> 
> So we'll want to rip out the code here (that validates that the child
> actually has the appID, etc that it says it has):
>   
>   
> https://dxr.mozilla.org/mozilla-central/rev/
> da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/netwerk/ipc/NeckoParent.cpp#134
> 
> But we'll keep the parts in that function where we enforce that the channel
> has a loadContext (unless neckosecurity is off).  Also, let's enforce that
> the channel has a loadInfo, and that it matches the loadContext (by calling
> NS_CompareLoadInfoAndLoadContext():  we might only need to do that in debug
> mode to save cycles?)

Do we expect add-ons using Necko in the child process to all be using the AsyncOpen2 APIs now?  Cause otherwise this will kill the child process if the user has such as add-on installed.

> Finally, network.disable.ipc.security is currently pref'ed off in
> browser/app/profile/firefox.js.  That seems bad--we want the
> loadContext/Info checks to happen in Firefox.  I'm fine with changing the
> name of the pref, but I'm guessing we can just turn it back on (and change
> the comment) and see what happens.  Maybe try that before you change a lot
> of other code (in case it turns out that Firefox has valid use cases for not
> providing a loadContext for some channels).

I think trying to turn this pref on is a great idea, but I think we should try that after removing the appId checks, otherwise add-ons which are using appIds will also create such crashes.

Comment 6

[Jason Duell]

Christoph,

I seem to have some vauge memory of sicking talking about us writing some kind of wrapper that would automatically add a loadInfo to asyncOpen() calls by addons? Am I remembering that right?  Did it ever happen?

Comment 7

[Jason Duell]

> removing the appId checks, otherwise add-ons which are using appIds will 
> also create such crashes

Ehsan--so addons are going to be using appIds, and those appIDs will be different than the ones IPDL thinks the child ought to have? I'm confused.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #7)
> > removing the appId checks, otherwise add-ons which are using appIds will 
> > also create such crashes
> 
> Ehsan--so addons are going to be using appIds, and those appIDs will be
> different than the ones IPDL thinks the child ought to have? I'm confused.

We have added nsSecCheckWrapChannel, in case an addon does not implement NewChannel2(), then we wrap the channel into nsSecCheckWrapChannel which allows to attach a loadInfo. The nsSecCheckWrapChannel basically just forwards all calls to the underlying channel but returns the loadInfo from the wrapper if requested. All that happens within nsIOService:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#763

Comment 9

[(no longer active)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #7)
> > removing the appId checks, otherwise add-ons which are using appIds will 
> > also create such crashes
> 
> Ehsan--so addons are going to be using appIds, and those appIDs will be
> different than the ones IPDL thinks the child ought to have? I'm confused.

Hmm, I'm actually not 100% sure... perhaps I was barking down the wrong tree there?

Andrea has an add-on that uses appIds, so it should be easy to toggle the pref with that add-on and see what happens.  Andrea, do you mind posting a link to your add-on here?

Comment 10

[Andrea Marchesini [:baku]]

https://addons.mozilla.org/en-US/firefox/addon/priv8/

Please don't break it :) We still don't have Containers exposed to WebExtensions. See bug 1302697

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #10)
> https://addons.mozilla.org/en-US/firefox/addon/priv8/
> 
> Please don't break it :) We still don't have Containers exposed to
> WebExtensions. See bug 1302697

I believe that issue is fixed now. However, as it seems in bug 1322610 there are still crashes occurring out in the wild because of addons - for someone that has the pref turned on.
After we get rid of XUL addons, I think we can move forward on this.

Comment 12

[Florian Bender]

This can continue now.

Comment 13

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 15

[Andrea Marchesini [:baku]]

Is this bug already fixed?

Comment 16

[Darkspirit]

(In reply to Andrea Marchesini [:baku] from comment 15)

Is this bug already fixed?

No, because network.disable.ipc.security is still there.

(Valentin Gosu [:valentin] from bug 1507157 comment 3)

This pref is left over from B2G days. It is currently disabled for firefox
desktop, but not for Android. This didn't affect us until now because we
always ran Android tests in non-e10s mode.

The pref ought to be removed in bug 1306801.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

No, it's not fixed. I'll have to get to it at some point, unless someone else has the cycles to do it sooner.

Comment 18

[(no longer active)]

Please note that this pref is actually used in the tests for UDPSocket, and replacing it with another pref with a different name for the same thing doesn't make much sense, so fixing this bug probably involves figuring out what to do about those tests first.

Comment 19

[Sunil Mayya]

I am not sure if this is high impactful to move it to the queue or push it to tech debt.
Valentin any thoughts?

Comment 20

[Valentin Gosu [:valentin] (he/him)]

I think this was already removed in bug 1319881
There's still a reference to [UsingNeckoIPCSecurity]https://searchfox.org/mozilla-central/rev/4582d908c17fbf7924f5699609fe4a12c28ddc4a/netwerk/ipc/NeckoParent.cpp#145) which doesn't exist anymore and GetValidatedOriginAttributes always returns a null pointer, so we could clean it up to return void.
Otherwise this is fixed.

Comment 21

[Oskar Mansfeld]

Attachment: Bug 1306801 - Removed UsingNeckoIPCSecurity() mention and changed return type for GetValidatedOriginAttributes to void. r=#necko

Also removed the now obsolete error checks and made CreateChannelLoadContext void as well, because it was now always returning a null pointer, too.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d15a37213282
Removed UsingNeckoIPCSecurity() mention and changed return type for GetValidatedOriginAttributes to void. r=necko-reviewers,kershaw

Comment 23

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d15a37213282