dsinclair: in order to do this we first need to address the TODO here:
https://cs.chromium.org/chromium/src/pdf/out_of_process_instance.cc?type=cs&q=Remove+this+dependency+on+VarPrivate/InstancePrivate.&sq=package:chromium&l=326

Can the PDF team look into that? Once that's done I'm happy to remove the PDF plugins dependency on those APIs.

So, if I understand correctly, we want to remove the code in the ModalDialog function which uses a PPB_Private code and create a method in the PPB_PDF which does the equivalent of showing a dialog?

Would it make sense the send this all the way up to the JS code and just create an HTML element instead of using an actual modal dialog?

I'll see what I can do.

Agree with the need for PERMISSION_PDF.  That's what I had in mind.

An underlying issue is that PERMISSION_PRIVATE isn't granular enough. It might also make sense to split off the https://cs.chromium.org/chromium/src/ppapi/c/private/ppb_instance_private.h methods into a separate permission and see where it can be revoked?

Ok, I'm lost in a twisty world of PPAPI. Does anyone have a pointer on documentation on how to plumb things through here?

Looking at the GetWindowObject, it's a PPP object which uses the dispatcher to send a message ... somewhere?

For the PDF stuff we have a PPB_PDF object, which doesn't seem to have the dispatcher, there is no PPP_PDF interface?

We need to be able to send a synchronous message from the plugin and get the result back from the browser before the OutOfProcess method returns. I can see there are ROUTED messages that can be sent and a SyncCall method, is that the right road to wander down?

https://chromium-review.googlesource.com/c/chromium/src/+/961270 is where I've gotten to, but I'm not sure how to move on from there at the moment?

@raymes - I wonder if there are other extensions abusing the PPB_PDF API under the auspices of the PERMISSION_PRIVATE permission. Even without dsinclair's changes, I'm wondering if we can add PERMISSION_PDF, grant it along with PERMISSION_PRIVATE to the PDF extension, and make the PPB_API check for the new permission.

At which point, we'll see what breaks?

Re: #8 we probably can but I think we might as well wait and do it altogether? Flash would be the only other plugin to have access to this API.

Friendly ping on this.  Given that this has been used in an attack, we should try to remove the script permission as soon as we can.  I'm tentatively marking this M65, but we should determine if that's possible and/or necessary.

Can someone update the bug with what the current plan is (e.g., is it dsinclair's CL from comment 6?), and whether we're blocked on anything?  Thanks!

We're currently waiting on https://chromium-review.googlesource.com/c/chromium/src/+/961270 which Dan is working on. However, there are other private APIs that the PDF plugin also requires, as shown by https://chromium-review.googlesource.com/c/chromium/src/+/963508 , so we still need a better design for the granularity of permissions.

So I think we need two new permissions (PERMISSION_SCRIPT, PERMISSION_PDF) as in
https://chromium-review.googlesource.com/c/chromium/src/+/963508

For the ModalDialog change, I'm still trying to work out how to get the right data passed back and forth. My understanding is that the attack has been mitigated through other changes at this point?

I'm OOO tomorrow, but will continue working on the PPAPI change Monday morning.

Thanks.  Please try to prioritize this if possible.  The fix for  issue 821613  might have to be reverted; we're not sure yet.  If so, that would leave us without a fix for the privilege escalation in the second variant of the attack.  (We do have a second fix for the first variant of the attack in  issue 821596 .)

creis: the fix here is going to be multiple CLs in size and probably not great to merge. I think we should probaly merge the fix in  issue 821613  instead. Obviously we should still get this fixed ASAP though. 

I've taken over tsepez's CL in https://chromium-review.googlesource.com/c/chromium/src/+/967907

Also see issue 823171 for locking down plugin capabilities even further.

Comment 16: Thanks for the context.  I agree with trying to merge the fix for  issue 821613  to M66 if that's safe (see my question on that bug about proceeding).  Do you think it's still possible to get this one fixed in M67?

I feel fairly certain we can get it in for M67.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b86026c962dd8d3cad5e826c4ce18432867cc44c

commit b86026c962dd8d3cad5e826c4ce18432867cc44c
Author: Jochen Eisinger <jochen@chromium.org>
Date: Fri Mar 23 11:30:19 2018

Expose modal dialogs to public API

We want to avoid having to go through JavaScript to invoke them for
PPAPI.

BUG= 821266 
R=dsinclair@chromium.org,haraken@chromium.org

Change-Id: I763a9a3eaa8036372cc559c8b9ec8a312873e1b3
Reviewed-on: https://chromium-review.googlesource.com/975862
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545404}
[modify] https://crrev.com/b86026c962dd8d3cad5e826c4ce18432867cc44c/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/b86026c962dd8d3cad5e826c4ce18432867cc44c/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/b86026c962dd8d3cad5e826c4ce18432867cc44c/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.h
[modify] https://crrev.com/b86026c962dd8d3cad5e826c4ce18432867cc44c/third_party/WebKit/public/web/WebLocalFrame.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e884f721cbbbe30152a346d75a50480a81503ed

