New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2010
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression

Restricted
  • Only users with Commit permission may comment.



Sign in to add a comment
link

Issue 38105: application/json rendered as HTML (XSS risk)

Reported by bendavi...@gmail.com, Mar 13 2010

Issue description

Chrome Version       : 5.0.351.0 (Developer Build 41249) Ubuntu
Other browsers tested:
  Firefox 3.x: OK

What steps will reproduce the problem?
1. Load a url that returns content with mimetype application/json

What is the expected result?
JSON text with newlines and whitespace, identical to plain-text.

What happens instead?
Newlines and whitespace are removed.


The reason for this is mostly for debugging purposes.  That is, when you 
want to see the JSON response from a url,  and that response is formatted 
(ie, with indents and newlines).

Please provide any additional information below. Attach a screenshot if
possible.
 

Comment 1 by abarth@chromium.org, Apr 8 2010

How does Safari behave?  I bet the best way to fix this is in WebKit where we pick
the document type based on the MIME type.  We want a text document.  If we're picking
Document or HTMLDocument, that's bad news bears.

Comment 2 by abarth@chromium.org, Apr 8 2010

http://webblaze.org/abarth/tests/json/ttt.php

Firefox: Download
Safari: Download
Chrome: Render as HTML?  XML?  Not sure yet.  Definitely not download or text.

Comment 3 by abarth@chromium.org, Apr 8 2010

Labels: -Pri-2 Pri-1 Security SecSeverity-Medium
Status: Assigned
That's what I was afraid of.  This is bad news bears:

http://webblaze.org/abarth/tests/json/ggg.php

We shouldn't be executing that script!

Comment 4 by abarth@chromium.org, Apr 8 2010

Summary: application/json rendered as HTML (XSS risk)

Comment 5 by lcamtuf@google.com, Apr 8 2010

Eeep!

Comment 6 by finnur@chromium.org, Apr 8 2010

Labels: Regression
This is a regression from Stable. We used to opt to download the document.

Comment 7 by aarya@google.com, Apr 8 2010

shouldn't we be downloading the document. that is what IE, Firefox, Opera and safari 
nightly are doing ??

Comment 8 by bendavi...@gmail.com, Apr 9 2010

Why should it be downloading?   It should display in the browser just as plain-text 
does.  Either that or the extensions API needs to be opened up a bit so that extensions 
can read the content-type of a document and switch it as preferences dictate.

Comment 9 by infe...@chromium.org, Apr 9 2010

adam, can i take this one up. it will help me to understand webkit in more detail. i
will see why we are rendering as html and change it to text/plain.

Comment 10 by abarth@chromium.org, Apr 9 2010

Ok.  Either text/plain or download is fine.  It's unclear which is the best.  I
suspect the code will lead you to towards the better solution.

Comment 11 by scarybea...@gmail.com, Apr 9 2010

I prefer the former, personally.
Our MIME type handing + content sniffing is very conservative. I wonder what is going 
wrong here? We didn't add a JSON pretty-printer did we?

Comment 12 by scarybea...@gmail.com, Apr 9 2010

Regression is probably http://src.chromium.org/viewvc/chrome?view=rev&revision=32375
Not downloading it shouldn't have caused us to find HTML in it, though :-/

Comment 13 by scarybea...@gmail.com, Apr 9 2010

Looks like we want to add application/json to DOMImplementation::isTextMIMEType() in 
WebKit. 1-liner? :D

Comment 14 by lcam...@gmail.com, Apr 9 2010

So wouldn't that result in the same problem repeating itself with the next change?

Wouldn't a better fix be to move it to supported_javascript_types, and rename 
supported_non_image_types to something more descriptive (supported_active_content) 
with a warning in the comments nearby?

Comment 15 by scarybea...@gmail.com, Apr 9 2010

Moving to supported javascript types would imply that application/json is a reasonable 
type to specify in <script type="blah">. Maybe it is. Do JSON and JSONP share the same 
MIME type?

Comment 16 by scarybea...@gmail.com, Apr 9 2010

Longer term, I'd prefer if WebKit ditched the list of text MIME types. Instead it 
should have a list of active types and default to text rendering. As opposed to visa 
versa.

Comment 17 by infe...@chromium.org, Apr 9 2010

