Assigning to asargent@ since they seem responsible for most the code in that file, including the relevant chunk.

I have a CL I'm about to send out for review fixing this:

https://codereview.chromium.org/2572833004/

elawrence asked a good question on the codereview:

"In the bug, one thing he mentioned was "Similar approach could be used by
extension for evaluation code from messages.json". This patch doesn't address
that concern, does it?"

The answer is that no, the CL does not address this concern, and doing so properly on the client side may be a much larger endeavor. Right now during the extension install process we use a sandbox to read and parse a subset of files that we later want to use directly in the browser process, and then pass those off to be rewritten back in the browser process at the end of the install process. This includes manifest.json and messages.json files (JSONReader->base::Value in the sandbox, then written out with JSONWriter back in the browser process) and images used as icons (content::DecodeImage->SkBitmap in the sandbox, then written out with gfx::PNGCodec::EncodeBGRASkBitmap back in the browser process). Further, the manifest.json file then gets copied into the Secure Preferences file. We currently throw out the original copies of these files, and the transformations done on them are "lossy" from a content verification perspective - JSON dictionary key/val pairs can get reordered and whitespace isn't preserved, and images are transcoded to PNG, so the binary block-level hashes aren't useful.

To be able to verify them on the client side, we need to preserve these files in their original condition, and either (a) always use a sandbox to parse them and pass the results to the browser process over IPC, incurring extra latency, or (b) cache the transcoded copies somewhere else and either secure that similar to Secure Preferences, or every time we use them asynchronously verify the original and that running the transcoding results in the same thing as what we have cached. Since extensions block startup (proxy settings API, content scripts with run_at: document_start, use blocking web request, etc.), the extra latency incurred by (a) might cause unacceptable startup slowdown. 

Things to keep in mind:

-Content verification is designed primarily as way to draw a clearer line between non-malicious behavior (using documented APIs) and malicious behavior (modifying user preferences files, extension data files, or the chrome binary itself). As a security mechanism, it cannot be a complete mitigation against a threat model of local binary software that can modify arbitrary files on disk; it can only make that a little more difficult, because ultimately that software can always just patch the chrome binary itself to turn off any checks we put in. 

-I think it is likely rare for extensions to eval code from strings in messages.json (I have not verified this) - note that the default Content Security Policy for extensions does not even allow eval [1] . Also we can possibly look for this on the server side during the malware detection pipeline and feed this into other systems like the Safe Browsing backend. 

-Many extensions currently include remote script via script tags, which can return arbitrary content that we don't necessarily have visibility into, although we have some ideas for how to do better on this front. 


[1] https://developer.chrome.com/extensions/contentSecurityPolicy

Hello, any news here? 
When will the fix be released? Does this bug fall under VRP and CVE-ID? 

Adding Andrew and VRP panel.

asargent@, is https://codereview.chromium.org/2572833004/ abandoned or are you still planning to land it?

Still planning to land it - I just ran into a small problem on windows that I need to clear up. 

Hello! Any news? Will this bug be fixed or not?=) 

lazyboy@, may I assign this to you, since you've reviewed the CL (https://codereview.chromium.org/2572833004/).

asargent@ is not replying, so we need to find a new owner for this issue. As per c#8, it looks like the fix was almost ready. Could you please help to find an owner to finish this?

@13, sure. I'll follow up with Antony to see if I can land this. What I remember is that the CL had some test failures in one or more platform.

My apologies for being slow to respond! (I've had a hard time finding time to get back to this given my new responsibilities on another team.) I gave lazyboy@ a braindump in chat and can answer any follow-up questions. 

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

commit d05d1e844e7d376400a7437989c4c50a5875b36f
Author: lazyboy <lazyboy@chromium.org>
Date: Mon Apr 24 21:20:42 2017

Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG= 672008 

Review-Url: https://codereview.chromium.org/2834873002
Cr-Commit-Position: refs/heads/master@{#466769}

[modify] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/chrome/browser/extensions/content_verifier_browsertest.cc
[add] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/chrome/test/data/extensions/content_verifier/content_script_locales.crx
[modify] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/extensions/browser/content_verifier.cc
[modify] https://crrev.com/d05d1e844e7d376400a7437989c4c50a5875b36f/extensions/browser/content_verifier.h

Non messages.json files (e.g. JS files) under _locales/* directory should be going through content verification with r466769.

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

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

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

commit 503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Fri Apr 28 20:40:34 2017

[Merge m59] Only whitelist messages.json files in _locales for content verification

The messages.json files used for i18n/l10n get transcoded during the
extension install process, so we need to avoid doing content
verification checks on them because we expect their contents to be
modified. The code we had for testing whether a path should be skipped
because of this was incorrect, skipping more files than it should.

Through the courtesy of Antony: https://crrev.com/2572833004

The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.

BUG= 672008 

Review-Url: https://codereview.chromium.org/2834873002
Cr-Commit-Position: refs/heads/master@{#466769}
(cherry picked from commit d05d1e844e7d376400a7437989c4c50a5875b36f)

Review-Url: https://codereview.chromium.org/2845223003 .
Cr-Commit-Position: refs/branch-heads/3071@{#302}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/chrome/browser/extensions/content_verifier_browsertest.cc
[add] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/chrome/test/data/extensions/content_verifier/content_script_locales.crx
[modify] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/extensions/browser/content_verifier.cc
[modify] https://crrev.com/503cfcd789d2d9769c2e1a6a1fe53b7be00c59ce/extensions/browser/content_verifier.h

Thanks lazyboy@ for jumping in and finishing the fix!

Please, check interesting example, that uses code form messages.json:

https://chrome.google.com/webstore/detail/vkokdl-adb/gfnnopbpnohffikkfecpgfcanoahilfm

I'm afraid the panel decided not to award for this bug, since verification is a defense against on disk manipulation by another process, which is outside our threat model.

Actually this technique could be used not only for patching extensions on disk but also for hiding malicious extensions. 

Let's assume, that I have good extension in store with code in locales or messages.json and bad extension, which I spread through my own installer (like Skype or other good applications do). Content verification should protect users from second. 

Will this bug receive CVE-ID?

Hi andrey, yep, this should get a CVE assigned when M59 goes stable.

Any chance this change caused this issue: https://groups.google.com/a/chromium.org/forum/#!topic/extensions-dev/wnNxx2wps5I ?

@elawrence that would be  https://crbug.com/720597  (which is a dupe of  issue 729978 )

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