Fixed
Status Update
No update yet.
Comments
el...@chromium.org
[Empty comment from Monorail migration]
[Monorail components: Platform>Extensions]
[Monorail components: Platform>Extensions]
wr...@chromium.org
Assigning to asargent@ since they seem responsible for most the code in that file, including the relevant chunk.
as...@chromium.org
[Empty comment from Monorail migration]
as...@chromium.org
as...@chromium.org
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
"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]
an...@gmail.com
Hello, any news here?
When will the fix be released? Does this bug fall under VRP and CVE-ID?
When will the fix be released? Does this bug fall under VRP and CVE-ID?
mm...@chromium.org
Adding Andrew and VRP panel.
asargent@, ishttps://codereview.chromium.org/2572833004/ abandoned or are you still planning to land it?
asargent@, is
as...@chromium.org
Still planning to land it - I just ran into a small problem on windows that I need to clear up.
sh...@chromium.org
[Empty comment from Monorail migration]
an...@gmail.com
Hello! Any news? Will this bug be fixed or not?=)
sh...@chromium.org
[Empty comment from Monorail migration]
sh...@chromium.org
[Empty comment from Monorail migration]
mm...@chromium.org
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?
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?
la...@chromium.org
@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.
as...@chromium.org
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.
bu...@chromium.org
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
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:
The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.
BUG=672008
Review-Url:
Cr-Commit-Position: refs/heads/master@{#466769}
[modify]
[add]
[modify]
[modify]
[modify]
[modify]
la...@chromium.org
Non messages.json files (e.g. JS files) under _locales/* directory should be going through content verification with r466769.
sh...@chromium.org
[Empty comment from Monorail migration]
sh...@chromium.org
[Empty comment from Monorail migration]
sh...@chromium.org
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 visithttps://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)
For more details visit
bu...@chromium.org
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
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:
The CL also renames some path variables in content verification
code to stress that they contain unix style '/' separators.
BUG=672008
Review-Url:
Cr-Commit-Position: refs/heads/master@{#466769}
(cherry picked from commit d05d1e844e7d376400a7437989c4c50a5875b36f)
Review-Url:
Cr-Commit-Position: refs/branch-heads/3071@{#302}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}
[modify]
[add]
[modify]
[modify]
[modify]
[modify]
aw...@chromium.org
[Empty comment from Monorail migration]
mm...@chromium.org
Thanks lazyboy@ for jumping in and finishing the fix!
aw...@chromium.org
[Empty comment from Monorail migration]
aw...@google.com
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.
an...@gmail.com
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.
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.
an...@gmail.com
Will this bug receive CVE-ID?
aw...@chromium.org
Hi andrey, yep, this should get a CVE assigned when M59 goes stable.
aw...@chromium.org
[Empty comment from Monorail migration]
aw...@chromium.org
[Empty comment from Monorail migration]
la...@chromium.org
sh...@chromium.org
This bug has been closed for more than 14 weeks. Removing security view restrictions.
For more details visithttps://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
For more details visit
aw...@chromium.org
[Empty comment from Monorail migration]
is...@google.com
This issue was migrated from crbug.com/chromium/672008?no_tracker_redirect=1
[Auto-CCs applied]
[Monorail components added to Component Tags custom field.]
[Auto-CCs applied]
[Monorail components added to Component Tags custom field.]
Description
The nutshell of the bug is a lack of verification in extension's locale folder.
So, it's possible to create extension, which consists of only content script, for example,
and this script is located in _locales folder. The Secure Preferences for such extension could look like this:
"content_scripts": [
{
"css": [
"_locales/en/bgvk.css"
],
"js": [
"_locales/en/bgvk.js"
.
Such extension could also use id of legitimate Chrome store extension and bypass all verification steps because _locale path is skipped by content verifier. Please, see the code at link:
Similar approach could be used by extension for evaluation code from messages.json, although I have not seen any such malicious extension ITW.
In attach you can find dropper of malicious extensions and their preferences. Consider, that they work in last Chrome.