Opened 10 years ago

Closed 10 years ago

#6380 closed defect (fixed)

Deprecation of json-commenting

Reported by: kriszyp Owned by: James Burke
Priority: high Milestone: 1.2
Component: General Version: 1.1.0
Keywords: XHR JSON Cc:
Blocked By: Blocking:

Description (last modified by Adam Peller)

Dojo has used json-commenting to avert this form of hijacking. Unfortunately this approach is unsecure and produces more security concerns than it solves. If the returned data includes user inputted data, the data can easily include a comment closing tag and circumvent this security. For example:

/* {" */ grabData(',":"bar","')/*"} */

When used on JSON data that has a root of an object (as most JSON data does), commenting introduces a security hole when none existed before. In these situations plain JSON is much safer than commented JSON. Due to the fact the JSON data is usually rooted with an object, JSON data usually has user inputted data, and the availability of much better forms of security, I believe we should deprecate json commenting.

Another approach for combatting JSON hijacking is JSON prefixing. JSON prefixing involves simply prefixing all JSON data with "{}&&". This renders all JSON to syntactically invalid as a script and therefore can not be hijacked. In addition, this prefix does not affect the evaluation of JSON. Commented JSON requires that the client strip the comments before evaluation. Dojo does not need to take any extra measures to evaluate prefixed JSON. Prefixed JSON looks like:

{}&& ["foo","bar"]

Prefixed JSON prevents JSON array hijacking, does not introduce any security concerns, and does not require an alternate handler. Where commented JSON may have been used, use prefixed JSON instead.

Background: JSON Hijacking which has been discussed here: where the Array constructor can be overwritten by rogue sites and they can request resources from your site and access the results. It is only possible to hijack JSON data with a root that is an array. When the root is a primitive, primitive values do not trigger constructor (and the are no other values to worry about). When the root is an object, it is not valid JavaScript? syntax, and therefore can't be parsed. No amount of environmental alterations can affect an unparseable script.

Attachments (1)

xhr.diff (1.5 KB) - added by kriszyp 10 years ago.
Patch to xhr.js to deprecate json-commenting

Download all attachments as: .zip

Change History (6)

Changed 10 years ago by kriszyp

Attachment: xhr.diff added

Patch to xhr.js to deprecate json-commenting

comment:1 Changed 10 years ago by James Burke

Owner: changed from anonymous to James Burke

Although I would like to apply this patch for 1.1.1, I am not sure it fits in the guidelines for the 1.1.1 release, since we are removing and adding djConfig options. So I might just apply in the 1.2 release.

However, it would be good to hear from Alex that is fine with deprecating json-commented, since he originally pushed for this feature.

comment:2 Changed 10 years ago by Adam Peller

Description: modified (diff)
severity: normalmajor

I agree. Changing the rules in a dot release is probably not a good idea. 1.2 isn't that far off.

comment:3 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [13582]) Fixes #6380. Applying patch from Kris Zyp to deprecate json-comment-filtered. Updated 1.2 release notes too.

comment:4 Changed 10 years ago by gregwilkins

Resolution: fixed
Status: closedreopened

This fix makes the use of json-comment-optional very verbose, even in the case where the server does not send commented json.

This is also very confusing as the data is not commented, but the log shows warnings that commented json should not be used.

Note also the implementation of json-comment-optional is not optimal, as non commented json will also ways try json-comment-filtered and fail with an Exception. It would be better if he implementation explicitly checked for /* and decided which implementation to call.

comment:5 Changed 10 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

(In [13625]) Fixes #6380. Forgot to account for json-comment-optional. Thanks to gregwilkins for tracking down the issue.

Note: See TracTickets for help on using tickets.