1449736 - IPC: use-after-free crash with Msg_PPluginScriptableObjectConstructor [@~PluginScriptableObjectParent]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Christoph Diehl [:posidron]]

Attachment: faulty.txt

The following message was identified to be responsible for this crash and got blacklisted from fuzzing until fixed.

Message: PPluginInstance::Msg_PPluginScriptableObjectConstructor

== ORIGINAL

$ hexdump -C /tmp/faulty/message.7675.51827.o
00000000  04 00 00 00 05 00 00 00  68 00 5b 00 01 04 00 00  |........h.[.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff 00 00 00 00  |................|
00000020  06 00 00 00                                       |....|
00000024

== MUTATED

$ hexdump -C /tmp/faulty/message.7675.51827.m
00000000  04 00 00 00 05 00 00 00  68 00 5b 00 01 04 00 00  |........h.[.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff 00 00 00 00  |................|
00000020  03 70 8a 00                                       |.p..|
00000024


See the attached file for further details.

Comment 1

[Christoph Diehl [:posidron]]

FYI, Crashes with the same stack but with the signature of a UAF Write are triggered too.

Comment 2

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

PPluginScriptableObjectConstructor doesn't have any arguments, just the routing ID of the new actor, so the mutation is probably that field, and then the PPluginScriptableObject sent in the NPN_GetValue_NPNVPluginElementNPObject reply won't match an actor.

Which brings us to https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/plugins/ipc/PluginInstanceChild.cpp#294

That's not checking the return value and it looks like it'll use an uninitialized pointer if that fails.

But this is a child-side actor, and I thought those would always crash on error, and that might be why there's no error checking?  But it's not crashing there, and using a bad pointer.

And PPluginInstance is a little unusual: the “parent” end is in a content process and the “child” end is in the NPAPI plugin process, so the child is the less-sandboxed one.  Probably neither end should trust the other, but definitely the child shouldn't trust the parent, which is the opposite of how it normally works.

Comment 3

[Christoph Diehl [:posidron]]

Attachment: uaf-write.txt

Attaching the stack for the UAF write 8

Comment 4

[Christoph Diehl [:posidron]]

The UAF write was probably caused by "PPluginScriptableObject::Reply_SetProperty" it was the only message which got fuzzed but two times during the instantiation of the plugin process. 

1.

$ hexdump -C /tmp/faulty/message.6148.52505.o
00000000  04 00 00 00 17 00 00 00  10 00 5e 00 a1 00 00 00  |..........^.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff db ff ff ff  |................|
00000020  01 00 00 00                                       |....|

$ hexdump -C /tmp/faulty/message.6148.52505.m
00000000  04 00 00 00 17 00 00 00  10 00 5e 00 a1 00 00 00  |..........^.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff db ff ff ff  |................|
00000020  ff 02 00 00                                       |....|

2.

$ hexdump -C /tmp/faulty/message.6148.52518.o
00000000  04 00 00 00 1a 00 00 00  10 00 5e 00 a1 00 00 00  |..........^.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff d6 ff ff ff  |................|
00000020  01 00 00 00                                       |....|

$ hexdump -C /tmp/faulty/message.6148.52518.m
00000000  04 00 00 00 1a 00 00 00  10 00 5e 00 a1 00 00 00  |..........^.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff d6 ff ff ff  |................|
00000020  01 ff 00 00                                       |....|

Comment 5

[Frederik Braun [:freddy]]

Jim, can you help us get an assignee on this bug.
It's sec-high and it has a test case.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Emailing to follow up.

Comment 7

[Jim Mathies [:jimm]]

For triage / investigation. David, can you take a look at this? It's been around for a while. We don't get a lot of sec bugs in plugin code, would be great if we could find an easy fix.

Comment 8

[David Parks [:handyman]]

I agree with Jed's findings in comment 2.  Its very likely to be that the PluginScriptableObjectChild returned by CallNPN_GetValue_NPNVPluginElementNPObject in PluginInstanceChild::InternalGetNPObjectForValue is not set because of the fuzzed message [1].  That method has bad error handling -- result should not be initialized and set for each switch case (since the compiler would catch this failure in that case).  Also, the `Call*`s should check for IPC errors as Jed pointed out.  But in this case the result value is whats totally broken.  Its ok that actorProtocol is unset by the Call as long as result isn't incidentally NPERR_NO_ERROR.  Since it is NPERR_NO_ERROR, it runs the conditional block after the Call and fails a few lines later.

I can put together a patch for this quickly but I haven't dealt with this fuzzer and I'm not sure how to run it.  Posidron pointed me to the meta-bug for instructions, bug 777067, but it might take me a bit to get up to speed on it.

--

[1] https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/plugins/ipc/PluginInstanceChild.cpp#294

Comment 9

[David Parks [:handyman]]

That should say: "result should not be initialized _but instead_ set for each switch case"

Comment 10

[David Parks [:handyman]]

I haven't been able to reproduce the issue with the fuzzer but I have a patch that implements the plan in comment 8.  I'll post it today -- we may as well try it.

---

The fuzzer may be easier to run on Linux.  A task for another day.  I'm running Windows 10 and it doesn't seem to be fully supported.  I'm not even sure that Fuzzy did anything in my attempts -- most of my issues were with running ASAN.  I never saw any output (log or files) from Fuzzy but I don't know why.  Once I got an ASAN build that didn't immediately fail (I had to remove some RDD stuff from ContentParent, block a part of the DLL interceptor, and run in WinDbg) I whitelisted the message in comment 0 and ran the tests in dom/plugins/test/mochitest.  I set FAULTY_PROBABIITY to 1 since the code suggests that _lower_ probability perturbs more frequently.  And I tried setting FAULTY_MESSAGE_PATH to a folder and to a txt file, Windows format (ie C:\foo\bar) and unix (/c/foo/bar), but I never saw any output.  I do believe I wasn't actually fuzzing.

Comment 11

[David Parks [:handyman]]

Attachment: Bug 1449736: Properly detect failure in receiving plugin NPObjects (r?jmathies)

We need to recognize IPDL errors in order to return the proper error status when the right PluginScriptableObject could not be found.

Comment 12

[Christoph Diehl [:posidron]]

@David Parks
Faulty is not supported on Windows (yet) may be in another Q. The way how it intercepts the messages relies on the POSIX implementation. The IPC implementation on Windows differs in that regard.

Comment 13

[David Parks [:handyman]]

Thanks posidron.  That confirms what I thought I was seeing.  Any chance it would be easy for you to try a run with this patch to see if you still get the failure?  (If no, that's cool -- we'll likely use the patch anyway).  I probably won't make the effort to set this up on Linux any time soon.

Comment 14

[Christoph Diehl [:posidron]]

This was during the time before we tried to use `rr` for Faulty. I do not have explicit reproduction steps and hence it would be a hit and miss for this bug. 

If you want you can use: https://github.com/MozillaSecurity/orion/tree/master/services/grizzly#example-faulty this is an example for running Faulty within a Docker container. The setup should still work but if there is an issue I can help out after the holidays. I believe though we would need to make some modifications to add a custom build via -v to that container.

Comment 15

[David Parks [:handyman]]

Thanks -- that'll be my plan B.  I think we can just land this and look at crashes.  I thought I saw more before but I believe I see 8 of the crashes from the UAF case in the last week [1].  (All were ESR strangely but, if you widen the date range, all builds are included).  If I'm right, that should stop.

---

[1] https://crash-stats.mozilla.com/search/?signature=~~PluginInstanceParent&date=%3E%3D2018-12-21T09%3A11%3A59.000Z&date=%3C2018-12-28T09%3A11%3A59.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 16

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

Backed out for mochitest failure at layout/generic/test/test_plugin_focus.html:

https://hg.mozilla.org/integration/autoland/rev/75be9882c2aea62baa2c00dd0d2ba15532300fc6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=usercancel%2Cretry%2Ctestfailed%2Cbusted%2Cexception&group_state=expanded&revision=49be7aaa199c0be85ec2667c650d4627cfec84c1
Please check that link, there are e.g. clipboard failures and also this: https://treeherder.mozilla.org/logviewer.html#?job_id=221160290&repo=autoland

[task 2019-01-10T20:49:11.460Z] 20:49:11 INFO - TEST-START | layout/generic/test/test_plugin_focus.html
[task 2019-01-10T20:54:12.815Z] 20:54:12 INFO - TEST-INFO | started process screentopng
[task 2019-01-10T20:54:13.102Z] 20:54:13 INFO - TEST-INFO | screentopng: exit 0
[task 2019-01-10T20:54:13.103Z] 20:54:13 INFO - TEST-UNEXPECTED-FAIL | layout/generic/test/test_plugin_focus.html | Test timed out.
[task 2019-01-10T20:54:13.104Z] 20:54:13 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:7
[task 2019-01-10T20:54:13.105Z] 20:54:13 INFO - reportError@SimpleTest/TestRunner.js:121:7
[task 2019-01-10T20:54:13.105Z] 20:54:13 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2019-01-10T20:54:13.106Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.107Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.107Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.107Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.108Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.109Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.110Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.110Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.111Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.112Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.112Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.113Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.113Z] 20:54:13 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2019-01-10T20:54:13.114Z] 20:54:13 INFO - TestRunner.runTests/<@SimpleTest/TestRunner.js:381:9
[task 2019-01-10T20:54:13.114Z] 20:54:13 INFO - promise callbackTestRunner.runTests@SimpleTest/TestRunner.js:368:50
[task 2019-01-10T20:54:13.115Z] 20:54:13 INFO - RunSet.runtests@SimpleTest/setup.js:201:3
[task 2019-01-10T20:54:13.116Z] 20:54:13 INFO - RunSet.runall@SimpleTest/setup.js:180:5
[task 2019-01-10T20:54:13.116Z] 20:54:13 INFO - hookupTests@SimpleTest/setup.js:273:5
[task 2019-01-10T20:54:13.117Z] 20:54:13 INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
[task 2019-01-10T20:54:13.117Z] 20:54:13 INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11

