1602485 - (OneByteOverwrites) [meta] Given an arbitrary read/write primitive, it shouldn't be to easy to gain chrome privileges

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P2)

Description

[Frederik Braun [:freddy]]

Might be worth going through this list at
https://searchfox.org/mozilla-central/source/dom/base/nsDeprecatedOperationList.h

But there's likely more.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Presumably all the callsites of xpc::IsInAutomation() should be looked at? I wonder whether we could put that value in a readonly memory mapping or something... In general, if readonly memory mappings are a reasonable mitigation for an arbitrary write primitive, it's worth thinking about what should be thus mapped.

Comment 2

[Masatoshi Kimura [:emk]]

Regarding bug 1602474, the attacker has ability to patch the code, so I'm not sure readonly memory mappings help.

Comment 3

[Boris Zbarsky [:bzbarsky]]

the attacker has ability to patch the code

Does it? I'd think our code is mapped readonly. But I guess the content process can call mprotect or equivalent so the JIT works?

Comment 4

[Tom Ritter [:tjr]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

the attacker has ability to patch the code

Does it? I'd think our code is mapped readonly. But I guess the content process can call mprotect or equivalent so the JIT works?

Correct ACG / !allow-jit would remove the ability for the the process to make the code writable; or make writable memory executable - but it requires moving the JIT out of process so it's a pretty far away goal. We can enable it on other processes though, we have it enabled on RDD. If we had that, and preference values are stored R/W, you could edit the preference to get arbitrary code execution in spite of the memory protections.

However there is a step between 'being able to R/W arbitrary memory' and 'full code execution' even with ACG. And it usually does involve using ROP to call VirtualProtect or the equivalent. Some mitigations (like CFG) are ones we are pursuing to make that step slightly more difficult. Admittedly this is not a strong line of defense at all.

And finally, when these techniques are published they become drop-in-parts to exploit chains. Privately we can expect people to find and maintain these techniques, but when they become public I think we should try to make an effort to kill them both to maintain better security optics and to force attackers to iterate on the techniques.

Comment 5

[Tom Ritter [:tjr]]

:mccr8 suggested that looking for things guarded by CrashIfNotInAutomation could yield additional items.

Comment 6

[Bobby Holley (:bholley)]

So, enablePrivilege and universalXPConnect were just removed in bug 1448967, which is awesome.

The other big one I'm aware of is forcePermissiveCOWs. Post-slaughterhouse, content access to chrome JS objects is extremely limited - which is generally what we want, but entirely breaks the SpecialPowers machinery we use in our tests. So we have this flag, which is only ever set by SpecialPowers, to allow content to freely access any chrome JS object it gets its hands on.

Under normal operation, content should never get a reference to a chrome JS object, so the protections being bypassed here are all defense-in-depth. This means that if we ever discover it being abused in the wild, we should theoretically be able to fix whatever vector allowed the object exposure in the first place. It's possible that could turn into whack-a-mole, but I'd generally expect such vectors to be pretty rare these days.

Another key difference between forcePermissiveCOWs and universalXPConnect is that the former always checks IsInAutomation() before acting on the special bit, whereas the latter had paths (IsCallerChrome) that just trusted the bit (for performance reasons, I think). So they'd need to spoof AreNonLocalConnectionsDisabled() as well.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #5)

:mccr8 suggested that looking for things guarded by CrashIfNotInAutomation could yield additional items.

That's basically just the same thing Boris mentioned in comment 1.

Comment 8

[Frederik Braun [:freddy]]

(In reply to Bobby Holley (:bholley) from comment #6)

Another key difference between forcePermissiveCOWs and universalXPConnect is that the former always checks IsInAutomation() before acting on the special bit, whereas the latter had paths (IsCallerChrome) that just trusted the bit (for performance reasons, I think). So they'd need to spoof AreNonLocalConnectionsDisabled() as well.

BTW, attackers already overwrite IsInAutomation() or AreNonLocalConnectionsDisabled() - See https://github.com/0vercl0k/CVE-2019-11708/blob/0e4e3d437bc7b589b595411a6c79b2e54344da2b/cthulhu.js#L5 for an example.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Bug 1602485. r=aklotz,froydnj

Comment 10

[Masatoshi Kimura [:emk]]

Although attackers can call VirtualProtect, it is only possible after they tamper sAutomationPrefIsSet. Note that the current known attack vector is already closed by removing enablePrivilege. This is just a defense-in-depth.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The exploit is already public.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• 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?: The attack is effective only when attackers can write arbitrary memory using other exploits.
• How likely is this patch to cause regressions; how much testing does it need?: Pretty low. This is a test-only pref.

Comment 12

[Frederik Braun [:freddy]]

That's a neat idea!

We sometimes use xpc::AreNonLocalConnectionsDisabled() instead of IsInAutomation() like in here
https://searchfox.org/mozilla-central/rev/803a42f24c8714631ed81cb824ea1c1a803cb7b8/dom/security/nsContentSecurityManager.cpp#864

I wonder if we want to convert the static bool for IsInAutomation() checks to this new AutomationPrefBool type.
I also wonder how and if we want to implement this type for other platforms.

Comment 13

[Masatoshi Kimura [:emk]]

I update the patch to protect AreNonLocalConnectionsDisabled(). Implementations for other platforms are welcome.

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Clearing sec review flag; I think given the nature of this bug (a hardening measure) and the type of change we expect it to make, we'll land it early in a cycle and watch it closely for stability purposes.

Comment 15

[Masatoshi Kimura [:emk]]

Manually adding to CC to workaround bug 1567526.

Comment 16

[Masatoshi Kimura [:emk]]

Aaron, could you review a change to AutoVirtualProtect? If it doesn't get reviewed until the start of the next cycle, I'll drop the change because it is not mandatory.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Done.

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The exploit is already public.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: The attack is effective only when attackers can write arbitrary memory using other exploits.
• How likely is this patch to cause regressions; how much testing does it need?: This is a test-only pref.

Comment 19

[Masatoshi Kimura [:emk]]

Requesting sec-approval again because a new cycle started.

Comment 20

[Tom Ritter [:tjr]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Approved to land in -central

Comment 21

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

https://hg.mozilla.org/integration/autoland/rev/ef01a12520ae39f25f6cfa3972c85e2b1deb1dcb
https://hg.mozilla.org/mozilla-central/rev/ef01a12520ae

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Looks like this would be trivial to uplift to Beta, which seems like a good idea with pwn2own coming up when 74 is the active release. Please nominate for approval if you agree.

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Beta/Release Uplift Approval Request

• User impact if declined: Easier to exploit
• Is this code covered by automated tests?: No
• 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): This patch involves configurations that are specific to automation. All tricky parts will only be executed in tests.
• String changes made/needed:

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121687 [details]
Bug 1602485. r=aklotz,froydnj

Adds a bit of security hardening. Approved for 74.0b4.

Comment 25

[Natalia Csoregi [:nataliaCs]]

Uplift: https://hg.mozilla.org/releases/mozilla-beta/rev/1ef422a869fabff82e54c5c7f65bd773dff3e942