Subject: [apps-discuss] JSON-Patch and XSRF

From: Mark Nottingham <[email-masked]>
Date: Tue, 27 Nov 2012 13:29:29 +1100

One thing to note about the format we've chosen for JSON-Patch -- at the top level, it's an array, which has some security implications, according to <http://haacked.com/archive/2009/06/24/json-hijacking.aspx>.

Practically speaking, this means that people shouldn't make Patch files containing sensitive information available through GET on Web servers, even if they're protected by some form of authentication.

Is it sufficient to note this in Security Considerations, or do people want to change the format?

Regards,

--
Mark Nottingham   http://www.mnot.net/

---

From: Julian Reschke <[email-masked]>
Date: Tue, 27 Nov 2012 18:15:54 +0100

On 2012-11-27 03:29, Mark Nottingham wrote:
> ...

...like sticking the array into a container; or going back to the old 
format?

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 27 Nov 2012 13:33:50 -0800

IMO, it is sufficient to note this in the security considerations.

Paul

On Tue, 2012-11-27 at 13:29 +1100, Mark Nottingham wrote:

> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss

---

From: Mark Nottingham <[email-masked]>
Date: Wed, 28 Nov 2012 08:34:55 +1100

I tend to agree.

On 28/11/2012, at 8:33 AM, "Paul C. Bryan" <[email-masked]> wrote:

>> One thing to note about the format we've chosen for JSON-Patch -- at the top level, it's an array, which has some security implications, according to <http://haacked.com/archive/2009/06/24/json-hijacking.aspx
>> >.
>>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 27 Nov 2012 13:40:47 -0800

In the first draft[1], it was wrapped in a { "patches": [ ... ] }
object. This was considered needlessly verbose at the time, leading to
the stripping of the object to have the raw array we have today.

Paul 

[1] http://tools.ietf.org/html/draft-pbryan-json-patch-00

---

From: Nico Williams <[email-masked]>
Date: Tue, 27 Nov 2012 16:45:55 -0600

On Tue, Nov 27, 2012 at 3:40 PM, Paul C. Bryan <[email-masked]> wrote:
> In the first draft[1], it was wrapped in a { "patches": [ ... ] } object.
> This was considered needlessly verbose at the time, leading to the stripping
> of the object to have the raw array we have today.

Sounds like premature optimization.  HTTP is already fairly wasteful.
Now for this optimization we get a rather scary-looking security
consideration.  I'd rather fix the spec to not have this problem, but
I understand that it may well be too late.

Nico
--

---

From: Mark Nottingham <[email-masked]>
Date: Wed, 28 Nov 2012 09:50:57 +1100

I've added this to Security Considerations:

>             Because Web browsers can be coerced into loading an arbitrary
>             JSON document whose root is an array, JSON Patch documents
>             containing sensitive information SHOULD NOT be exposed on a Web
>             server (i.e., made available by HTTP GET), even if access is
>             authenticated.

I think this is adequate, as long as we agree that it'll be uncommon to need to expose JSON Patch files via HTTP GET. If there is a significant use case where this is necessary, we should change the format.

Regards,

---

From: James M Snell <[email-masked]>
Date: Tue, 27 Nov 2012 15:27:36 -0800

Hmm... Right off hand I can think of one use case in particular... albeit
one that is entirely hypothetical for me. If, for instance, someone wanted
to have an atompub-type collection of patch documents, or expose pending
patch documents via a queue of sorts with a RESTful interface. I'm not
building such apps so I'm not going to say either of these are critical but
they are interesting cases nonetheless.

- James

On Tue, Nov 27, 2012 at 2:50 PM, Mark Nottingham <[email-masked]> wrote:

> I think this is adequate, as long as we agree that it'll be uncommon to
> need to expose JSON Patch files via HTTP GET. If there is a significant use
> case where this is necessary, we should change the format.
>
> Regards,
>
>
>
> object.

---

From: "Manger, James H" <[email-masked]>
Date: Wed, 28 Nov 2012 11:08:54 +1100

A better place for this would by the Security Considerations section of (an updated) RFC 4627 JSON.

That "JSON hijacking" article is from 2.5 years ago <http://haacked.com/archive/2009/06/24/json-hijacking.aspx>. There is a decent chance it doesn't work in current browsers. It may also be fixed in EcmaScript5.

With a very quick try I couldn't get the attack to work in Firefox (which apparently explicitly fixed it) or Chrome or IE (which apparently never supported the features that the bug exploits).

I did notice, though, that all 3 browsers silently accepted a JSON array as JavaScript, while all 3 displayed an error message (on their debugging consoles) when trying to interpret a JSON object as JavaScript. That is almost sufficient to scare me off using a top-level JSON array, since an array value appears to make it into the browser execution environment and you have to hope it cannot be extracted.

I don't think the proposed Security Considerations text is a good approach. If to be safe with up-to-date browsers, a server really "SHOULD NOT allow (authenticated) GET requests to return a JSON array with non-public data" then I think we do (very regrettably) need to change the format -- and probably a bunch of other JSON formats as well. Not allowing servers to safely publish JSON-Patch values is too high a burden.

On the other hand, if this hijacking is no longer an issue with up-to-date browsers, then Security Considerations text noting that a JSON array is a valid (though pointless) JavaScript Program could be included. This is really a warning to JavaScript engine developers to be careful. This is my preferred result.

Can anyone with more JavaScript mojo confirm that <script src="array.json"> no longer exposes the data to other script on the page?

--
James Manger

> -----Original Message-----
> From: [email-masked] [mailto:apps-discuss-
> [email-masked]] On Behalf Of Mark Nottingham
> Sent: Wednesday, 28 November 2012 9:51 AM
> To: Nico Williams
> Cc: Julian Reschke; Apps Discuss
> Subject: Re: [apps-discuss] JSON-Patch and XSRF
> 
> need to expose JSON Patch files via HTTP GET. If there is a significant
> use case where this is necessary, we should change the format.
> 
> Regards,
> 
> 
> 
> >> This was considered needlessly verbose at the time, leading to the
> >> stripping of the object to have the raw array we have today.
> >

---

From: "Manger, James H" <[email-masked]>
Date: Wed, 28 Nov 2012 11:58:56 +1100

Curiously, a JSON object that forgets to quote a name can be a valid JavaScript program.
  { id:[2,3,5,7,11] }

This is not valid JSON (it should be { "id":[2,3,5,7,11] }), but I wouldn't be surprised if many JSON parsers were lenient in accepting names without quotes. When treated as a JavaScript program the unquoted name is parsed as a label and the result is as vulnerable as a raw JSON array to "hijacking". Changing formats from an array to an object might not buy quite as much safety as hoped.

--
James Manger

> [email-masked]] On Behalf Of Manger, James H
> Sent: Wednesday, 28 November 2012 11:09 AM
> To: Mark Nottingham; Nico Williams
…
> I did notice, though, that all 3 browsers silently accepted a JSON
> array as JavaScript, while all 3 displayed an error message (on their
> debugging consoles) when trying to interpret a JSON object as
> JavaScript. That is almost sufficient to scare me off using a top-level
> JSON array, since an array value appears to make it into the browser
> execution environment and you have to hope it cannot be extracted.
> 
> 
> I don't think the proposed Security Considerations text is a good
> approach. If to be safe with up-to-date browsers, a server really
> "SHOULD NOT allow (authenticated) GET requests to return a JSON array
> with non-public data" then I think we do (very regrettably) need to
> change the format -- and probably a bunch of other JSON formats as
> well. Not allowing servers to safely publish JSON-Patch values is too
> high a burden.
> 
> On the other hand, if this hijacking is no longer an issue with up-to-
> date browsers, then Security Considerations text noting that a JSON
> array is a valid (though pointless) JavaScript Program could be
> included. This is really a warning to JavaScript engine developers to
> be careful. This is my preferred result.
> 
> 
> Can anyone with more JavaScript mojo confirm that <script
> src="array.json"> no longer exposes the data to other script on the
> page?
> 
> --
> James Manger

---

From: "Paul C. Bryan" <[email-masked]>
Date: Tue, 27 Nov 2012 17:10:50 -0800

I'm reticent to address what is clearly a security concern with browser
implementations by modifying our specification with the proposed
object-instead-of-array workaround. The JSON Patch specification doesn't
address how the patch document is processed, save for mentioning that
it's intended at least to support HTTP PATCH, which is not susceptible
to the outlined CSRF attack. If this attack can be demonstrated with
modern browsers, I think it's probably worthy of mention under security
considerations.

Paul

---

From: Mark Nottingham <[email-masked]>
Date: Wed, 28 Nov 2012 17:58:50 +1100

> Can anyone with more JavaScript mojo confirm that <script src="array.json"> no longer exposes the data to other script on the page?

I tried a number of browsers, and got hits on:

Camino 2.0.6   
Omniweb 5.9
Flock 2.5.6 / 2.6.1
Opera 9.8  (version < 9.8 has .03% market share)
Chrome 7 / 8 / 9 / 10   (version <= 10 has .45% market share)
Safari 4.0.5  (version 4.0 has .1% market share)
Firefox 2 / 3  (version <= 3 has .25% market share)
Navigator 9
Seamonkey 1 

Couldn't find any version of IE that was vulnerable.

Market share numbers are from <http://gs.statcounter.com/#browser_version-ww-monthly-201210-201210-bar>; download the CSV. 

If we believe that, it means we're looking at about .85% of the *current* global browser market being vulnerable.

---

From: "Manger, James H" <[email-masked]>
Date: Thu, 29 Nov 2012 09:41:41 +1100

Thanks Mark,

That is an impressive list of browsers.
I'm now happy to stick with a JSON array.
It looks like JavaScript engines have addressed the issue, even if a few strangler are yet to deploy the fix.
A Security Considerations paragraph (if it is even necessary here) needn't tell servers they "SHOULD NOT" allows authenticated GETs of JSON-Patches.

--
James Manger

> From: Mark Nottingham [mailto:[email-masked]]
> Sent: Wednesday, 28 November 2012 5:59 PM
> To: Manger, James H
> Cc: Nico Williams; Julian Reschke; Apps Discuss
> > On the other hand, if this hijacking is no longer an issue with up-
> to-date browsers, then Security Considerations text noting that a JSON
> Safari 4.0.5  (version 4.0 has .1% market share) Firefox 2 / 3
> (version <= 3 has .25% market share) Navigator 9 Seamonkey 1
> 
> Market share numbers are from
> <http://gs.statcounter.com/#browser_version-ww-monthly-201210-201210-
> bar>; download the CSV.
> 
> If we believe that, it means we're looking at about .85% of the
> *current* global browser market being vulnerable.
> 
> 
> --

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 29 Nov 2012 09:57:25 +1100

Anybody else have a strong opinion?

---

From: "Paul C. Bryan" <[email-masked]>
Date: Wed, 28 Nov 2012 19:17:51 -0800

+1. Given the data presented, security considerations doesn't even seem
necessary.

Paul

---

From: Nico Williams <[email-masked]>
Date: Thu, 29 Nov 2012 04:07:55 -0600

necessary.

Agreed. It's an Emily Litella moment!

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 30 Nov 2012 09:39:39 +1100

OK, I've softened the SC language to:

>             A few older Web browsers can be coerced into loading an
>             arbitrary JSON document whose root is an array, leading to a
>             situation where a JSON Patch document containing sensitive
>             information could be exposed to attackers, even if access is
>             authenticated. However, such browsers are not widely used (
>             estimated to comprise less than 1% of the market, at the time
>             of writing). Publishers who are nevertheless concerned about
>             this attack are advised to avoid making such documents available
>             with HTTP GET.

Personally, I think it's still worth having it in (WITHOUT a requirement); does anyone disagree?

Cheers,

On 29/11/2012, at 9:07 PM, Nico Williams <[email-masked]> wrote:

> > +1. Given the data presented, security considerations doesn't even seem necessary.
>

---

From: "Paul C. Bryan" <[email-masked]>
Date: Thu, 29 Nov 2012 14:46:29 -0800

If we are to include this in security considerations, then I think it's
best to outline the nature of the attack, i.e. being explicit that it's
vulnerable to a [C|X]SRF attack.

Paul

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 30 Nov 2012 09:50:22 +1100

I'm a bit concerned that if we did that, we'd have to either define what an XSRF attack is, or find a suitable reference...

On 30/11/2012, at 9:46 AM, "Paul C. Bryan" <[email-masked]> wrote:

> If we are to include this in security considerations, then I think it's best to outline the nature of the attack, i.e. being explicit that it's vulnerable to a [C|X]SRF attack.
> 
> Paul 
> 
>> On 29/11/2012, at 9:07 PM, Nico Williams <
>> [email-masked]
>> > wrote:
>> 
>> > 
>> > On Nov 28, 2012 9:17 PM, "Paul C. Bryan" <
>> [email-masked]
>> > wrote:
>> > >

---

From: Martin Thomson <[email-masked]>
Date: Fri, 30 Nov 2012 13:02:50 -0800

On 29 November 2012 14:50, Mark Nottingham <[email-masked]> wrote:
> find a suitable reference...

Adam Barth has a pretty comprehensive paper on the various methods.  I
don't know where it was published other than here:
http://www.adambarth.com/papers/2008/barth-jackson-mitchell-b.pdf