New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug

Blocking:
issue 817858



Sign in to add a comment

Site Isolate PDFium

Project Member Reported by palmer@chromium.org, Feb 6

Issue description

Tom says: “I’m still seeing an external PPAPI process when I open a PDF file, and only one process even when I have a mix of pdf from secure and insecure origins.” So we have some risk of cross-origin PDF content. Consider https://internal.corp.example.com/q1-earnings.pdf and https://evil.com/evil.pdf. It would be better to do in-process PDFium in a site-appropriate renderer, or use different PPAPI processes per SiteInstance.

This is currently a gap in our Spectre mitigation plan, so it's probably Pri-1.
 
Labels: ReleaseBlock-Stable Security_Severity-High M-66 Security_Impact-Stable
We may want to consider doing servicification (bug 702993) first and get off of PPAPI. Unless it's easy to tweak PPAPI to allow multiple PDF plugins.
Cc: jam@chromium.org
Do we know if servicification is at a point to support the PDF requirements yet? Or is that just a no-go at this point.
Labels: -Type-Bug-Security -Security_Impact-Stable -Security_Severity-High -ReleaseBlock-Stable Type-Bug
Changing flags, since this is a security improvement rather than fixing a vulnerability. Not sure the view restriction is needed, but I'll leave it in place.

Re #1: Flash and NaCl can both launch more than one process, so we should be able to tweak this. Also, this is intended as a quick fix. so, we don't want to gate it on a long pole like converting PPAPI to servicification.
Blocking: 817858
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 1

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

commit 8db30ad0468bc729faff01d57cf4b1bec0081bb3
Author: Tom Sepez <tsepez@chromium.org>
Date: Thu Mar 01 21:38:54 2018

Manage ppapi plugins on a per-origin basis.

Expands the management of plugins to consider origin when assigning
a process. Only passes empty origins at present, so no observable
change expected.

Bug:  809614 
Change-Id: I129495ddc94c86dc1c4aa1b3a7942354eabe59d4
Reviewed-on: https://chromium-review.googlesource.com/915182
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540294}
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/frame_host/render_frame_message_filter.h
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/plugin_service_impl.h
[add] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/plugin_service_impl_browsertest.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/ppapi_plugin_process_host.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/browser/ppapi_plugin_process_host.h
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/common/frame_messages.h
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/renderer/pepper/pepper_plugin_registry.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/renderer/pepper/pepper_plugin_registry.h
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/renderer/pepper/plugin_module.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/renderer/pepper/plugin_module.h
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/8db30ad0468bc729faff01d57cf4b1bec0081bb3/content/test/BUILD.gn

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 2

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

commit 19fecb3def631c62bb3536117e6760ae61e4bbc9
Author: Tom Sepez <tsepez@chromium.org>
Date: Fri Mar 02 18:40:21 2018

Bound ppapi process totals when isolating by origin.

Otherwise, we risk having an unbounded number of processes.
Results in a "failed to load plugin" page once N == 15 created.
Add test to fork off a handful.

No plugins are yet isolated by origin, so no observable change
to chrome. That will happen in a subsequent CL.

Follow-up to:
  https://chromium-review.googlesource.com/c/chromium/src/+/915182

Bug:  809614 
Change-Id: Ie71d41d03d380e31f006173b659023e3de0e3ac8
Reviewed-on: https://chromium-review.googlesource.com/942235
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540565}
[modify] https://crrev.com/19fecb3def631c62bb3536117e6760ae61e4bbc9/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/19fecb3def631c62bb3536117e6760ae61e4bbc9/content/browser/plugin_service_impl.h
[modify] https://crrev.com/19fecb3def631c62bb3536117e6760ae61e4bbc9/content/browser/plugin_service_impl_browsertest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 8

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

commit f84d72289024c3795f4fbb24012eb88d60bb67cb
Author: Tom Sepez <tsepez@chromium.org>
Date: Thu Mar 08 19:34:50 2018

Add mechanism to count PDF PPAPI processes in browser tests

Call it as appropriate in pdf_extension_test.cc

Split off from https://chromium-review.googlesource.com/c/chromium/src/+/922727
so as to get a baseline before making further changes.

Bug:  809614 
Change-Id: I85e947230fd4f87547dac169e2271c2510c41b29
Reviewed-on: https://chromium-review.googlesource.com/953712
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541866}
[modify] https://crrev.com/f84d72289024c3795f4fbb24012eb88d60bb67cb/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/f84d72289024c3795f4fbb24012eb88d60bb67cb/content/browser/plugin_service_impl.h
[modify] https://crrev.com/f84d72289024c3795f4fbb24012eb88d60bb67cb/content/public/browser/plugin_service.h
[modify] https://crrev.com/f84d72289024c3795f4fbb24012eb88d60bb67cb/content/test/fake_plugin_service.cc
[modify] https://crrev.com/f84d72289024c3795f4fbb24012eb88d60bb67cb/content/test/fake_plugin_service.h

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 9

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

commit ceae97b000fd209df034b1b3c5b6d12ef5d561e5
Author: Tom Sepez <tsepez@chromium.org>
Date: Fri Mar 09 18:25:20 2018

Don't share PDF plugins across origin boundaries.

Controlled by --enable-features=PdfIsolation, default is off.
Stricter than required (full origin, not eTLD+1).
Add pdf_extension_test for multiple domains.
A subsequent patch may attempt to apply this to secure origins only.

Bug:  809614 

Change-Id: Ic38c2c5074c7723210e38a754b930fb152c031b7
Reviewed-on: https://chromium-review.googlesource.com/953979
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542162}
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/content/public/common/content_features.cc
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/content/public/common/content_features.h
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/ceae97b000fd209df034b1b3c5b6d12ef5d561e5/content/renderer/render_frame_impl.cc

Status: Fixed (was: Assigned)
Code landed, will use 817858 to track launch and finch flag.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 9

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

commit 5ff974fc729167fd8b86376c1a1b25774634906f
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Fri Mar 09 23:49:38 2018

Revert "Don't share PDF plugins across origin boundaries."

This reverts commit ceae97b000fd209df034b1b3c5b6d12ef5d561e5.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 542162 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2NlYWU5N2IwMDBmZDIwOWRmMDM0YjFiM2M1YjZkMTJlZjVkNTYxZTUM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20MSan%20Tests/8413

Sample Failed Step: browser_tests

Original change's description:
> Don't share PDF plugins across origin boundaries.
> 
> Controlled by --enable-features=PdfIsolation, default is off.
> Stricter than required (full origin, not eTLD+1).
> Add pdf_extension_test for multiple domains.
> A subsequent patch may attempt to apply this to secure origins only.
> 
> Bug:  809614 
> 
> Change-Id: Ic38c2c5074c7723210e38a754b930fb152c031b7
> Reviewed-on: https://chromium-review.googlesource.com/953979
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Tom Sepez <tsepez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542162}

Change-Id: Idc8255cdc2ad8bfada325fa9e49069e5d32c0246
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  809614 
Reviewed-on: https://chromium-review.googlesource.com/957783
Cr-Commit-Position: refs/heads/master@{#542284}
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/content/public/common/content_features.cc
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/content/public/common/content_features.h
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/5ff974fc729167fd8b86376c1a1b25774634906f/content/renderer/render_frame_impl.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Mar 10

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

commit abc55490658e0ec0bf2be019449b7248b923ce90
Author: Tom Sepez <tsepez@chromium.org>
Date: Sat Mar 10 00:20:16 2018

Revert "Add about:flag for feature PdfIsolation"

This reverts commit 3f7270c272c98bafff4c7943e56d9884ae41ff86.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Add about:flag for feature PdfIsolation
> 
> Bug:  809614 
> Change-Id: I63fb89898c9ed83e532fd148a0a4dd318c194693
> Reviewed-on: https://chromium-review.googlesource.com/956488
> Commit-Queue: Tom Sepez <tsepez@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542215}

TBR=isherman@chromium.org,jam@chromium.org,tsepez@chromium.org

Change-Id: Ifec7ccfc80d125bdef35f41f997aa412db7a8823
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  809614 
Reviewed-on: https://chromium-review.googlesource.com/957784
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542287}
[modify] https://crrev.com/abc55490658e0ec0bf2be019449b7248b923ce90/chrome/browser/about_flags.cc
[modify] https://crrev.com/abc55490658e0ec0bf2be019449b7248b923ce90/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/abc55490658e0ec0bf2be019449b7248b923ce90/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/abc55490658e0ec0bf2be019449b7248b923ce90/tools/metrics/histograms/enums.xml

Project Member

Comment 14 by sheriffbot@chromium.org, Mar 10

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 12

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

commit ff34ad77e936323bd0b4118d0e248d6dc6769c4c
Author: Tom Sepez <tsepez@chromium.org>
Date: Mon Mar 12 23:24:41 2018

Re-land "Don't share PDF plugins across origin boundaries."

This reverts commit 5ff974fc729167fd8b86376c1a1b25774634906f.

Original CL was https://chromium-review.googlesource.com/953979
This CL differs from the original in that in the test framework,
PDFs are loaded into separate tabs, instead of on top of each
other. Under slow test infrastructure (e.g. MSAN), the plugin
processes would exit before we could complete the loads and
count them. This fixes the test (and the shipping code is unchanged
from the original).

TBR=jam@chromium.org

Bug:  809614 
Change-Id: Id13a97023a60d86fb72c47d580c001b737bf17bf
Reviewed-on: https://chromium-review.googlesource.com/959236
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542652}
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/content/public/common/content_features.cc
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/content/public/common/content_features.h
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/ff34ad77e936323bd0b4118d0e248d6dc6769c4c/content/renderer/render_frame_impl.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Mar 13

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

commit 92a2cd243d427bcfae17afeb03c1f3d634decfe1
Author: Tom Sepez <tsepez@chromium.org>
Date: Tue Mar 13 19:23:09 2018

Reland "Add about:flag for feature PdfIsolation""

This reverts commit abc55490658e0ec0bf2be019449b7248b923ce90.
This cl had to be reverted to allow the revert of its predecessor.
Now that the predecessor is relanded, it can be re-applied.

Original CL at https://chromium-review.googlesource.com/c/chromium/src/+/956488

TBR=jam@chromium.org

Bug:  809614 
Change-Id: I7a4eceed9d8a7df5e7a60ae79b226eab8a0dd9ad
Reviewed-on: https://chromium-review.googlesource.com/961105
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542879}
[modify] https://crrev.com/92a2cd243d427bcfae17afeb03c1f3d634decfe1/chrome/browser/about_flags.cc
[modify] https://crrev.com/92a2cd243d427bcfae17afeb03c1f3d634decfe1/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/92a2cd243d427bcfae17afeb03c1f3d634decfe1/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/92a2cd243d427bcfae17afeb03c1f3d634decfe1/tools/metrics/histograms/enums.xml

Project Member

Comment 17 by sheriffbot@chromium.org, Jun 16

Labels: -Restrict-View-SecurityNotify allpublic
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

Sign in to add a comment