1325242 - Either remote nsProtocolProxyService for e10s or fix its content-process users

Status: NEW

(Core :: Networking: Proxy, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I've found a few things using nsProtocolProxyService in a content process: WebRTC, which tries to get the “default” proxy by looking up example.com and uses AsyncResolve, and NPAPI, which exposes general lookup using the DeprecatedBlockingResolve method.  SearchFox suggests that those are the only ones that currently exist, but I could be missing something.

This has been causing problems with sandboxing on Linux, especially on systems with with gconf (bug 1322784) but also with dconf (bug 1321134).

More generally, giving direct access to general system configuration information has privacy implications (and possibly worse, if the low-level interface involved would also allow modifications) and exposes additional attack surface.

Ideally we could adjust the callers to not look for this information.  NPAPI is doing this, but bug 778201 comment #4 suggests that Java was the only plugin using it, and we're dropping support for Java “by the end of 2016” according to https://blog.mozilla.org/futurereleases/2015/10/08/npapi-plugins-in-firefox/.  (The crash in bug 1322784 is due to the nsPluginHost constructor pre-instantiating the component so that DeprecatedBlockingResolve can work, so that just indicates that an NPAPI plugin was loaded, not necessarily that it actually used (or would have used, if it hadn't crashed) the proxy lookup feature.)

As for WebRTC, the default proxy would somehow need to make its way to PeerConnectionMedia::Init alongside the other configuration info; I haven't looked at its call chain in depth to suggest anything specific.

(If the NPAPI case can be removed and the WebRTC case can be handled inside the media layer, and there isn't anything else, then it would make sense to move this bug to one of the WebRTC components.)

The alternative is giving the content process a (very limited) implementation of nsIProxyProtocolService that messages the parent over IPDL IPC.  In this case it would still be good if we could remove the NPAPI usage, so we could avoid adding more sync IPC, but not strictly necessary.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The NPAPI case is fixed now (bug 778201), so PeerConnectionMedia is the only content-process proxy service caller left.  Are there plans for doing something about that, or should I just try to add something to PContent/PNecko to work around it?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

forward NI to drno/bwc

Comment 3

[Byron Campen [:bwc]]

I guess we can add this to the pile of reasons to move our ICE logic (ie; everything that runs on content STS right now) to the parent process entirely...

So mjf recently implemented an IPC dip for obtaining local network interfaces, that lives in PeerConnectionMedia alongside the proxy lookup stuff. It might be possible to expand that a little bit to include the proxy lookup too.

Comment 4

[Michael Froman [:mjf]]

I'll have to take a look at how easy that would be.  The good news is that PeerConnectionMedia is already setup to deal with the async nature of this for the proxy query.  I have not looked at the data dependencies yet.

Comment 5

[Nils Ohlmeier [:drno]]

Just to make sure I understand the asking here: basically nsProtocolProxyService should not be used by any content process, because we want to tighten the sandbox once more, correct?

I see three options on the table:
#1 as Byron indicated: move our ICE stack (nICEr) from content into a new process (bug 1322426)
#2 there is another bug (I would have to look it up) where the idea was/is to move the whole proxy logic from nICEr to Necko via an IPC interface, but that got mostly neglected in favor of #1
#3 just make the nsProtocolProxyService part accessible over IPC, which Michael offered to look at in comment #4 in terms of effort

Comment 6

[Nils Ohlmeier [:drno]]

For option #2 its bug 1287225.
Recent discussion resulted in that we need to fix bug 1287225 even if we eventually will implement option #1.

Originally I was thinking for option #2 to still do the proxy query first and then include the proxy address. But it might be even better to design the interface for #2 to avoid querying for the proxy address in the first place, and just do a "get me proxy connection" kind of call without even worrying if there is a proxy around.

If we are going to do option #2 any way, option #3 would be just a waste of time and effort. So it depends on how quickly we want/need to fix this here.
Jed any input on time constraints?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The time constraints are a lot looser than I thought when I started trying to answer the needinfo.  Breaking this down by config framework:

GConf: Calls utime() but doesn't care if it fails; thus, working fine on not-Nightly, and crashing on Nightly but it's a two-line workaround that won't affect security.  (Also, gconf is uncommon and it's not clear how much we really support it.)

DConf: Slow but functionally correct (see bug 1321134 comment #8); not really an issue for the WebRTC proxy use case, and fixable with a small change to the broker policy in any case.  (Important subtlety: DConf uses DBus only for writing prefs, not reading, so this is not in fact a DBus-removal blocker.)

Also, there's at least one other content-process user of DConf (see bug 1321134 comment #3 and following), so dealing with this won't (by itself) prevent having to do something about DConf.


In conclusion, there's no real time pressure on this.

Comment 8

[Firefox Bug Husbandry Bot]

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

Comment 9

[Firefox Bug Husbandry Bot]

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

Comment 10

[BMO Automation]

Moving bug to Core/Networking: Proxy.