commit 2e884f721cbbbe30152a346d75a50480a81503ed
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon Mar 26 16:43:05 2018

Add a ShowDialog callback for PDF plugin.

This CL adds a custom callback for the PDF plugin to use to display
dialogs.

Bug:  821266 
Change-Id: I6bf97678e773a086b5198a9a961805dcde6acc39
Reviewed-on: https://chromium-review.googlesource.com/961270
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545814}
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/components/pdf/renderer/BUILD.gn
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/components/pdf/renderer/pepper_pdf_host.cc
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/components/pdf/renderer/pepper_pdf_host.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/pdf/out_of_process_instance.cc
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/c/private/ppb_pdf.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/cpp/private/pdf.cc
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/cpp/private/pdf.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/proxy/pdf_resource.cc
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/proxy/pdf_resource.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/proxy/ppapi_messages.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/thunk/ppb_pdf_api.h
[modify] https://crrev.com/2e884f721cbbbe30152a346d75a50480a81503ed/ppapi/thunk/ppb_pdf_thunk.cc

The PDF change for Modal Dialog is committed. Passing this over to raymes@ for the remaining permissions CL.

Thanks Dan!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85d754a8d45b84d83e3801a3fdeadbad5fe412ea

commit 85d754a8d45b84d83e3801a3fdeadbad5fe412ea
Author: Raymes Khoury <raymes@chromium.org>
Date: Wed Mar 28 00:57:24 2018

Add ppapi::PERMISSION_PDF and ppapi::PERMISSION_CROSS_ORIGIN_URL_LOADS

Create a PDF permission for APIs specific to the PDF plugin. Also remove
PERMISSION_PRIVATE from PDF as it does not need most of those APIs. One
exception is the PPB_URLLoader_Trusted API for which we move permission
checking to happen inside ppb_url_loader.cc.

Bug:  821266 
Change-Id: I5f174d1fd2bff6fb7475956d9b9c772474648515
Reviewed-on: https://chromium-review.googlesource.com/967907
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546345}
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/chrome/common/chrome_content_client.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/chrome/common/pepper_flash.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/chrome/common/ppapi_utils.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/content/renderer/pepper/pepper_plugin_instance_impl.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/content/renderer/pepper/pepper_url_loader_host.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/content/renderer/pepper/plugin_module.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/api/trusted/ppb_url_loader_trusted.idl
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/c/trusted/ppb_url_loader_trusted.h
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/proxy/interface_list.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/proxy/ppb_instance_proxy.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/proxy/ppb_var_deprecated_proxy.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/proxy/ppp_class_proxy.cc
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/shared_impl/ppapi_permissions.h
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/thunk/interfaces_ppb_private.h
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/thunk/interfaces_ppb_private_no_permissions.h
[add] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/thunk/interfaces_ppb_private_pdf.h
[modify] https://crrev.com/85d754a8d45b84d83e3801a3fdeadbad5fe412ea/ppapi/thunk/thunk.h

This should be fixed. Still tracking some additional lock down in issue 823171

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot