Closed Bug 1377668 Opened 7 years ago Closed 7 years ago

JSON object with "type": "string" collapses to “Invalid object”

Categories

(DevTools :: JSON Viewer, defect, P3)

Product:
DevTools
Component:
JSON Viewer
Version:
55 Branch
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: lucas.werkmeister, Assigned: Honza)

References

Details

Attachments

(2 files)

Bug 1377668 - Avoid invalid object by using noGrip option;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Screenshot (Web Console vs JSON Viewer) as mentioned in comment #24
70.96 KB, image/png
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170629075044

Steps to reproduce:

Open a JSON file with the following content (e. g. locally, or https://lucaswerkmeister.de/x-type-string.json):

    { "x": { "type": "string" } }

Collapse the “x” member.

Alternatively, open https://www.wikidata.org/wiki/Special:EntityData/Q42.json and expand .entities.Q42.claims.P1417[0].mainsnak, and observe the collapsed datavalue member.


Actual results:

The collapsed value is summarized as “Invalid object”; the title attribute instructs the user to file a bug on bugzilla.mozilla.org.

In the Browser Console, the error message “<unavailable>” (without quotes, with angle brackets) is logged, with the location information reps.js:496 (pointing to resource://devtools/client/shared/components/reps/reps.js).


Expected results:

The collapsed value should be summarized as “Object”.

Similarly, a value with { "type": "undefined" } collapses to “undefined” (without quotes), and a value with { "type": "number" } collapses to “[Object object]” (without quotes, with brackets).

It looks like the JSON file can trick the JSON viewer into using a different renderer for the value, which sounds like it *might* be exploitable, so I’m marking this as “Security” (but feel free to make the bug public if you disagree).
Honza, one for you?

(In reply to Lucas Werkmeister from comment #0)
> It looks like the JSON file can trick the JSON viewer into using a different
> renderer for the value

(FWIW, I am not sure if this is actually true, Honza would know better)

> , which sounds like it *might* be exploitable, so I’m
> marking this as “Security” (but feel free to make the bug public if you
> disagree).

This would indeed be a serious problem if the document could communicate with anything else (ie you could run code outside of the context where it's meant to run, unlike running JS in normal webpages), but the JSON viewer document should be isolated (ie doesn't have any specific privileges at all besides communicating with the parent process for the 'copy' and 'save' functionality, which you could do in a normal webpage, too). You can verify this by running JS in the JSON viewer document with e.g. the web console (without even trying to exploit the former). Based on this, I think this can be unhidden, but I'll let Honza make the call here.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: JSON Viewer
Ever confirmed: true
Flags: needinfo?(odvarko)
Thanks for the report!

The problem is related to `Reps` (DevTools rendering engine) and so, I reported an issue on github.
https://github.com/devtools-html/devtools-core/issues/480

This is not security issue and the flag can be removed.

@Gijs: Can you please unhide this report?

Thanks!

Honza
Flags: needinfo?(odvarko) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
Group: firefox-core-security
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Depends on https://github.com/devtools-html/devtools-core/pull/483
(need to wait till the new Reps version lands in m-c)

Honza
Depends on: 1376534
Comment on attachment 8883927 [details]
Bug 1377668 - Avoid invalid object by using noGrip option;

https://reviewboard.mozilla.org/r/154906/#review159882

This is a small change and I don't see what could be wrong here :)
I know that the reps bundle didn't landed yet into m-c, but would it be possible to add a test to make sure the test case won't be a problem anymore ?
For now the test would fail, and when we land the reps bundle, after rebasing the test would be green.

This is not mandatory since there are tests on the Reps side, but it would prevent us some regressions in case someone remove/change the prop in Reps.
What do you think about it Honza ?
Attachment #8883927 - Flags: review?(nchevobbe) → review+
Good point, done!

Honza
Comment on attachment 8883927 [details]
Bug 1377668 - Avoid invalid object by using noGrip option;

https://reviewboard.mozilla.org/r/154906/#review159920

::: devtools/client/jsonview/test/browser_jsonview_object-type.js:17
(Diff revision 2)
> +  yield addJsonViewTab(TEST_JSON_URL);
> +
> +  let count = yield getElementCount(".jsonPanelBox .treeTable .treeRow");
> +  is(count, 2, "There must be two rows");
> +
> +  // Collapsed auto-expanded node.

nit: s/collapsed/collapse
Does this fix bug 1373102 too?
(In reply to Oriol [:Oriol] from comment #11)
> Does this fix bug 1373102 too?

Yes, it should
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> (In reply to Oriol [:Oriol] from comment #11)
> > Does this fix bug 1373102 too?
> 
> Yes, it should

Yes, it'll, I've tested this.

Honza
Depends on: 1380790
No longer depends on: 1380790
Why not land this?
Ah, forgotten patch.

There is another Reps PR needed for this issue.
https://github.com/devtools-html/devtools-core/pull/663

(I am keeping the NI flag open for now)

Honza
Depends on: 1399460
Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko)
status-firefox57: --- → fix-optional
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52fa1b99d27c
Avoid invalid object by using noGrip option; r=nchevobbe
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff7d6888f249
Avoid invalid object by using noGrip option; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/ff7d6888f249
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Seeing a similar behaviour on the Web Console (Network Tab); the object does render (so it's not a major cause for alarm) but the tree displays an "Invalid Object" tag which, when hovered upon, shows a tooltip saying "This object could not be rendered, please file a bug on bugzilla.mozilla.org" (which is what led me here :) )

I have captured a screenshot using a public Google API Discovery Document URL (https://content.googleapis.com/discovery/v1/apis/storage/v1/rest?fields=kind%2Cname%2Cversion%2CrootUrl%2CservicePath%2Cresources%2Cparameters%2Cmethods%2CbatchPath%2Cid&pp=0), opened directly in a browser tab. Note that both JSON viewers (in-tab and Web Console) display the JSON tree correctly, but the in-tab one does not display the "Invalid Object" tag (e.g. for the `/parameters/alt` node).

(Cannot seem to find a way to attach the screenshot right here; will try to attach it as the next comment)

I'm on Nightly 61.0a1 (slightly old, "61.0a1 (2018-04-13) (64-bit)"; I can try upgrading within the next few days, but I'm fairly sure that won't make a difference :) )
Screenshot (Web Console vs JSON Viewer) as mentioned in comment #24
See Also: → 1457701
Yes, as said in https://github.com/devtools-html/devtools-core/pull/483#issuecomment-314118338, the netmonitor needs noGrip:true too, filed bug 1457701. And in fact reps should fail fast and loud if this parameter is not provided instead of seemingly work in most cases but absolutely fail in edge ones.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: