Closed Bug 1607494 Opened 2 years ago Closed 2 years ago

Run PAC Scripts without Javascript Optimizations

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 72+ fixed
firefox72 + fixed
firefox73 + fixed
firefox74 + fixed

People

(Reporter: tjr, Assigned: jandem)

References

Details

(Keywords: csectype-sandbox-escape, sec-want, Whiteboard: [post-critsmash-triage])

Attachments

(2 files)

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.

Group: javascript-core-security

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.

(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.)

Flags: needinfo?(dd.mozilla)

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]

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).

(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.

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.

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.

See Also: → 1608102

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.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

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.
Attachment #9119990 - Flags: sec-approval?

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.

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?

Flags: needinfo?(dd.mozilla) → needinfo?(mozilla)

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.

Attachment #9119990 - Flags: sec-approval? → sec-approval+

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.

Flags: needinfo?(mozilla)

[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.

Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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 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
Attachment #9119990 - Flags: approval-mozilla-release?
Attachment #9119990 - Flags: approval-mozilla-esr68?
Attachment #9119990 - Flags: approval-mozilla-beta?
Attachment #9120158 - Flags: approval-mozilla-release?
Attachment #9120158 - Flags: approval-mozilla-beta?
Attachment #9120158 - Flags: approval-mozilla-release?
Attachment #9120158 - Flags: approval-mozilla-esr68?
Attachment #9120158 - Flags: approval-mozilla-beta?
Attachment #9119990 - Flags: approval-mozilla-esr68?

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.

Attachment #9119990 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9120158 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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

Attachment #9119990 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

No advisory for this?

(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.

(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.

Group: core-security-release

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.

You need to log in before you can comment on or make changes to this bug.