Commit 15120efa authored by rob's avatar rob Committed by Commit bot

Fix XSS in app launcher and remove use of unvalidated URL

The third parameter of "launchApp" is only used for the webstore app,
and used to append utm_source=chrome-ntp-icon to the app URL.
But the launchApp handler did not validate that the URL is safe.
To fix that issue, I specialize the parameter for launchApp: It now takes the
source string ("chrome-ntp-icon") instead of a URL without validation.

BUG=668665
TEST=Manually using test case from bug report. Also opened the app launcher and
verified that clicking on the Webstore icon still leads to the same place.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2527413002
Cr-Commit-Position: refs/heads/master@{#434939}
parent 22733070
......@@ -396,13 +396,8 @@ cr.define('ntp', function() {
onClick_: function(e) {
if (/** @type {MouseEvent} */(e).button > 1) return;
var url = !this.appData_.is_webstore ? '' :
appendParam(this.appData_.url,
'utm_source',
'chrome-ntp-icon');
chrome.send('launchApp',
[this.appId, APP_LAUNCH.NTP_APPS_MAXIMIZED, url,
[this.appId, APP_LAUNCH.NTP_APPS_MAXIMIZED, 'chrome-ntp-icon',
e.button, e.altKey, e.ctrlKey, e.metaKey, e.shiftKey]);
// Don't allow the click to trigger a link or anything
......@@ -709,9 +704,9 @@ cr.define('ntp', function() {
if (html) {
// It's important that we don't attach this node to the document
// because it might contain scripts.
var node = this.ownerDocument.createElement('div');
node.innerHTML = html;
title = node.textContent;
var doc = document.implementation.createHTMLDocument();
doc.body.innerHTML = html;
title = doc.body.textContent;
}
// Make sure title is >=1 and <=45 characters for Chrome app limits.
......
......@@ -67,6 +67,7 @@
#include "extensions/common/extension.h"
#include "extensions/common/extension_icon_set.h"
#include "extensions/common/extension_set.h"
#include "net/base/url_util.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/webui/web_ui_util.h"
#include "url/gurl.h"
......@@ -482,9 +483,7 @@ void AppLauncherHandler::HandleLaunchApp(const base::ListValue* args) {
CHECK(args->GetString(0, &extension_id));
double source = -1.0;
CHECK(args->GetDouble(1, &source));
std::string url;
if (args->GetSize() > 2)
CHECK(args->GetString(2, &url));
GURL override_url;
extension_misc::AppLaunchBucket launch_bucket =
static_cast<extension_misc::AppLaunchBucket>(
......@@ -511,6 +510,16 @@ void AppLauncherHandler::HandleLaunchApp(const base::ListValue* args) {
extensions::RecordAppLaunchType(launch_bucket, extension->GetType());
} else {
extensions::RecordWebStoreLaunch();
if (args->GetSize() > 2) {
std::string source_value;
CHECK(args->GetString(2, &source_value));
if (!source_value.empty()) {
override_url = net::AppendQueryParameter(
extensions::AppLaunchInfo::GetFullLaunchURL(extension),
extension_urls::kWebstoreSourceField, source_value);
}
}
}
if (disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB ||
......@@ -522,7 +531,7 @@ void AppLauncherHandler::HandleLaunchApp(const base::ListValue* args) {
? extensions::LAUNCH_CONTAINER_WINDOW
: extensions::LAUNCH_CONTAINER_TAB,
disposition, extensions::SOURCE_NEW_TAB_PAGE);
params.override_url = GURL(url);
params.override_url = override_url;
OpenApplication(params);
} else {
// To give a more "launchy" experience when using the NTP launcher, we close
......@@ -538,7 +547,7 @@ void AppLauncherHandler::HandleLaunchApp(const base::ListValue* args) {
old_contents ? WindowOpenDisposition::CURRENT_TAB
: WindowOpenDisposition::NEW_FOREGROUND_TAB,
extensions::SOURCE_NEW_TAB_PAGE);
params.override_url = GURL(url);
params.override_url = override_url;
WebContents* new_contents = OpenApplication(params);
// This will also destroy the handler, so do not perform any actions after.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment