1607494 - Run PAC Scripts without Javascript Optimizations

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Tom Ritter [:tjr]]

Bug 1607443 was a report about a JS bug exploited in the parent via a PAC script.

If we disabled JS Engine optimizations for PAC scripts, we would eliminate a lot of local-network-accessible attack surface available in the parent process.

Comment 1

[Daniel Veditz [:dveditz]]

Or don't run it in a privileged process at all? Is that a separate bug or an alternative to disabling the JIT? The JIT isn't the only source of exploitable JS bugs we've had.

Comment 2

[Tom Ritter [:tjr]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Or don't run it in a privileged process at all? Is that a separate bug or an alternative to disabling the JIT? The JIT isn't the only source of exploitable JS bugs we've had.

We could run it in the socket process. That would be nice. But I think deploying the Socket process is far enough away to consider disabling mitigations in the meantime... (Adding Dragana to see what she thinks.)

Comment 3

[Daniel Veditz [:dveditz]]

I assumed it did run in the socket process. Have we not shipped that? I have one in Nightly on mac but it appears not to be sandboxed so I'm not sure that helps much. (Maybe it's partially sandboxed and just doesn't trip the Activity Monitor "sandbox" definition?)

[Confirmed that Firefox 72 on Mac does NOT have a socket process]

Comment 4

[Jan de Mooij [:jandem]]

We should consider disabling WebAssembly in PAC scripts too. Maybe a per-runtime override to turn off all native codegen (JS JITs, Wasm, regexp JIT).

Comment 5

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #4)

We should consider disabling WebAssembly in PAC scripts too. Maybe a per-runtime override to turn off all native codegen (JS JITs, Wasm, regexp JIT).

For JS and regex there's a fallback to the interpreter, though; for wasm there's not (and we don't want to add an interpreter i think). That said, only very large PAC scripts would likely benefit from wasm, so this seems sensible for now.

Comment 6

[Ted Campbell [:tcampbell]]

Running elsewhere should probably be a different bug. Disabling JITs is a reasonable sized problem that we can tackle in these next few weeks and can be discussed here.

Comment 7

[Dragana Damjanovic [:dragana]]

Socket process is far in the future. There is one but it is use only for WebRTC. We plan to try to turn the socket process on Nightly in 76 (the project was de-prioritized). The socket process is not sandboxed yet. I agree with statement above that we should start by disabling JITs

I can have look if we can run it elsewhere. I will open another bug for that.

Comment 8

[Jan de Mooij [:jandem]]

We investigated disabling Ion in the parent process (bug 1608102), but the main blocker there is devtools performance. I'm going to add a per-context override to disable Ion just for the PAC runtime.

We should continue to look into other PAC mitigations too; we've had non-JIT security bugs as well.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

Comment 10

[Ted Campbell [:tcampbell]]

Comment on attachment 9119990 [details]
Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This mitigates part of the chain we believe was used in CVE-2019-17026. There seems to be twitter discussion suggesting this is externally known, so the cat is probably out of the bag.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: ESR68 will need a different patch, but that patch is a single line since enableIon was a per-context flag at the time.
• How likely is this patch to cause regressions; how much testing does it need?: This prevents compiling into the highest tier JIT. There is already a pref to disable Ion globally, this just forces the PAC context to always disable. Stability risk is low. If there are extreme uses of PAC, they may see a perf impact without Ion.

Comment 11

[Ted Campbell [:tcampbell]]

Requesting sec-approval just to make sure. We should get this into nightly and beta. If there are further point releases in 72, it might be beneficial to ship too.

Comment 12

[Tom Ritter [:tjr]]

Hey Mike - we would really like to turn off Javascript optimizations for PAC scripts. I expect that enterprises are the only people who are running huge PAC scripts... do you have any anecdotal data about that? If it's ever come up before? Should we consider a pref for them? Do you think we could land it on ESR as 68.4.2 or 68.5 or would you want it to wait until the next big ESR release?

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9119990 [details]
Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

Approved to land and request uplift. The 'PAC scripts run in the parent process' exploit technique is publicly known, I don't see any reason to try to obfuscate what we're doing here.

Comment 14

[Mike Kaply [:mkaply]]

I know that some folks have some big PAC scripts, but I'm not sure the attack surface is worth it.

I'm fine with turning it off in the ESR dot release and if we get pushback, we can address it in another rlease.

Comment 15

[Ted Campbell [:tcampbell]]

Attachment: Bug 1607494 - [ESR] Disable Ion for the PAC script thread.

Comment 16

[Ted Campbell [:tcampbell]]

https://hg.mozilla.org/integration/autoland/rev/b95097b198726095d39f3074ab35a67ccac3ab8d

Comment 17

[Ted Campbell [:tcampbell]]

[Tracking Requested - why for this release]:
This is a hardening patch as follow up to a security incident. If a point-release happens for 72, this is a lower-risk change to consider including.

Patch is currently on autoland and will be in next NIghtly. I will request better uplift by or before Monday.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Seems like we want this in Fx73/68.5esr at a minimum. I'm less certain about 72.0.2 (and presumably 68.4.2esr if we take it for 72).

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 9119990 [details]
Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: Security - potential JIT bugs could be triggered via PAC scripts and execute directly in the parent process.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Main risk is perf impact but in practice most PAC scripts are probably small and not too perf-sensive, and most users don't use PAC scripts.
• String changes made/needed: N/A

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119990 [details]
Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

Approved for 73.0b5 and 68.5esr. Leaving the release request for Julien to evaluate.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b72cf65f7d74

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/eb7f3c7a1477

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9119990 [details]
Bug 1607494 - Disable Ion for the PAC script thread. r?tcampbell!

mitigation for sec issues, approved for 72.0.2

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Landed on FIREFOX_ESR_68_4_X_RELBRANCH for 68.4.2.
https://hg.mozilla.org/releases/mozilla-esr68/rev/ec4175ed144b80188abcba4486ee7f03dbc2cc06

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/82c6ae32ec01

Comment 27

[Mike Hommey [:glandium]]

No advisory for this?

Comment 28

[Julien Cristau [:jcristau]]

(In reply to Mike Hommey [:glandium] from comment #27)

No advisory for this?

AIUI that's correct as this is hardening not fixing a specific flaw.

Comment 29

[Tom Ritter [:tjr]]

(In reply to Julien Cristau [:jcristau] from comment #28)

(In reply to Mike Hommey [:glandium] from comment #27)

No advisory for this?

AIUI that's correct as this is hardening not fixing a specific flaw.

Correct.

Comment 30

[Tom Ritter [:tjr]]

I'm going to give this the csectype-sandbox-escape, keyword to capture the PAC attack vector, even if this exact bug does not explicitly cover it.