Opened 6 years ago

Closed 6 years ago

#6380 closed defect (fixed)

Deprecation of json-commenting

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

Description (last modified by 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: http://getahead.org/blog/joe/2007/03/05/json_is_not_as_safe_as_people_think_it_is.html 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 6 years ago.
Patch to xhr.js to deprecate json-commenting

Download all attachments as: .zip

Change History (6)

Changed 6 years ago by kriszyp

Patch to xhr.js to deprecate json-commenting

comment:1 Changed 6 years ago by jburke

  • Owner changed from anonymous to jburke

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 6 years ago by peller

  • Description modified (diff)
  • Milestone changed from 1.1.1 to 1.2
  • severity changed from normal to major

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 6 years ago by jburke

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:4 Changed 6 years ago by gregwilkins

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 6 years ago by jburke

  • Resolution set to fixed
  • Status changed from reopened to closed

(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.