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

Issue 821613 link

Starred by 3 users

Restrict PDFium extension from running script inside chrome:// URLs

Project Member Reported by creis@chromium.org, Mar 13 2018

Issue description

As reported in  issue 821138  (under issue 820838), the PDFium extension has the ability to run script inside chrome:// URLs.  This is due to the <all_urls> permission in its manifest (and possibly the fact it's a component extension):
https://cs.chromium.org/chromium/src/chrome/browser/resources/pdf/manifest.json

As a result, the extension process can use chrome.tabs.update with a JavaScript URL or chrome.tabs.executeScript to run script inside of a WebUI page, which can be used as a step in a sandbox escape.


 

Comment 1 by dcheng@chromium.org, Mar 13 2018

Cc: sa...@chromium.org
I tried changing <all_urls> host permissions to chrome://resources/*, and everything still appears to work.

raymes, any idea why we needed the host permissions in https://chromium.googlesource.com/chromium/src/+/c064ddad5554afe987ef088d4bf1165cc7eda74e are needed? Did we change how we pass pdf streams to the plugin?

Comment 2 by dcheng@chromium.org, Mar 14 2018

Cc: kinuko@chromium.org
> (and possibly the fact it's a component extension)
Definitely :) We certainly don't allow non-component extensions to run script on chrome:// pages.

It definitely seems like we should be able to restrict this significantly to specify appropriate hosts.  In  issue 821138 , you mentioned that the extension needs to fetch resources from chrome://print - is that the case?  If so, how did it work when we just restricted to chrome://resources?

Looking farther ahead, it would be nice to not auto-grant chrome:// pages to component extensions, even if they request <all_urls>.  Unfortunately, this might be a breaking change, and would involve auditing (and possibly updating) existing extensions.  Still absolutely worth doing, but not something we can do as an immediate fix (unlike, perhaps, updating the pdf extension).

Comment 4 by dcheng@chromium.org, Mar 14 2018

#3: I think the way that the PDFium plugin is passed the pdf resource has changed to no longer depend on host permissions for this.

I audited all the other (hopefully) component extension manifests. No other extension appears to specify <all_urls>; the only things I saw were chrome://theme and chrome://resources. So maybe we can already do that change?

Comment 5 by raymes@chromium.org, Mar 14 2018

sammc and I checked a few other things and the fix seems good. Namely we were a bit worried about network mounts and range requests but they work fine.

In the process we discovered that we should lock down the plugin to only be able to make requests on the same origin as the PDF that it's loading (currently it can make cross origin requests, as can flash). But I'll file a separate bug for this.
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 14 2018

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

commit d8206c55039dc71555ea92bde3080c8743ad11b8
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Mar 14 08:40:45 2018

Only grant host permissions for chrome://resources/* to PDF component extension

Bug:  821613 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Icdaab0eb77a72a7b6ad0c491424672614c24a9fa
Reviewed-on: https://chromium-review.googlesource.com/961727
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543030}
[modify] https://crrev.com/d8206c55039dc71555ea92bde3080c8743ad11b8/chrome/browser/resources/pdf/manifest.json

Comment 7 by dcheng@chromium.org, Mar 14 2018

NextAction: 2018-03-15

Comment 8 by dcheng@chromium.org, Mar 14 2018

Labels: Security_Severity-High M-65 Security_Impact-Stable

Comment 9 by dcheng@chromium.org, Mar 14 2018

Status: Fixed (was: Assigned)
Project Member

Comment 10 by sheriffbot@chromium.org, Mar 14 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: pbomm...@chromium.org gov...@chromium.org

Comment 12 Deleted

+awhalley@(Security TPM), is this block M65 further roll out?
Nope, will not block rollout.
Labels: M-66
OK, Is this need to be included in next week M65 respin? The change is not yet baked in canary/dev/beta.




Ideally, yes. I've set a next action to check on the next canary with this CL.
The NextAction date has arrived: 2018-03-15

Comment 18 by creis@chromium.org, Mar 15 2018

dcheng@: Thanks for following up to verify and request merge.

One sanity check: was the <all_urls> line necessary in the past, and stopped being necessary at some point?  Or was it never necessary?  Just want to make sure we're not risking a regression when we merge this to M66 and M65.
This won't make a dev or beta before the stable merge is needed, so it doesn't meet the normal bar for a stable merge. We'll pick it up in 66 (or a later 65 spin if any)
I tried to figure out what changed to make this no longer require host permissions. In a version of Chrome that predates sammc's fix for opening pdfs on shares (https://chromium.googlesource.com/chromium/src/+/c064ddad5554afe987ef088d4bf1165cc7eda74e), here's the error:
Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' chrome://resources".

pdf.js:465 Not allowed to load local resource: file://dcheng2-w/Users/dcheng/Downloads/pdftest/f1040.pdfpdf.js:465 PDFViewer.afterZoom_
extensions::uncaught_exception_handler:8 Error in response to tabs.setZoomSettings: TypeError: this.plugin_.postMessage is not a function
    at Object.PDFViewer.afterZoom_ (chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/pdf.js:465:18)
    at Object.PDFViewer (chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/pdf.js:132:17)
    at initViewer (chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/main.js:38:14)
    at Object.<anonymous> (chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/main.js:80:7)

Examining PDFViewer.plugin_ gives:
<embed id="plugin" type="application/x-google-chrome-pdf" src="file://dcheng2-w/Users/dcheng/Downloads/pdftest/f1040.pdf" stream-url="blob:chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/36C0D1AC-8D53-4202-AD68-373E279EF494" headers="" full-frame="" style="height: 1247px; width: 1249px;">

So the source is directly embedded, which would explain the console error.

However, looking a recent build of Chrome and examining PDFViewer.plugin_ *also* gives this as the outerHTML:
<embed id="plugin" type="application/x-google-chrome-pdf" src="file://dcheng2-w/Users/dcheng/Downloads/pdftest/f1040.pdf" stream-url="blob:chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/cdcd072f-acd2-4361-a8aa-edb383be80dc" headers="" background-color="0xFF525659" top-toolbar-height="56" full-frame="">

Which is essentially the same thing but somehow doesn't trigger any errors now. I've also verified that the PDF viewer still works on Canary, which does have my CL... so I have no clue. Maybe we changed some plumbing for mime_handler_view...?

One potential CL that seems like it could have affected this? It mentions fixing something for top-level PDFs: maybe embedding PDFs on file:// 
https://chromium.googlesource.com/chromium/src/+/1d0b49e4794131e5f53265bf85d961020a725a30

Though it doesn't seem like a great candidate. raymes, sammc: any ideas?
Not sure if this helps, but here's the stack where we hit the local load failure message in the build from https://commondatastorage.googleapis.com/chromium-browser-continuous/index.html?prefix=Win/313166/:

00 009bdbc4 69a10580 chrome_child!blink::Document::addConsoleMessage [c:\b\build\slave\win\build\src\third_party\webkit\source\core\dom\document.cpp @ 4914]
01 009bdbe4 69931693 chrome_child!blink::FrameLoader::reportLocalLoadFailed+0xd0 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\loader\frameloader.cpp @ 823]
02 009bdc04 699319f3 chrome_child!blink::HTMLPlugInElement::pluginIsLoadable+0x63 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 631]
03 009bdc70 69975006 chrome_child!blink::HTMLPlugInElement::requestObject+0xa3 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 503]
04 009bdcac 69931f92 chrome_child!blink::HTMLEmbedElement::updateWidgetInternal+0xa6 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlembedelement.cpp @ 150]
05 009bdcb4 69a09817 chrome_child!blink::HTMLPlugInElement::updateWidget+0x12 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 189]
06 009bdcec 69a027aa chrome_child!blink::FrameView::updateWidgets+0xa7 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\frame\frameview.cpp @ 2015]
07 (Inline) -------- chrome_child!blink::FrameView::updateWidgetsTimerFired+0x18 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\frame\frameview.cpp @ 2031]
08 009bdcf8 698d3571 chrome_child!blink::FrameView::flushAnyPendingPostLayoutTasks+0x4a [c:\b\build\slave\win\build\src\third_party\webkit\source\core\frame\frameview.cpp @ 2042]
09 009bdd0c 699318b0 chrome_child!blink::Document::updateLayoutIgnorePendingStylesheets+0x91 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\dom\document.cpp @ 1960]
0a 009bdd18 699317ce chrome_child!blink::HTMLPlugInElement::renderPartForJSBindings+0x10 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 399]
0b (Inline) -------- chrome_child!blink::HTMLPlugInElement::pluginWidgetForJSBindings+0x8 [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 334]
0c 009bdd2c 69f4cc9c chrome_child!blink::HTMLPlugInElement::pluginWrapper+0x3e [c:\b\build\slave\win\build\src\third_party\webkit\source\core\html\htmlpluginelement.cpp @ 317]
0d 009bdd68 69f4d159 chrome_child!blink::`anonymous namespace'::getScriptableObjectProperty<blink::V8HTMLEmbedElement,v8::Local<v8::String> >+0x2c [c:\b\build\slave\win\build\src\third_party\webkit\source\bindings\core\v8\custom\v8htmlpluginelementcustom.cpp @ 53]
0e 009bdd78 69f023f9 chrome_child!blink::V8HTMLObjectElement::namedPropertyGetterCustom+0x39 [c:\b\build\slave\win\build\src\third_party\webkit\source\bindings\core\v8\custom\v8htmlpluginelementcustom.cpp @ 121]
0f 009bdd88 696d599e chrome_child!blink::HTMLObjectElementV8Internal::namedPropertyGetterCallback+0x19 [c:\b\build\slave\win\build\src\out\release\gen\blink\bindings\core\v8\v8htmlobjectelement.cpp @ 719]
10 009bddb4 695e06a7 chrome_child!v8::internal::PropertyCallbackArguments::Call+0x6e [c:\b\build\slave\win\build\src\v8\src\arguments.cc @ 87]
11 009bde04 695df8c1 chrome_child!v8::internal::JSObject::GetPropertyWithInterceptor+0x127 [c:\b\build\slave\win\build\src\v8\src\objects.cc @ 13555]
12 009bde2c 696db34c chrome_child!v8::internal::Object::GetProperty+0x31 [c:\b\build\slave\win\build\src\v8\src\objects.cc @ 123]
13 009bdebc 696db9a5 chrome_child!v8::internal::LoadIC::Load+0x2fc [c:\b\build\slave\win\build\src\v8\src\ic\ic.cc @ 742]
14 (Inline) -------- chrome_child!v8::internal::__RT_impl_LoadIC_Miss+0x1cb [c:\b\build\slave\win\build\src\v8\src\ic\ic.cc @ 2293]
15 009bdff0 2ea0d4dc chrome_child!v8::internal::LoadIC_Miss+0x1d5 [c:\b\build\slave\win\build\src\v8\src\ic\ic.cc @ 2260]
WARNING: Frame IP not in any known module. Following frames may be wrong.
16 009be010 3dac9f58 0x2ea0d4dc
17 009be034 2ea30755 0x3dac9f58
18 009be050 2ea168ca 0x2ea30755
19 009be08c 695fff83 0x2ea168ca
1a 009be0e8 695ff15c chrome_child!v8::internal::Invoke+0x213 [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 129]
1b 009be11c 697b15d9 chrome_child!v8::internal::Execution::Call+0x13c [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 180]
1c (Inline) -------- chrome_child!v8::internal::__RT_impl_Runtime_Apply+0x1c9 [c:\b\build\slave\win\build\src\v8\src\runtime\runtime-function.cc @ 607]
1d 009be1a0 2ea0d4dc chrome_child!v8::internal::Runtime_Apply+0x1d9 [c:\b\build\slave\win\build\src\v8\src\runtime\runtime-function.cc @ 574]
1e 009be1c0 3da16eb2 0x2ea0d4dc
1f 009be200 2ea32c55 0x3da16eb2
20 009be220 2ea30755 0x2ea32c55
21 009be244 2ea168ca 0x2ea30755
22 009be280 695fff83 0x2ea168ca
23 009be2dc 695ff15c chrome_child!v8::internal::Invoke+0x213 [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 129]
24 009be310 695ba6b5 chrome_child!v8::internal::Execution::Call+0x13c [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 180]
25 009be350 6a4dee49 chrome_child!v8::Function::Call+0xb5 [c:\b\build\slave\win\build\src\v8\src\api.cc @ 4028]
26 009be444 696d57be chrome_child!extensions::`anonymous namespace'::ExtensionImpl::Apply+0x2d9 [c:\b\build\slave\win\build\src\extensions\renderer\safe_builtins.cc @ 192]
27 009be478 696a50dd chrome_child!v8::internal::FunctionCallbackArguments::Call+0x7e [c:\b\build\slave\win\build\src\v8\src\arguments.cc @ 34]
28 009be4e4 696a6afd chrome_child!v8::internal::HandleApiCallHelper<0>+0x20d [c:\b\build\slave\win\build\src\v8\src\builtins.cc @ 1078]
29 009be508 2ea0d4dc chrome_child!v8::internal::Builtin_implHandleApiCall+0x2d [c:\b\build\slave\win\build\src\v8\src\builtins.cc @ 1100]
2a 009be528 3da2ad63 0x2ea0d4dc
2b 009be564 2ea0ce7b 0x3da2ad63
2c 009be57c 3dac76d5 0x2ea0ce7b
2d 009be5b4 3dadc8a8 0x3dac76d5
2e 009be5f8 2ea30755 0x3dadc8a8
2f 009be628 2ea168ca 0x2ea30755
30 009be664 695fff83 0x2ea168ca
31 009be6c0 695ff15c chrome_child!v8::internal::Invoke+0x213 [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 129]
32 009be6f4 695ba6b5 chrome_child!v8::internal::Execution::Call+0x13c [c:\b\build\slave\win\build\src\v8\src\execution.cc @ 180]
33 009be734 69f441a3 chrome_child!v8::Function::Call+0xb5 [c:\b\build\slave\win\build\src\v8\src\api.cc @ 4028]
34 009be774 69dd6d64 chrome_child!blink::V8ScriptRunner::callFunction+0xe3 [c:\b\build\slave\win\build\src\third_party\webkit\source\bindings\core\v8\v8scriptrunner.cpp @ 428]
35 009be7d8 69dd6be1 chrome_child!blink::ScriptController::callFunction+0x154 [c:\b\build\slave\win\build\src\third_party\webkit\source\bindings\core\v8\scriptcontroller.cpp @ 165]
36 009be804 69872c21 chrome_child!blink::ScriptController::callFunction+0x41 [c:\b\build\slave\win\build\src\third_party\webkit\source\bindings\core\v8\scriptcontroller.cpp @ 149]
37 009be820 6a4ccc96 chrome_child!blink::WebLocalFrameImpl::callFunctionEvenIfScriptDisabled+0x21 [c:\b\build\slave\win\build\src\third_party\webkit\source\web\weblocalframeimpl.cpp @ 856]
38 009be85c 6a4d9870 chrome_child!extensions::ScriptContext::CallFunction+0xa6 [c:\b\build\slave\win\build\src\extensions\renderer\script_context.cc @ 157]
39 009bea6c 6a4cd7f5 chrome_child!extensions::ModuleSystem::CallModuleMethod+0x2e0 [c:\b\build\slave\win\build\src\extensions\renderer\module_system.cc @ 295]
3a 009beaf8 6a4dfad5 chrome_child!extensions::ScriptContext::OnResponseReceived+0x135 [c:\b\build\slave\win\build\src\extensions\renderer\script_context.cc @ 272]
3b 009beb24 6a4d4d1b chrome_child!extensions::RequestSender::HandleResponse+0x45 [c:\b\build\slave\win\build\src\extensions\renderer\request_sender.cc @ 139]
3c (Inline) -------- chrome_child!DispatchToMethodImpl+0x17 [c:\b\build\slave\win\build\src\base\tuple.h @ 246]
3d (Inline) -------- chrome_child!DispatchToMethod+0x17 [c:\b\build\slave\win\build\src\base\tuple.h @ 253]
3e 009beb80 6a4d5683 chrome_child!ExtensionMsg_Response::Dispatch<extensions::ExtensionHelper,extensions::ExtensionHelper,void,void (__thiscall extensions::ExtensionHelper::*)(int,bool,base::ListValue const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &)>+0x5b [c:\b\build\slave\win\build\src\extensions\common\extension_messages.h @ 382]
3f 009bec64 6a1912e7 chrome_child!extensions::ExtensionHelper::OnMessageReceived+0x73 [c:\b\build\slave\win\build\src\extensions\renderer\extension_helper.cc @ 136]
40 009bf074 6a70911e chrome_child!content::RenderViewImpl::OnMessageReceived+0xc7 [c:\b\build\slave\win\build\src\content\renderer\render_view_impl.cc @ 1282]

What's happening is accessing the post message property on plugin_ is forcing it to be instantiated--but instantiation fails since it's not supposed to be able to load a local resource.

So actually, the lack of a failure on new builds is actually really suspicious. Is the PDF viewer enforcing its own security policy somewhere? I don't see any errors (like I would expect) in the console output when specifying a file:// pdf in a top-level about:blank page, but the embed doesn't seem to be loading either...
This is a good question and I don't know the answer. One thing I noticed though is that it looks like the code for checking whether the src can be loaded in blink has been refactored somewhat so my theory is that something in this code has changed (this check may possibly be broken altogether..).

I would also add that although blink was throwing up an error here, we could easily work around it. I don't believe that it's important that we set the src= attribute. The plugin does all of its own URL loading, so we could equally pass that URL in under a different attribute. blink does not actually know whether the pepper plugin will make a request to the URL in src so in that sense I think it's a false positive.
Cc: japhet@chromium.org
Raymes: I've been trying to bisect this down a bit. I can confirm that this doesn't seem to be because of https://codereview.chromium.org/2842253002.

So at this point, I'm really not sure what made this start working. It was some commit between r313214 and r474107: I checked out r474106 and updated PDF component extension manifest to match the latest changes... and it worked.

I wanted to test https://codereview.chromium.org/1645313002 as well to narrow things down a bit, but I wasn't able to build (GN has evolved in ways that aren't backwards compatible, it would seem).

I've already burned too much time on this today. We should try to understand what's going on with the embed code here, but that can be followed up separately. On the bright side, that means the fix here is likely safe for merge to M66 (and M65), in that it shouldn't explode there. =)
I'm OOO today but I can help next week or sammc@ may be able to take a look. It may be easier to bisect by making a small extension that reproduces the issue (potentially with flash or the test plugin) and using the bisect tool with it. 
OK, I bisected the embed src error down to https://chromium.googlesource.com/chromium/src/+log/a1ebacdb1bbacbf534ef9f8dc763bc77273fdfd0..3617b0eea7ec74b8e731a23fed2f4070cbc284c4. So perhaps I made an error in my build earlier?
After some further thought, I'm pretty sure the actual error message is orthogonal to the host permissions that were previously needed.

Even without the console error printed, <embed src="file://..."> doesn't work if the owner document isn't also on a file:// URL: a plugin is never loaded. So the only thing that r474108 changed is that the console error is no longer printed.

So something changed somewhere else such that the PDFium component extension doesn't trigger the security check for embedding file:// URLs in non-file:// contexts... but only in the component extension.

Furthermore, whatever changed doesn't seem to depend on extension host permissions. So if we can build a copy of the PDFium component extension with chrome://resources/ as the only host permission, we should be able to bisect exactly when that change occurred.
I did some digging. I'm wondering if r474108 really is the culprit, though I can't verify it at all. At the very least I suspect r474108 changes more than just whether the console warning is printed. It removes a check from AllowedToLoadObject which queries the SecurityOrigin::CanDisplay (which in turn queries CanRequest). This is the same whitelist that is manipulated by host permissions (see https://cs.chromium.org/chromium/src/extensions/renderer/dispatcher.cc?rcl=2ff761dbda62eda83a4a8378b7741c9cfeaf791b&l=1274). 

Without that check, AFAICT the only thing that gets queried when loading an <embed> plugin is the CSP. Those checks should pass because we whitelist all origins for object-src. So as far as I can tell from looking into the code everything points toward r474108. With that said, I still believe you when you say the <embed> still doesn't load and that seems weird.

If you can't figure this out by my time tomorrow morning I can come into the office and dig into it. I'm still very suspicious of r474108 though. Did you try adding that check back into HTMLPlugInElement.cpp on a ToT build to see if it repros the issue? 
Project Member

Comment 28 by sheriffbot@chromium.org, Mar 16 2018

Labels: Merge-Request-66
Project Member

Comment 29 by sheriffbot@chromium.org, Mar 16 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Ok, on ToT adding back in the GetDocument().GetSecurityOrigin()->CanDisplay(url) check in HTMLPlugInElement::AllowedToLoadObject from r474108 causes the plugin not to load, which is what is expected if r474108 is the culprit. It fails in exactly the same way as in  issue 452319 , namely:
If the manifest.json includes permissions for file://*/ then local file URLs will load, but network mounts don't load.

The exact codepath that is being hit is SecurityOrigin::CanDisplay which checks SchemeRegistry::ShouldTreatURLSchemeAsLocal which returns true for file: URLs. It then queries CanLoadLocalResources() and SecurityPolicy::IsAccessToURLWhiteListed() which both return false when loading a network mount while IsAccessToURLWhiteListed returns true with a local file URL. This is likely due to pattern matching not matching the network mount URLs though I didn't dig that far. With <all_urls> specified IsAccessToURLWhiteListed seems to return true regardless. 

I wasn't able to check out and build much older versions nearer to sammc's CL (I spent way too long trying without success) but based on the above I feel fairly confident that r474108 is the issue. 
Does #6 need to be merged, or are there other CLs as well?

Comment 32 by creis@chromium.org, Mar 19 2018

Comment 31: dcheng@ is out today, but yes, I think we would just need to merge r543030 from comment 6.

However, I'm having trouble following the discussion about japhet@'s r474108 and what the implications are.  raymes@/sammc@/japhet@: Is it safe to proceed with the merge to M66, or do we need to do something more?  (If we can merge, we'd like to do it ASAP.)
tldr, I believe it's safe to proceed with a merge to M66. r474108 removed the check that was previously the reason for needing the host permissions.

There is an outstanding question about whether that is a regression or intended change. It's hard to know exactly why those checks were there (I'm sure it's steeped in history). I'll file a bug and assign to japhet to follow up on that. But that was landed 10 months ago. So I think we should merge this as a stop-gap. Does that make sense? 
Filed  issue 821613  to follow up on r474108.

Comment 35 by creis@chromium.org, Mar 19 2018

Comments 33-34: Perfect, thanks!  And it looks like issue 823518 was the one you meant in comment 34.  :)

abdulsyed@: Is it ok to merge r543030 to M66, given comment 33?  It sounds like it's safe back to 60.0.3109.0, when r474108 landed.

Comment 36 by creis@chromium.org, Mar 20 2018

Cc: falken@chromium.org
+falken@ for context on ensuring we don't allow ServiceWorkers on chrome:// URLs in the followup issue 823475.
Labels: -Merge-Review-66 Merge-Approved-66
Approving merge to M66. Branch:3359
Project Member

Comment 38 by bugdroid1@chromium.org, Mar 20 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf287aafe15dc5f54bb75ff1d4b74c9a9050b6b8

commit bf287aafe15dc5f54bb75ff1d4b74c9a9050b6b8
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Mar 20 17:55:05 2018

Only grant host permissions for chrome://resources/* to PDF component extension

TBR=dcheng@chromium.org

(cherry picked from commit d8206c55039dc71555ea92bde3080c8743ad11b8)

Bug:  821613 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Icdaab0eb77a72a7b6ad0c491424672614c24a9fa
Reviewed-on: https://chromium-review.googlesource.com/961727
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#543030}
Reviewed-on: https://chromium-review.googlesource.com/971403
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#349}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/bf287aafe15dc5f54bb75ff1d4b74c9a9050b6b8/chrome/browser/resources/pdf/manifest.json

Labels: Merge-Request-65
Even though the exploit doesn't currently work in M65, that's only because the reporter used a M66 and newer exploit to get code execution. If a bug is found in the V8 that ships with M65, we'll have the same bug there.

As the manifest change should be safe, I'm going to pre-emptively request a merge in case there's another M65 respin.
+awhalley@ for M65 merge review.

Comment 41 Deleted

dcheng@ - now the change in #38 has been out in Beta, mind double checking there's nothing that looks related in the crash reports?

Assuming it's good, and it's a safe, easy merge, govind@ - good for 65
I compared the top 200 crashes between the previous and current beta versions, and don't see anything that could be obviously related.
dcheng@ - thanks!

govind@ - Good for 65.
Labels: -Merge-Request-65 Merge-Approved-65
Approving merge to M65 branch 3325 based n comments #43, #44 and per offline chat with  awhalley@. 

Note: We won't respin M65 for this but we will pick it up for next respin (if any). 
Project Member

Comment 46 by bugdroid1@chromium.org, Mar 26 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/215a3b7b569cb804b9a0fce0dc4e1b59fbb279a7

commit 215a3b7b569cb804b9a0fce0dc4e1b59fbb279a7
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Mar 26 18:36:05 2018

Only grant host permissions for chrome://resources/* to PDF component extension

TBR=dcheng@chromium.org

(cherry picked from commit d8206c55039dc71555ea92bde3080c8743ad11b8)

Bug:  821613 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Icdaab0eb77a72a7b6ad0c491424672614c24a9fa
Reviewed-on: https://chromium-review.googlesource.com/961727
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#543030}
Reviewed-on: https://chromium-review.googlesource.com/981112
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#742}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/215a3b7b569cb804b9a0fce0dc4e1b59fbb279a7/chrome/browser/resources/pdf/manifest.json

Labels: Release-0-M66
Project Member

Comment 48 by sheriffbot@chromium.org, Jun 20 2018

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