Only allow --install-chrome-app to install apps.

Only allow --install-chrome-app to install apps.

This prevents --install-chrome-app from being used to install
malicious extensions.

BUG=341353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287261

[tapted, 2014-08-01 05:02:38]

lgtm % nits (assuming the restriction shouldn't just be platform apps -- hosted
apps and "legacy packaged" apps will pass this check as well)

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
File chrome/browser/apps/install_chrome_app.cc (right):

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:47: const Callback& callback);
nit: you can probably drop this argument, and pass a null callback in the
constructor instead.

Since it's in an anonymous namespace, I'd probably inline the
constructor/destructor here too, and just have OnManifestParsed declared out of
line, but either is fine.

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:49: protected:
nit: I'd probably start private: here, since nothing inherits from this.

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:50: friend class
base::RefCountedThreadSafe<WebstoreInstallWithPromptAppsOnly>;
nit: is this needed? (I think the template pieces that would complain are not
instantiated, but my brain can't quite compile things that far)

[jackhou1, 2014-08-01 05:26:44]

Yeah, I think allowing hosted and legacy apps is fine too, since they can't
affect normal browser tabs.

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
File chrome/browser/apps/install_chrome_app.cc (right):

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:47: const Callback& callback);
On 2014/08/01 05:02:38, tapted wrote:
> nit: you can probably drop this argument, and pass a null callback in the
> constructor instead.
> 
> Since it's in an anonymous namespace, I'd probably inline the
> constructor/destructor here too, and just have OnManifestParsed declared out
of
> line, but either is fine.

Done.

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:49: protected:
On 2014/08/01 05:02:38, tapted wrote:
> nit: I'd probably start private: here, since nothing inherits from this.

Done.

https://codereview.chromium.org/434843002/diff/1/chrome/browser/apps/install_...
chrome/browser/apps/install_chrome_app.cc:50: friend class
base::RefCountedThreadSafe<WebstoreInstallWithPromptAppsOnly>;
On 2014/08/01 05:02:38, tapted wrote:
> nit: is this needed? (I think the template pieces that would complain are not
> instantiated, but my brain can't quite compile things that far)

Done.

[jackhou1, 2014-08-01 05:26:54]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-01 05:27:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/434843002/20001

[commit-bot: I haz the power, 2014-08-01 14:03:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 16:14:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 16:14:23]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[jackhou1, 2014-08-03 12:32:44]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-03 12:33:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/434843002/20001

[commit-bot: I haz the power, 2014-08-03 14:45:46]

Change committed as 287261

[gab, 2014-08-04 18:00:23]

Thanks, one question below.

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
File chrome/browser/apps/install_chrome_app.cc (right):

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
chrome/browser/apps/install_chrome_app.cc:63: if
(!manifest()->HasKey(extensions::manifest_keys::kApp)) {
Does this manifest come from the third-party provided manifest? If so, how will
this help against malicious side-loading (i.e., if they can just write this bit
in their manifest?).

[jackhou1, 2014-08-05 01:25:25]

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
File chrome/browser/apps/install_chrome_app.cc (right):

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
chrome/browser/apps/install_chrome_app.cc:63: if
(!manifest()->HasKey(extensions::manifest_keys::kApp)) {
On 2014/08/04 18:00:23, gab wrote:
> Does this manifest come from the third-party provided manifest? If so, how
will
> this help against malicious side-loading (i.e., if they can just write this
bit
> in their manifest?).

This manifest comes from the webstore. I'd assume the manifest uploaded by a
third party is sufficiently validated by webstore, but I can double check if
there's any doubt. 

Also, I believe manifest_keys::kApp is how we determine whether an extension is
an app. So if the manifest contains a valid "app" key, we would treat it like an
app and it would not get to extension-only APIs.

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

[gab, 2014-08-05 12:40:51]

Anthony, PTAL at my question below.

Thanks!
Gab

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
File chrome/browser/apps/install_chrome_app.cc (right):

https://codereview.chromium.org/434843002/diff/20001/chrome/browser/apps/inst...
chrome/browser/apps/install_chrome_app.cc:63: if
(!manifest()->HasKey(extensions::manifest_keys::kApp)) {
On 2014/08/05 01:25:25, jackhou1 wrote:
> On 2014/08/04 18:00:23, gab wrote:
> > Does this manifest come from the third-party provided manifest? If so, how
> will
> > this help against malicious side-loading (i.e., if they can just write this
> bit
> > in their manifest?).
> 
> This manifest comes from the webstore. I'd assume the manifest uploaded by a
> third party is sufficiently validated by webstore, but I can double check if
> there's any doubt. 

OKay, I think this will be true in M38, +asargent to confirm.

> 
> Also, I believe manifest_keys::kApp is how we determine whether an extension
is
> an app. So if the manifest contains a valid "app" key, we would treat it like
an
> app and it would not get to extension-only APIs.

Okay cool, do you know where I can find a list of APIs that are extension-only?
(or even better a whitelist of those that are allowed for apps?)

> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

[asargent_no_longer_on_chrome, 2014-08-06 20:46:38]

Sorry for delay in responding.

You can see which permissions, APIs, and manifest-controlled features are
available to extensions, V1 packaged apps (essentially just extensions with
a launch icon), hosted apps, and V2 packaged apps by looking at the
"extension_types" fields in these files:

chrome/common/extensions/api/_api_features.json
chrome/common/extensions/api/_manifest_features.json
chrome/common/extensions/api/_permission_features.json

extensions/common/api/_api_features.json
extensions/common/api/_manifest_features.json
extensions/common/api/_permission_features.json

("legacy_packaged_app" means V1 packaged app, and "platform_app" means V2
packaged app)

The reason we have two copies of these is that there is an ongoing project
to extract as many of the APIs as possible into the extensions/ top-level
component, but it isn't finished yet.




On Tue, Aug 5, 2014 at 5:40 AM, <gab@chromium.org> wrote:

> Anthony, PTAL at my question below.
>
> Thanks!
> Gab
>
>
> https://codereview.chromium.org/434843002/diff/20001/
> chrome/browser/apps/install_chrome_app.cc
> File chrome/browser/apps/install_chrome_app.cc (right):
>
> https://codereview.chromium.org/434843002/diff/20001/
> chrome/browser/apps/install_chrome_app.cc#newcode63
> chrome/browser/apps/install_chrome_app.cc:63: if
> (!manifest()->HasKey(extensions::manifest_keys::kApp)) {
> On 2014/08/05 01:25:25, jackhou1 wrote:
>
>> On 2014/08/04 18:00:23, gab wrote:
>> > Does this manifest come from the third-party provided manifest? If
>>
> so, how
>
>> will
>> > this help against malicious side-loading (i.e., if they can just
>>
> write this
>
>> bit
>> > in their manifest?).
>>
>
>  This manifest comes from the webstore. I'd assume the manifest
>>
> uploaded by a
>
>> third party is sufficiently validated by webstore, but I can double
>>
> check if
>
>> there's any doubt.
>>
>
> OKay, I think this will be true in M38, +asargent to confirm.
>
>
>  Also, I believe manifest_keys::kApp is how we determine whether an
>>
> extension is
>
>> an app. So if the manifest contains a valid "app" key, we would treat
>>
> it like an
>
>> app and it would not get to extension-only APIs.
>>
>
> Okay cool, do you know where I can find a list of APIs that are
> extension-only? (or even better a whitelist of those that are allowed
> for apps?)
>
>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/extensions/common/manifest.cc&l=119
>
> https://codereview.chromium.org/434843002/
>

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