Upstream: Add WebAPK security check in WebappLauncherActivity.

Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122
Committed: https://crrev.com/3429e4e39615924504f2c95c1b97f61fed654881
Cr-Commit-Position: refs/heads/master@{#396547}

[Xi Han, 2016-05-27 14:08:12]

Patchset #1 (id:1) has been deleted

[Xi Han, 2016-05-27 14:09:56]

Description was changed from

==========
Upstream WebAPK security check in WebappLauncherActivity.

BUG=609122
==========

to

==========
Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122
==========

[Xi Han, 2016-05-27 14:10:33]

hanxi@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Xi Han, 2016-05-27 14:10:33]

Hi Peter, could you please take a look? Thanks!

[pkotwicz, 2016-05-27 15:20:16]

LGTM

Looks ready to send over for a security review

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:95:
* Returns whether the MAC is present and valid for the fullscreen PWA to be
opened.
Nit: PWA -> "Progressive Web App" dfalcantara@ will probably have a better name
to put in

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:172:
* @param webapkPackage The package name of the requested WebAPK.
For the sake of consistency: webapkPackage -> webApkPackage

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:181:
boolean isValidWebApk = WebApkValidator.isValidWebApk(this, webapkPackage);
This can be cleaned up a bit. How about:

if (webApkPackage == null) {
    return false;
}

if (!WebApkValidator.isValidWebApk(this, webApkPackage)) {
    Log.d(TAG, "%s is not valid WebAPK", webApkPackage);
    return false;
}

if (!webApkPackage.equals(WebApkValidator.queryWebApkPackage(this, url)) {
    Log.d(TAG, "%s is not within scope of %s WebAPK", url, webApkPackage);
    return false;
}
return true;

You can use "%s" in Log statements thanks to the magic of org.chromium.base.Log

[Xi Han, 2016-05-27 15:34:31]

hanxi@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Xi Han, 2016-05-27 15:34:32]

Hi Dan, could you please take a look? Thanks!

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:172:
* @param webapkPackage The package name of the requested WebAPK.
On 2016/05/27 15:20:16, pkotwicz wrote:
> For the sake of consistency: webapkPackage -> webApkPackage

Done.

https://codereview.chromium.org/2010403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:181:
boolean isValidWebApk = WebApkValidator.isValidWebApk(this, webapkPackage);
On 2016/05/27 15:20:15, pkotwicz wrote:
> This can be cleaned up a bit. How about:
> 
> if (webApkPackage == null) {
>     return false;
> }
> 
> if (!WebApkValidator.isValidWebApk(this, webApkPackage)) {
>     Log.d(TAG, "%s is not valid WebAPK", webApkPackage);
>     return false;
> }
> 
> if (!webApkPackage.equals(WebApkValidator.queryWebApkPackage(this, url)) {
>     Log.d(TAG, "%s is not within scope of %s WebAPK", url, webApkPackage);
>     return false;
> }
> return true;
> 
> You can use "%s" in Log statements thanks to the magic of
org.chromium.base.Log 
>   

Done.

[gone, 2016-05-27 18:20:49]

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:63:
// Permit the launch to a standalone web app frame if:
if any of the following are true

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:95:
* Returns whether the MAC is present and valid for the fullscreen Progressive
Web App to be
Web apps are currently almost never Progressive.

/**
 * Checks whether or not the MAC is present and valid for the web app shortcut.
 *
 * The MAC is used to prevent malicious apps from launching Chrome into a full
screen
 * Activity for phishing attacks (among other reasons).
 *
 * @param url The URL for the web app.
 * @param mac MAC to compare the URL against.  See {@link WebappAuthenticator}.
 * @return Whether the MAC is valid for the URL.
 */
private boolean isValidMacForUrl(String url, String mac) {
}

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:107:
return IntentHandler.wasIntentSenderChrome(intent,
does this not fit on a line?

[Xi Han, 2016-05-27 18:33:49]

Thanks Dan, ptal.

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:63:
// Permit the launch to a standalone web app frame if:
On 2016/05/27 18:20:49, dfalcantara wrote:
> if any of the following are true

Done.

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:95:
* Returns whether the MAC is present and valid for the fullscreen Progressive
Web App to be
On 2016/05/27 18:20:49, dfalcantara wrote:
> Web apps are currently almost never Progressive.
> 
> /**
>  * Checks whether or not the MAC is present and valid for the web app
shortcut.
>  *
>  * The MAC is used to prevent malicious apps from launching Chrome into a full
> screen
>  * Activity for phishing attacks (among other reasons).
>  *
>  * @param url The URL for the web app.
>  * @param mac MAC to compare the URL against.  See {@link
WebappAuthenticator}.
>  * @return Whether the MAC is valid for the URL.
>  */
> private boolean isValidMacForUrl(String url, String mac) {
> }

Thanks!

https://codereview.chromium.org/2010403002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:107:
return IntentHandler.wasIntentSenderChrome(intent,
On 2016/05/27 18:20:49, dfalcantara wrote:
> does this not fit on a line?

Updated.

[gone, 2016-05-27 18:36:33]

lgtm

[Xi Han, 2016-05-27 18:41:10]

hanxi@chromium.org changed reviewers:
+ rsesseta@gmail.com

[Xi Han, 2016-05-27 18:41:10]

Hi Robert, could you please review this CL from security perspective? Thanks!

[Xi Han, 2016-05-27 19:19:22]

hanxi@chromium.org changed reviewers:
+ rsesek@chromium.org
- rsesseta@gmail.com

[Xi Han, 2016-05-27 19:19:41]

Hi Robert, could you please review this CL from the security perspective?

[Robert Sesek, 2016-05-27 20:05:58]

lgtm

[Xi Han, 2016-05-27 20:06:26]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2016-05-27 20:06:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2010403002/#ps60001
(title: "dfalcantara@'s comments.")

[commit-bot: I haz the power, 2016-05-27 20:06:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2010403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2010403002/60001

[commit-bot: I haz the power, 2016-05-27 20:10:56]

Description was changed from

==========
Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122
==========

to

==========
Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122
==========

[commit-bot: I haz the power, 2016-05-27 20:10:57]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-27 20:12:30]

Description was changed from

==========
Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122
==========

to

==========
Upstream: Add WebAPK security check in WebappLauncherActivity.

BUG=609122

Committed: https://crrev.com/3429e4e39615924504f2c95c1b97f61fed654881
Cr-Commit-Position: refs/heads/master@{#396547}
==========

[commit-bot: I haz the power, 2016-05-27 20:12:31]

Patchset 3 (id:??) landed as
https://crrev.com/3429e4e39615924504f2c95c1b97f61fed654881
Cr-Commit-Position: refs/heads/master@{#396547}