Thanks for filing this; agree that hardening this API seems like a good defense. I'm assigning SecuritySevHigh based on the fact that this is useful in a sandbox escape but requires the precondition of a compromised renderer [1].

Steven: Can you help us find a good owner for this? The original contributor who landed this API doesn't appear to work on Chrome any longer.


[1] https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md#High-severity

I do not have permission to view issue 835887.

We do use a whitelist for settingsPrivate.setPref, however ::prefs::kDownloadDefaultDirectory is explicitly whitelisted so that it can be changed in Settings UI.

Also, this API is only available to WebUI, so can not be used by other renderers. The Settings page or other WebUI would need to be compromised.

This seems like more of a non-CrOS issue since CrOS generally protects system directories.

->dpapad@ since this is a general Settings issue and not CrOS specific.

Using settingsPrivate.setPref API to begin with, predates my involvement with the Settings page. IIUC, this is a larger issue and not just about Downloads, although this is the one being focused here.

Settings WebUI is a highly privileged page, and if compromised lot of things could be set in ways that would be problematic, not just the default downloads directory.

> Modifying prefs like download.default_directory can once again be gated on native UI.

The settingsPrivate API is providing a convenient (although perhaps not considered as secure?) way to flip preferences without having to explicitly add pairs of JS chrome.send and C++ WebUIMessageHandlers or other private API calls.

I can recall at least one case where @stevenjb had to address a similar issue (see  issue 799711 ), by disallowing settingsPrivate to modify the pref, and adding a dedicated quickUnlockPrivate.setLockScreenEnabled method, see [1].

So unless we can guard some or all settingsPrivate.setPref() calls by a user-gesture check on the C++ side, I think it would be beneficial to re-evaluate whether settingsPrivate.setPref() is something we should keep as a whole, or reconsider.


[1] https://chromium-review.googlesource.com/c/chromium/src/+/967321

The lock screen example (which allowed disabling the lock screen, potentially leaving the device in a more vulnerable state) was a clear case where we should not have been using setPref.

It may be worth auditing the entire whitelist in prefs_util.cc:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_private/prefs_util.cc?q=prefs_util&sq=package:chromium&l=96

We should probably determine the correct level of security allowed for JS UI and add a comment to the top of the whitelist.

dpapad@ - Should this be untriaged then? "Available" bugs tend to get lost/forgotten. This seems like something that somebody on the Settings team should own.

Marking as Untrigated instead.

dpapad@ - please find an appropriate owner for this issue per c#4.  Thanks!

This bug is  SecuritySevHigh and marked as a P1. Is this justified? The Settings page has had the same privilleges since M59 basically when it launched (and even the previous Options UI had similar privilleges, with a minor difference that it was not using chrome.settingsPrivate AFAIU).

Moreover the issue discussed here does not seem immediately actionable, since there is not a clear plan, or even agreement on what the ideal solution would look like. Is this basically a matter of preforming additional validation on the C++ side, when the JS side calls setPref?

Realistically, the WebUI team is working at capacity on the MD Refresh and Polymer 2 migration, so it is unlikely for this to make progress within Q2 unless there is a re-prioritization, or unless someone else (not currently working on the above mentioned efforts) can help.

As a first pass, shall we remove prefs::kDownloadDefaultDirectory from the whitelist? 

The downloads section in chrome://settings correctly uses the selectDownloadLocation API to trigger the native UI when the user wants to change the download location. setPref() isn't and shouldn't be used there.

We reference download.default_directory here:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/downloads_page/downloads_page.html?type=cs&q=download.default_directory&sq=package:chromium&l=40

However the control calls:
this.browserProxy_.selectDownloadLocation()

i.e. the JS does not modify the pref directly, we just use the pref for policy indicators.

A straightforward fix might be to introduce a "read only" concept for whitelisted prefs, e.g. maybe add settings_api::PrefType::PREF_TYPE_READ_ONLY_STRING.

> A straightforward fix might be to introduce a "read only" concept for whitelisted prefs, e.g. maybe add settings_api::PrefType::PREF_TYPE_READ_ONLY_STRING.

Doesn't this concept already exist? See https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_private/prefs_util.cc?q=prefs_util&sq=package:chromium&l=71.

So it does. Should be a quick/easy fix then.

I'll put up a quick CL to fix this.

Renaming to match the specific issue here, since settingsPrivate.setPref *is* restricted via a whitelist, it just isn't restricting download.default_directory.

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

commit 4a95aa30d879ea1a8a4ad34f6557c38bc222c542
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue May 15 19:17:05 2018

Make download.default_directory read only in settingsPrivate

Bug:  836362 
Change-Id: I677cffed597ce6dc50247916055bd3182d4a206e
Reviewed-on: https://chromium-review.googlesource.com/1041567
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558802}
[modify] https://crrev.com/4a95aa30d879ea1a8a4ad34f6557c38bc222c542/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/4a95aa30d879ea1a8a4ad34f6557c38bc222c542/chrome/test/data/extensions/api_test/settings_private/test.js

We should definitely merge this into 67.

Not sure if it's possible or critical enough to merge to 66 (it's been around for a long time).

This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

+awhalley@ (Security TPM) for M67 merge review. Pls note CL listed at #14 is not baked in canary yet.

govind@ - good for 67

Approving merge to M67 branch 3396 based on comment #20. Please merge ASAP. Thank you.

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

commit d44ed250c08901e7975ee30ca683b10ba8ed0cd0
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Thu May 17 18:39:31 2018

Make download.default_directory read only in settingsPrivate

TBR=stevenjb@chromium.org

(cherry picked from commit 4a95aa30d879ea1a8a4ad34f6557c38bc222c542)

Bug:  836362 
Change-Id: I677cffed597ce6dc50247916055bd3182d4a206e
Reviewed-on: https://chromium-review.googlesource.com/1041567
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#558802}
Reviewed-on: https://chromium-review.googlesource.com/1064743
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#624}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/d44ed250c08901e7975ee30ca683b10ba8ed0cd0/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/d44ed250c08901e7975ee30ca683b10ba8ed0cd0/chrome/test/data/extensions/api_test/settings_private/test.js

(reward-topanel as part of considering issue 835887)

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