Closed Bug 1394681 Opened 7 years ago Closed 7 years ago

network monitor replaced with web content

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: glob, Assigned: Honza)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

Attached image network tab replaced
nightly 57.0a1 (2017-08-28) (64-bit), osx

if you perform the follow steps, the network monitor dev tools content is replaced with a url of your choosing:

1. new tab, open dev tools, switch to network tab
2. navigate to a url that returns json containing a url
   eg https://drop.glob.com.au/landfill/sample.json
   which is just {"url":"https://glob.com.au/"}
3. show request details -> response tab
4. click on the url under the JSON section

expected result

- url field changes to <input> or similar, so you can copy the url

actual result

- entire network monitor tab changed to the url
- refreshing the page doesn't restore the original network tab content
Im not sure how serious this is, but a couple of observations from testing:
 - the loaded page opens in the Chrome process (this would be a way to bypass sandboxing if it was trigger-able from a web content process
 - For the loaded web page, window.top is chrome://browser/content/browser.xul (not dangerous in itself, afaik, but not normal either)

Neither of those things are great, and its likely you could interfere with devtools from web content. (if you could coerce a user to follow the STR)
Is the content itself regular web content, or does it inherit chrome privileges from its parent? Does it have some kind of intermediate dev tools privileged access?

Rating sec-high just because this is a sandbox escape at least.
Honza, can you take a look?
Flags: needinfo?(odvarko)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Comment on attachment 8904517 [details] [diff] [review]
bug1394681.patch

Review of attachment 8904517 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Honza!

I've verified that the current patch has been fixed the security issue. Follow the STR from description will open the url link in a new tab.

I just see one usability broken after clicking on the url link in response panel, then the url value will be changed into editable state, but it is unable to be changed back (to be an url link again).

Is this design intentional?

::: devtools/client/netmonitor/index.html
@@ +56,3 @@
>            const App = createFactory(require("./src/components/app"));
>            const sourceMapService = toolbox.sourceMapURLService;
> +          const app = App({sourceMapService, openLink });

nit: add whitespace before sourceMapService

::: devtools/client/netmonitor/src/components/app.js
@@ +20,5 @@
>  /*
>   * App component
>   * The top level component for representing main panel
>   */
> +function App({ statisticsOpen, sourceMapService, openLink }) {

nit: format component properties into multiple line with alphabetical order.

function App({
  openLink,
  statisticsOpen,
  sourceMapService,
}) {

@@ +25,3 @@
>    return (
>      div({ className: "network-monitor" },
> +      !statisticsOpen ? MonitorPanel({sourceMapService, openLink}) : StatisticsPanel()

nit: add whitespace between properties and define them in alphabetical order in possible.

{ openLink, sourceMapService }

@@ +34,5 @@
>  App.propTypes = {
>    statisticsOpen: PropTypes.bool.isRequired,
>    // Service to enable the source map feature.
>    sourceMapService: PropTypes.object,
> +  openLink: PropTypes.func,

nit: define propTypes in alphabetical order if possible.

::: devtools/client/netmonitor/src/components/params-panel.js
@@ +33,5 @@
>  /*
>   * Params panel component
>   * Displays the GET parameters and POST data of a request
>   */
> +function ParamsPanel({ request, openLink }) {

nit: Please convert these properties into multiline.

::: devtools/client/netmonitor/src/components/security-panel.js
@@ +23,5 @@
>   * If the site is being served over HTTPS, you get an extra tab labeled "Security".
>   * This contains details about the secure connection used including the protocol,
>   * the cipher suite, and certificate details
>   */
> +function SecurityPanel({ request, openLink }) {

nit: Please convert these properties into multiline.

::: devtools/client/netmonitor/src/components/stack-trace-panel.js
@@ +15,5 @@
>  
>  // Components
>  const StackTrace = createFactory(require("devtools/client/shared/components/stack-trace"));
>  
> +function StackTracePanel({ request, sourceMapService, openLink }) {

nit: Please convert these properties into multiline.
Attachment #8904517 - Flags: review?(rchien)
(In reply to Ricky Chien [:rickychien] from comment #5)
> I just see one usability broken after clicking on the url link in response
> panel, then the url value will be changed into editable state, but it is
> unable to be changed back (to be an url link again).
> 
> Is this design intentional?
Nice catch!

This isn't caused by this patch, the problem was there before.
I filled bug 1397228 for it.

All nits fixed.

Thanks for the review!

Honza
Attachment #8904517 - Flags: review?(rchien)
Attachment #8904517 - Flags: review?(rchien)
Attached patch bug1394681.patchSplinter Review
Attachment #8904517 - Attachment is obsolete: true
Attachment #8904967 - Flags: review?(rchien)
Comment on attachment 8904967 [details] [diff] [review]
bug1394681.patch

Review of attachment 8904967 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. thanks!
Attachment #8904967 - Flags: review?(rchien) → review+
https://hg.mozilla.org/mozilla-central/rev/13af2a4eee01
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Group: firefox-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.