i do see text/css and text/ in the supported_non_image_types, but for those does not
look to be rendered as html. having application/json in that list might not be a
problem, i am still trying to find why it is getting sniffed as html. moving to
supported_javascript_types works to resolve the problem (and also chris's solution),
but just looking into the code more to understand what is going on.

Comment 18 by abarth@chromium.org, Apr 9 2010

The problem is the code in WebKit defaults to Document when it doesn't know the MIME
type.  We either need to fix that or add tests / DCHECKs to our code so that people
know they have to worry about this issue when changing that array.

Comment 19 by infe...@chromium.org, Apr 9 2010

Status: Started
Basically as chris said in comment #13, the fix is a one-liner in the istextmimetype
function. it can added as another clause, but i am thinking to go down Michal's way
of adding "application/json" to both mime_util.cc (for chrome) and
MIMETypeRegistry.cpp (for safari) since json is another javascript mime type. i will
add the layout test in webkit. i will also add comments near non image type
definitions for both chrome and safari to prevent people from blinding adding any new
type and causing xss. application/json is the official format and i didn't find any
other mime types like text/json, etc valid. even if someone uses like text/json, it
won't appear in the supportednonimagetypes and will get downloaded as expected.

Does everyone agree to this ? If yes, i will go ahead with the quick fix.

bool DOMImplementation::isTextMIMEType(const String& mimeType)
{
    if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType) ||
        (mimeType.startsWith("text/") && mimeType != "text/html" &&
         mimeType != "text/xml" && mimeType != "text/xsl"))
        return true;

    return false;
}

Comment 20 by infe...@chromium.org, Apr 9 2010

had a chat with adam. json is not javascript, so should not add it to javascript
types. so, will fix it chris' way by adding in the same if condition. working on
patch with layout test.

Comment 21 by lcam...@gmail.com, Apr 9 2010

My concern is that if you special-case it in DOMImplementation::isTextMIMEType(), 
things will go wrong. What's the purpose of isSupportedJavaScriptMIMEType(), other 
than making this distinction, and where would the semantic nit-picking matter?

Comment 22 by lcam...@gmail.com, Apr 9 2010

More specifically, it feels to me like a much better fix to have two descriptively 
named arrays / functions, something like shouldBeDisplayedAsText() and 
shouldBeHandledAsActiveContent(), and then only invoke them from 
DOMImplementation::isTextMIMEType(), etc. Tweaking the current design by adding a 
special case looks rather weird and error-prone, even if it fixes it in the short run. 
Plus, other contents of supported_non_image_types seem puzzling (why is "text/" there? 
the "text-but-not-html" logic is already duplicated in 
DOMImplementation::isTextMIMEType).

Comment 23 by infe...@chromium.org, Apr 9 2010

Hi Michal,

i don't think adding json will be a good thing in isSupportedJavaScriptMIMEType since
ScriptElementData::shouldExecuteAsJavaScript() in dom›ScriptElement.cpp uses it for
executing scripts.

Comment 24 by lcam...@gmail.com, Apr 9 2010

Labels: Restrict-View-SecurityTeam
Ah, good point. But in any case, the need to duplicate MIME type across two files, 
with the silent failure mode being undesired rendering as HTML, does not feel right ;p

Comment 25 by infe...@chromium.org, Apr 9 2010

Hi Michal, it will be in the same file WebKit\WebCore\dom\DOMImplementation.cpp
shared by both chrome and webkit

Comment 27 by bugdro...@gmail.com, Apr 9 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=44143 

------------------------------------------------------------------------
r44143 | inferno@chromium.org | 2010-04-09 15:27:39 -0700 (Fri, 09 Apr 2010) | 6 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/base/mime_util.cc?r1=44143&r2=44142

Prevent people from adding mime type to supported_non_image_types that will make it render as HTML. This is not the fix for bug, but to prevent people from making the same mistake again.

BUG= 38105 
TEST=None

Review URL: http://codereview.chromium.org/1617013
------------------------------------------------------------------------

Comment 28 by scarybea...@gmail.com, Apr 10 2010

Status: FixUnreleased
@bendavis78: thanks for preventing this regression from ever hitting a stable release.
Would you like us to credit you in out v5 release notes and if so, what name (and 
optional affiliation) should be use?

Comment 29 by infe...@chromium.org, Apr 10 2010

Bug fixed in webkit revision r57386: <http://trac.webkit.org/changeset/57386>

Comment 30 by bendavi...@gmail.com, Apr 13 2010

@scarybeasts:  Well, I hate to take credit as I didn't discover the actual XSS nature 
of this bug (my motives for fixing were just to make JSON debugging easier!).  But you 
really want to,  you can put me in as just "Ben Davis".  :-)

Comment 31 by abarth@chromium.org, Apr 15 2010

Comment 32 by scarybea...@gmail.com, Jun 9 2010

Labels: -Restrict-View-SecurityTeam
Status: Fixed

Comment 33 by cevans@google.com, Jun 15 2010

We should merge this to v5. Although IE also interprets application/json as HTML if it can, there's no need for us to follow the same path.

Comment 34 by cevans@google.com, Jun 15 2010

(v5 branched WebKit at 57286)

Comment 35 by infe...@chromium.org, Jun 15 2010

Status: WillMerge

Comment 36 by infe...@chromium.org, Jun 15 2010

Labels: -Area-Undefined Area-WebKit Restrict-View-SecurityTeam

Comment 37 by bugdro...@gmail.com, Jun 15 2010

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=49840 

------------------------------------------------------------------------
r49840 | inferno@chromium.org | 2010-06-15 15:06:24 -0700 (Tue, 15 Jun 2010) | 26 lines
Changed paths:
   A http://src.chromium.org/viewvc/chrome/branches/WebKit/375/LayoutTests/http/tests/security/resources/send-mime-types.php
   A http://src.chromium.org/viewvc/chrome/branches/WebKit/375/LayoutTests/http/tests/security/xss-DENIED-mime-type-execute-as-html-expected.txt
   A http://src.chromium.org/viewvc/chrome/branches/WebKit/375/LayoutTests/http/tests/security/xss-DENIED-mime-type-execute-as-html.html
   M http://src.chromium.org/viewvc/chrome/branches/WebKit/375/WebCore/dom/DOMImplementation.cpp?r1=49840&r2=49839
   M http://src.chromium.org/viewvc/chrome/branches/WebKit/375/WebCore/platform/MIMETypeRegistry.cpp?r1=49840&r2=49839

Merge 57386 - 2010-04-09  Abhishek Arya  <inferno@chromium.org>

        Reviewed by Adam Barth.

        Prevent HTTP responses served with JSON content type from being rendered as HTML. 

        Test: http/tests/security/xss-DENIED-mime-type-execute-as-html.html

        * dom/DOMImplementation.cpp:
        (WebCore::DOMImplementation::isTextMIMEType): Render application/json as text/plain.
        * platform/MIMETypeRegistry.cpp:
        (WebCore::initializeSupportedNonImageMimeTypes): Add a compile assert to prevent addition of new mime types in non-image types. 
2010-04-09  Abhishek Arya  <inferno@chromium.org>

        Reviewed by Adam Barth.

        Test non-image and javascript mime types are not rendered as HTML.

        * http/tests/security/resources/send-mime-types.php: Added.
        * http/tests/security/xss-DENIED-mime-type-execute-as-html-expected.txt: Added.
        * http/tests/security/xss-DENIED-mime-type-execute-as-html.html: Added.

BUG= 38105 
TEST=NONE
TBR=abarth@webkit.org
Review URL: http://codereview.chromium.org/2845006
------------------------------------------------------------------------

Comment 38 by infe...@chromium.org, Jun 15 2010

Status: FixUnreleased

Comment 39 by jsc...@chromium.org, Jun 17 2010

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Bulk edit for SecurityNotify Migration.

Comment 40 by scarybea...@gmail.com, Jun 19 2010

Adding Emanuele Gentili, to whom we are grateful for noticing the regression on the stable channel.

Comment 41 by jsc...@chromium.org, Mar 21 2011

Labels: Type-Security

Comment 42 by jsc...@chromium.org, Oct 5 2011

Labels: SecImpacts-None
Batch update.

Comment 43 by jsc...@chromium.org, Apr 18 2012

Labels: -Restrict-View-SecurityNotify
Lifting view restrictions.

Comment 44 by jsc...@chromium.org, Apr 18 2012

Status: Fixed

Comment 45 by bugdroid1@chromium.org, Oct 13 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 46 by bugdroid1@chromium.org, Mar 9 2013

Project Member
Labels: -Regression -Type-Security Type-Bug-Regression

Comment 47 by bugdroid1@chromium.org, Mar 9 2013

Project Member
Labels: -Area-WebKit -SecSeverity-Medium -SecImpacts-None Cr-Content Security-Severity-Medium Security-Impact-None

Comment 48 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: Security_Severity-None

Comment 49 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Impact-None Security_Impact-None

Comment 50 by bugdroid1@chromium.org, Mar 21 2013

Project Member
Labels: -Security-Severity-Medium -Security_Severity-None Security_Severity-Medium

Comment 51 by bugdroid1@chromium.org, Apr 6 2013

Project Member
Labels: -Cr-Content Cr-Blink

Comment 52 by laforge@google.com, Jul 24 2013

Cc: -jeffreyc@chromium.org

Sign in to add a comment