Final review needed for chrome.settingsPrivate API (+security review)

[Oren Blasberg]

Hi apps-dev,

As part of the material design Settings effort [1] we are adding a granular, more secure chrome.settingsPrivate API to set and fetch user/device prefs.

Can we please get a final sign-off on this API, plus a security lgtm?

Ben Kalman has already lgtm'd our API, but then went on vacation. He suggested asking Rockot@ to do a final sign-off, as well as obtaining a security pass.

Relevant links:

• API Proposal
  
• API Design Doc
  
• Implementation CL
  

Thanks!

Oren

[1] http://www.chromium.org/developers/updating-webui-for-material-design/settings-material-design

[Oren Blasberg]

Thanks Ken.

Friendly ping regarding security review.  Joel, do you know someone who might take a quick look?

On Mon, Mar 23, 2015 at 12:54 PM, Ken Rockot <roc...@chromium.org> wrote:

API and CL both LGTM

To unsubscribe from this group and stop receiving emails from it, send an email to apps-dev+u...@chromium.org.

[Joel Weinberger]

Hi everyone. This generally looks reasonable to me assuming that we're locking down this new packaged app in some meaningful way. Which brings up many broader concerns about how this app. Has there been a security review of the new settings app yet? Also, I'm unfamiliar with the current chrome.send() approach to getting settings preferences; is that how it's currently implemented? I thought that all of the preferences and settings pages were WebUI pages, and thus the preferences were type checked at compile time when the WebUI is built?

Also, going forward, please use https://chrome-security-survey.appspot.com/ to submit security review requests. It gives us a much, much better way to track security reviews, and makes sure all of the relevant questions are asked.

[Oren Blasberg]

Hi Joel,

The new Settings packaged app would be bundled with Chrome, and the API would be explicitly whitelisted only to work with that app. However, it's looking like at least for the initial iteration of the new Settings, we will just be loading it in the browser UI and not as a packaged app due to some other technical bumps we hit along the way (I18n being one of them).

Has there been a security review of the new settings app yet?

As mentioned, we haven't been developing it as an "app" exactly; this is still WebUI in fact. The code is located under chrome/browser/resources/settings. See [1]. We weren't aware of a need for a security review done on the new settings other than this very security review, on the new extension API.. Is there such a need? Could you point us to what we need to do, if so?

Also, I'm unfamiliar with the current chrome.send() approach to getting settings preferences; is that how it's currently implemented? I thought that all of the preferences and settings pages were WebUI pages, and thus the preferences were type checked at compile time when the WebUI is built?

Yes, the existing settings pages are WebUI and use chrome.send. The new settings is also WebUI, but we prefer not to use chrome.send because it is fragile -- the C++ code relies on the presence of some javascript method(s) and/or variables existing in the global scope of the JS code in order to work. It also means that if we change one of those method names, it's easy to forget to update the other side, leading to hard-to-track regressions. Using the extension API gives us a nice explicit contract.

I'm not aware of any compile-time type checking on the preferences with the existing settings code. With this new API implementation we enumerate the prefs allowed to be touched by the UI calling into the API, along with their types. So the API code will enforce type correctness at runtime.

[1] http://www.chromium.org/developers/updating-webui-for-material-design/settings-material-design

Thanks!

Oren

[Joel Weinberger]

Hi Oren. Once again, this email has skipped my inbox, so I'm very sorry for the delayed response.

Yes, this API seems good-to-go to me. As for a larger security review for the feature on the whole., I was hoping that there would be a launch bug for this change. This large of a redesign of such a sensitive system seems quite non-trivial and would need, beyond security, proper UX and product approval. As part of a launch bug, you'd fill out a security review at https://chrome-security-survey.appspot.com/ which would help us to much better understand the feature overall. Basically, this approval of the API should not be construed as a security review of the larger redesign and feature.

Thanks!

--Joel