[DevTools] Whitelist remoteFrontendUrl and remoteBase params. (issue 2065823004 by dgozman@chromium.org)

[dgo...@chromium.org]

Reviewers: caseq
CL: https://codereview.chromium.org/2065823004/

Message:
Could you please take a look?

Description:
[DevTools] Whitelist remoteFrontendUrl and remoteBase params.

This also fixes loadScriptsPromise to not normalize hostname.

BUG=619414,618333

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+13, -3 lines):
M third_party/WebKit/Source/devtools/front_end/Runtime.js
M third_party/WebKit/Source/devtools/front_end/devtools.js


Index: third_party/WebKit/Source/devtools/front_end/Runtime.js
diff --git a/third_party/WebKit/Source/devtools/front_end/Runtime.js b/third_party/WebKit/Source/devtools/front_end/Runtime.js
index 37fb273fa25386f4eaeaabb79f00cc562855cf84..2f0b1ee9713e90beb692924aa62af7a8b27d0b35 100644
--- a/third_party/WebKit/Source/devtools/front_end/Runtime.js
+++ b/third_party/WebKit/Source/devtools/front_end/Runtime.js
@@ -121,8 +121,13 @@ function loadScriptsPromise(scriptNames, base)
for (var i = 0; i < scriptNames.length; ++i) {
var scriptName = scriptNames[i];
var sourceURL = (base || self._importScriptPathPrefix) + scriptName;
+
var schemaIndex = sourceURL.indexOf("://") + 3;
- sourceURL = sourceURL.substring(0, schemaIndex) + normalizePath(sourceURL.substring(schemaIndex));
+ var pathIndex = sourceURL.indexOf("/", schemaIndex);
+ if (pathIndex === -1)
+ pathIndex = sourceURL.length;
+ sourceURL = sourceURL.substring(0, pathIndex) + normalizePath(sourceURL.substring(pathIndex));
+
if (_loadedScripts[sourceURL])
continue;
urls.push(sourceURL);
@@ -1160,7 +1165,8 @@ Runtime.experiments = new Runtime.ExperimentsSupport();
Runtime._remoteBase = Runtime.queryParam("remoteBase");
{(function validateRemoteBase()
{
- if (Runtime._remoteBase && !Runtime._remoteBase.startsWith("https://chrome-devtools-frontend.appspot.com/"))
+ var remoteBaseRegexp = /^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;
+ if (Runtime._remoteBase && !remoteBaseRegexp.test(Runtime._remoteBase))
Runtime._remoteBase = null;
})();}

Index: third_party/WebKit/Source/devtools/front_end/devtools.js
diff --git a/third_party/WebKit/Source/devtools/front_end/devtools.js b/third_party/WebKit/Source/devtools/front_end/devtools.js
index f3a91417f68c5ed57a954f58d7eafead6c55e625..1b278045a32cb793cbcf07b635e0bcfe57c602aa 100644
--- a/third_party/WebKit/Source/devtools/front_end/devtools.js
+++ b/third_party/WebKit/Source/devtools/front_end/devtools.js
@@ -985,6 +985,8 @@ function installObjectObserve()
*/
function sanitizeRemoteFrontendUrl()
{
+ var remoteBaseRegexp = /^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;
+ var remoteFrontendUrlRegexp = /^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_rev\/@?[0-9a-zA-Z]+\/(devtools|inspector)\.html$/;
var queryParams = location.search;
if (!queryParams)
return;
@@ -993,7 +995,9 @@ function sanitizeRemoteFrontendUrl()
var pair = params[i].split("=");
var name = pair.shift();
var value = pair.join("=");
- if (name === "remoteFrontendUrl" && !value.startsWith("https://chrome-devtools-frontend.appspot.com/"))
+ if (name === "remoteFrontendUrl" && !remoteFrontendUrlRegexp.test(value))
+ location.search = "";
+ if (name === "remoteBase" && !remoteBaseRegexp.test(value))
location.search = "";
}
}

[ca...@chromium.org]

lgtm




https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/devtools/front_end/devtools.js
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/devtools/front_end/devtools.js#newcode988
third_party/WebKit/Source/devtools/front_end/devtools.js:988: var

remoteBaseRegexp =
/^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;

nit: consider extracting a constant. also, perhaps just use [\w] for
brevity?

https://codereview.chromium.org/2065823004/

[dgo...@chromium.org]

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/devtools/front_end/devtools.js
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/devtools/front_end/devtools.js#newcode988
third_party/WebKit/Source/devtools/front_end/devtools.js:988: var
remoteBaseRegexp =
/^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;

On 2016/06/16 12:51:16, caseq wrote:
> nit: consider extracting a constant. also, perhaps just use [\w] for
brevity?

These two files cannot share any code.

https://codereview.chromium.org/2065823004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/2065823004/1


https://codereview.chromium.org/2065823004/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #1 (id:1)

https://codereview.chromium.org/2065823004/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 1 (id:??) landed as
https://crrev.com/554517a4587bfb0071bcd3c7eff6645a0b06d72a
Cr-Commit-Position: refs/heads/master@{#400768}

https://codereview.chromium.org/2065823004/

[rchlo...@opera.com]

I wish that the URL would be constructed from constants defined in
chrome/browser/ui/webui/devtools_ui.cc and exposed to JS instead of hardcoding
them in two separate places.

We in Opera have to apply more patches if things are hardcoded like that.
If Google is potentially interested then I could improve that (although I'm not
very familiar with this code).


https://codereview.chromium.org/2065823004/

[dgo...@chromium.org]

On 2016/06/30 14:36:39, Rafał Chłodnicki wrote:
> I wish that the URL would be constructed from constants defined in
> chrome/browser/ui/webui/devtools_ui.cc and exposed to JS instead of hardcoding
> them in two separate places.
>
> We in Opera have to apply more patches if things are hardcoded like that.
> If Google is potentially interested then I could improve that (although I'm
not
> very familiar with this code).

I see the problem. The two locations serve different purposes though:
devtools_ui constructs it, and devtools.js validates it for security reasons.
I think it's actually possible to do the validation in devtools_ui, and just
prevent navigation or not create DevToolsUIBindings when the url is suspicious.
It would be a bit harder to implement, but we can go for it. Are you interested
to contribute?

https://codereview.chromium.org/2065823004/

[rchlo...@opera.com]

I wouldn't mind but I'm a bit overloaded right now. My managers have other
assignments for me.
And also you seem to have much better idea of how to fix that. :)
And I don't have access to related security bugs (although our security group
does and explained to me what are those about) so it's hard for me to
communicate about this issue.


(Also I can't CC myself on this review as it's closed so I'm not getting
notifications.)

https://codereview.chromium.org/2065823004/

[dgo...@chromium.org]

+lushnikov, who was interested in this area.

https://codereview.chromium.org/2065823004/