Comment 17

[David Parks [:handyman]]

Looking into it.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

David, is this still something you might work on?
I'm going through some untouched-for-a-while sec-high issues to make sure we've investigated all we can before marking them stalled.

Comment 19

[David Parks [:handyman]]

Hmm.. This had fallen off my radar while I was on WebGL but I'm turning back to plugin sandboxing issues now so I'll add it to that list.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix at this point for 70. We could still take a patch in 71/72.

Comment 21

[David Parks [:handyman]]

Attachment: Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

Properly handles NPError reporting and makes sure that, in the case of failure, it does not return junk for the NPObject.

Comment 22

[David Parks [:handyman]]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. It at least requires plugin use and some special IPC control at plugin start.
• 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 patch is small and simple and this area of code is largely inactive. Effort is somewhere between trivial and mild.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It adds rare error handling that would otherwise always be a UAF.

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

sec-approved to land now and request uplifts

Comment 24

[David Parks [:handyman]]

Not sure what the smart move is on uplifting this so I'm going to land it and give it a week or so on nightly. We have time before the next release. Its not really high volume enough (on nightly or anywhere) for that to be a great test but it will give some confidence that we don't break anything serious before uplifting.

Comment 25

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

https://hg.mozilla.org/integration/autoland/rev/e017a75fabe40fad39edd3dbfc3a111636155684
https://hg.mozilla.org/mozilla-central/rev/e017a75fabe4

Comment 26

[Brindusa Tot, Desktop QA]

Can this be verified manually? If yes, could you please help us with STR or testcase?

Comment 27

[David Parks [:handyman]]

Unfortunately, we never figured out how to force this -- it was found with fuzzing and appears in crash-stats so the idea is to land this to see if it fixes the issue. But I'm not even 100% sure this will stop the crashes. That's much of why I want to wait a bit before uplift.

Comment 28

[David Parks [:handyman]]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, about 200 a month related to signatures that I believe this covers.
• 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: Medium
• Why is the change risky/not risky? (and alternatives if risky): All it does is return a valid error instead of undefined data. This should always be at least as good as what we have and I believe the error handling is sound so it should only fix things. Having said that, its only had about a week of exposure on nightly and that's really not enough to say that its been fixed, given the infrequency.
• String changes made/needed:

Comment 29

[David Parks [:handyman]]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: critical
• User impact if declined: Crashes -- see beta uplift answer for details.
• Fix Landed on Version: 72
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The code is probably safe but it is infrequent and can't be said to have been tested in Nightly. See beta-uplift answer for details.
• String or UUID changes made by this patch:

Comment 30

[Pascal Chevrel:pascalc]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

Was on 2 weeks in Nightly and we are in our last week of betas, uplift approved for 71 beta 11, thanks.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5c921325dd03

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9099447 [details]
Bug 1449736: Properly detect failure in receiving plugin NPObjects r=jmathies!

Approved for 68.3esr also.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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