1459640 - JSON viewer has no Content Security Policy if the source URL is not a http(s)

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Rob Wu [:robwu]]

As a part of bug 1457500, I am investigating the current use cases of content viewers, to make sure that the use cases are covered, and to avoid introducing security regressions.

During my audit, I found that the JSON viewer relies on a non-empty Content Security policy bug at https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/devtools/client/jsonview/converter-child.js#84-91 (the CSP was introduced in bug 1442840).

The CSP is conditional on the channel being a nsIHttpChannel. This means that if the page is served via non-http-channels such as file:, data: and blob:, that the JSON Viewer does not have any CSP.

Example (not working as intended):
STR.
1. Visit data:application/json,{}
2. Open the console and run: eval("")
3. Result: (no error)

Example of expected behavior:
1. Visit a JSON HTTP resource, e.g. https://api.github.com/
2. Open the console and run: eval("")
3. Result:
Error: call to eval() blocked by CSP
Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src”). Source: call to eval() or related function blocked by CSP.

Comment 1

[Rob Wu [:robwu]]

s/Content Security policy bug/Content Security policy/.

Comment 2

[:Gijs (he/him)]

I'm not aware of any security issues that would result from the page not having a CSP, and this report doesn't contain any, so I don't think this is critical.

That said, the patch is trivial so I will attach one.

Comment 3

[:Gijs (he/him)]

Attachment: Patch v1

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8973984 [details] [diff] [review]
Patch v1

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

Nice, thanks!

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #2)
> I'm not aware of any security issues that would result from the page not
> having a CSP, and this report doesn't contain any, so I don't think this is
> critical.

Am I OK to just land this with a clear commit message, and open up the bug as a sec-audit, or am I missing something and is this more serious than I'm claiming here?

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8973984 [details] [diff] [review]
Patch v1

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

::: devtools/client/jsonview/converter-child.js
@@ +85,5 @@
>  
>      // Enforce strict CSP:
>      try {
>        request.QueryInterface(Ci.nsIHttpChannel);
> +      request.setResponseHeader("Content-Security-Policy", kCSP, false);

I would also add

request.setResponseHeader("Content-Security-Policy-Report-Only", "", false);

to prevent websites from observing CSP violations in the internal JSON viewer.

Comment 7

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #5)
> (In reply to :Gijs (he/him) from comment #2)
> > I'm not aware of any security issues that would result from the page not
> > having a CSP, and this report doesn't contain any, so I don't think this is
> > critical.
> 
> Am I OK to just land this with a clear commit message, and open up the bug
> as a sec-audit, or am I missing something and is this more serious than I'm
> claiming here?

To exploit this bug, a separate XSS vulnerability in the JSON viewer is needed. Even if such an issue were to be found, it would merely result in code execution at a null principal, on non-http(s) schemes.

IMO the risks of publishing this issue is small, but please let dveditz@ confirm this before opening the bug.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to :Gijs (he/him) from comment #2)
> I'm not aware of any security issues that would result from the page not
> having a CSP, and this report doesn't contain any, so I don't think this is
> critical.

One of the security bugs we fixed in Firefox 60 was that the JSON Viewer would linkify javascript: urls contained in the content which would run when clicked. A CSP could have stopped that, except it wouldn't be too hard to have this content end up as a saved local file: or blob:. The scenarios I'm coming up with (assuming another similar bug) are admittedly unlikely, but they would work (in the functional, if not social, sense) which makes me worry there's some clever/terrible combination lurking. Let's just fix it.

(In reply to Rob Wu [:robwu] from comment #7)
> Even if such an issue were to be found, it would merely result in
> code execution at a null principal, on non-http(s) schemes.

That's more than enough to cause trouble. Imagine a JSON file with the metadata for your mail inbox, where one of the mails is an XSS sent you by an attacker. When run the XSS can now read the potentially-sensitive subject lines of all your mail and exfiltrate it. Similarly, The bugzilla API returns bug queries (in my case lists of sensitive security bugs) as JSON. Admittedly I don't look at them in JSON form, but maybe if I were debugging a script I would.

(In reply to :Gijs (he/him) from comment #5)
> Am I OK to just land this with a clear commit message, and open up the bug
> as a sec-audit, or am I missing something and is this more serious than I'm
> claiming here?

That seems fine -- an attack would require an XSS bug we don't at this moment know about.

Comment 9

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #6)
> Comment on attachment 8973984 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8973984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/jsonview/converter-child.js
> @@ +85,5 @@
> >  
> >      // Enforce strict CSP:
> >      try {
> >        request.QueryInterface(Ci.nsIHttpChannel);
> > +      request.setResponseHeader("Content-Security-Policy", kCSP, false);
> 
> I would also add
> 
> request.setResponseHeader("Content-Security-Policy-Report-Only", "", false);
> 
> to prevent websites from observing CSP violations in the internal JSON
> viewer.

Good point, thanks. Added that, landing on inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/093b3a6f608998cc9d1fb66d68445bf2e30e84df

Comment 10

[Oriol Brufau [:Oriol]]

Just wondering, is there any reason to allow scripts from any resource:// url instead of only resource://devtools-client-jsonview/ and resource://devtools-client-shared/ ?

Comment 11

[:Gijs (he/him)]

(In reply to Oriol Brufau [:Oriol] from comment #10)
> Just wondering, is there any reason to allow scripts from any resource://
> url instead of only resource://devtools-client-jsonview/ and
> resource://devtools-client-shared/ ?

My understanding is that it's not possible to restrict the resource: protocol in this way, maybe Christoph can clarify.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing devtools' devtools/client/jsonview/test/browser_jsonview_content_type.js, at least on OS X debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c34056cde92e21b6d72c4b0b6deb640e68701039

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=093b3a6f608998cc9d1fb66d68445bf2e30e84df&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178026720&repo=mozilla-inbound

04:00:16     INFO - Entering test bound 
04:00:16     INFO - Test JSON content types started
04:00:16     INFO - Adding a new JSON tab with URL: 'data:application/json,[1,2,3]'
04:00:16     INFO - Adding a new tab with URL: data:application/json,[1,2,3]
04:00:16     INFO - Buffered messages logged at 03:58:46
04:00:16     INFO - Tab added and finished loading
04:00:16     INFO - Loading the helper frame script chrome://mochitests/content/browser/devtools/client/shared/test/frame-script-utils.js
04:00:16     INFO - AppReadyState is "loading". Await "complete"
04:00:16     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_content_type.js | The JSON Viewer should only load for valid content types. - 
04:00:16     INFO - Sending message Test:JsonView:Eval to content
04:00:16     INFO - Expecting message Test:JsonView:Eval from content
04:00:16     INFO - Console message: [JavaScript Error: "Error: call to eval() blocked by CSP"]
04:00:16     INFO - Console message: [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src”). Source: call to eval() or related function blocked by CSP."]
04:00:16     INFO - Buffered messages finished
04:00:16     INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_content_type.js | Test timed out - 

Also failing:
04:02:10 INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_row_selection.js | Test timed out -
04:02:13 INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_save_json.js | Cleanup function threw an exception - [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIComponentRegistrar.unregisterFactory]" nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)" location: "JS frame :: chrome://specialpowers/content/MockFilePicker.jsm :: cleanup :: line 88" data: no]

Comment 13

[Oriol Brufau [:Oriol]]

(In reply to :Gijs (he/him) from comment #11)
> My understanding is that it's not possible to restrict the resource:
> protocol in this way, maybe Christoph can clarify.

At first glance, it seemed to work when I tried.

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #12)
> 04:00:16     INFO - Sending message Test:JsonView:Eval to content
> 04:00:16     INFO - Expecting message Test:JsonView:Eval from content
> 04:00:16     INFO - Console message: [JavaScript Error: "Error: call to
> eval() blocked by CSP"]

Sorry, before I knew about ContentTask.spawn, I used eval in some tests.
Do you want me to fix this in another bug?

And it might be good to add a test that eval will now be blocked.

Comment 14

[:Gijs (he/him)]

(In reply to Oriol Brufau [:Oriol] from comment #13)
> (In reply to :Gijs (he/him) from comment #11)
> > My understanding is that it's not possible to restrict the resource:
> > protocol in this way, maybe Christoph can clarify.
> 
> At first glance, it seemed to work when I tried.

I mean, in any case it's orthogonal to this bug, which is about the desired CSP not being applied at all.

> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #12)
> > 04:00:16     INFO - Sending message Test:JsonView:Eval to content
> > 04:00:16     INFO - Expecting message Test:JsonView:Eval from content
> > 04:00:16     INFO - Console message: [JavaScript Error: "Error: call to
> > eval() blocked by CSP"]
> 
> Sorry, before I knew about ContentTask.spawn, I used eval in some tests.
> Do you want me to fix this in another bug?

No, I think I'll just fix it in an update to the patch.

> And it might be good to add a test that eval will now be blocked.

I assume we have CSP tests generally, so I'm not sure what this would help with... checking that we don't change the CSP? I guess I'm not confident that people doing that wouldn't just change the test...

Comment 15

[:Gijs (he/him)]

Attachment: Bug 1459640 - add CSP also via meta tag,

This also fixes all the tests to not rely on eval() anymore.

Review commit: https://reviewboard.mozilla.org/r/243376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243376/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8974987 [details]
Bug 1459640 - add CSP also via meta tag,

https://reviewboard.mozilla.org/r/243376/#review249242

Thanks, those tests are easier to follow now

::: devtools/client/jsonview/test/head.js
(Diff revision 1)
>  
>  function normalizeNewLines(value) {
>    return value.replace("(\r\n|\n)", "\n");
>  }
> -
> -function evalInContent(code) {

Please also remove the listener for this message at https://searchfox.org/mozilla-central/source/devtools/client/jsonview/test/doc_frame_script.js#127, since this is the only consumer

Comment 17

[Oriol Brufau [:Oriol]]

I think you can also remove the definitions of $ and $$ just below that.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8974987 [details]
Bug 1459640 - add CSP also via meta tag,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243376/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09566acf52e7
add CSP also via meta tag, r=bgrins

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/09566acf52e7

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #11)
> (In reply to Oriol Brufau [:Oriol] from comment #10)
> > Just wondering, is there any reason to allow scripts from any resource://
> > url instead of only resource://devtools-client-jsonview/ and
> > resource://devtools-client-shared/ ?
> 
> My understanding is that it's not possible to restrict the resource:
> protocol in this way, maybe Christoph can clarify.

Gijs is right; obviously it would be possible to add a path restriction if really needed, but that's not how the setup is generally laid out.

Comment 22

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 62.0a1 (2018-05-08) on Windows 10, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID   : 20180605100102
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0