Bug 71851 - Implement script MIME restrictions for X-Content-Type-Options: nosniff
: Implement script MIME restrictions for X-Content-Type-Options: nosniff
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All Unspecified
: P2 Minor
Assigned To:
:
:
: 72414 109029
:
  Show dependency treegraph
 
Reported: 2011-11-08 13:15 PST by
Modified: 2013-02-12 15:46 PST (History)


Attachments
Proposed patch. (27.96 KB, patch)
2011-11-09 15:11 PST, Thomas Sepez
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Patch + address comments (28.24 KB, patch)
2011-11-09 16:04 PST, Thomas Sepez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.19 KB, patch)
2011-11-09 16:14 PST, Thomas Sepez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.74 KB, patch)
2013-02-06 04:24 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.44 KB, patch)
2013-02-07 02:20 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.34 KB, patch)
2013-02-09 07:24 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-08 13:15:18 PST
Prevent executing script along the lines of: http://msdn.microsoft.com/en-us/library/gg622941(v=vs.85).aspx
------- Comment #1 From 2011-11-08 16:03:30 PST -------
We probably use put this behind an ENABLE(NOSNIFF) ifdef.
------- Comment #2 From 2011-11-09 10:55:32 PST -------
Good point.  ENABLE(NOSNIFF) will reduce friction to approval.
------- Comment #3 From 2011-11-09 15:11:40 PST -------
Created an attachment (id=114373) [details]
Proposed patch.
------- Comment #4 From 2011-11-09 15:14:01 PST -------
Attachment 114373 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/loader/cache/CachedScript.cpp:154:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #5 From 2011-11-09 15:20:46 PST -------
(From update of attachment 114373 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=114373&action=review

> Source/WebCore/dom/ScriptElement.cpp:52
> +#if ENABLE(NOSNIFF)
> +#include "Console.h"
> +#include "DOMWindow.h"
> +#endif

These can just be included unconditionally.

> Source/WebCore/dom/ScriptElement.cpp:300
> +        String consoleMessage = "Refused to load script from " + m_cachedScript->url().string() + " due to MIME type mismatch.\n";

Do we want to say something about nosniff here?

> Source/WebCore/dom/ScriptElement.cpp:301
> +        m_element->document()->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, consoleMessage, 1, String());

m_element->document()->domWindow()->console()->addMessage

can be much shorter:

m_element->document()->addConsoleMessage

> Source/WebCore/loader/cache/CachedScript.cpp:43
> +#if ENABLE(NOSNIFF)
> +#include "HTTPParsers.h"
> +#endif

Same here.

> Source/WebCore/loader/cache/CachedScript.cpp:167
> +            permittedMimeTypes.add("application/ecmascript");
> +            permittedMimeTypes.add("application/javascript");
> +            permittedMimeTypes.add("application/x-javascript");
> +            permittedMimeTypes.add("text/ecmascript");
> +            permittedMimeTypes.add("text/javascript");
> +            permittedMimeTypes.add("text/jscript");
> +            permittedMimeTypes.add("text/x-javascript");
> +            permittedMimeTypes.add("text/vbs");
> +            permittedMimeTypes.add("text/vbscript");

Does this list of types exist somewhere already?

> Source/WebCore/platform/network/HTTPParsers.cpp:379
> +    if (contentTypeOptions.findIgnoringCase("nosniff") != notFound)

This will match xnosniffbob, which isn't right.
------- Comment #6 From 2011-11-09 16:04:01 PST -------
Created an attachment (id=114383) [details]
Patch + address comments

Q. ENABLE_NOSNIFF is on by default so that the new tests pass.  Better way of doing this?
------- Comment #7 From 2011-11-09 16:08:44 PST -------
Attachment 114383 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/dom/ScriptElement.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #8 From 2011-11-09 16:14:03 PST -------
Created an attachment (id=114387) [details]
Patch
------- Comment #9 From 2011-11-09 16:30:29 PST -------
> Q. ENABLE_NOSNIFF is on by default

New ENABLE macros and their default values need to be discussed on webkit-dev.
------- Comment #10 From 2011-11-09 16:53:59 PST -------
(From update of attachment 114387 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=114387&action=review

> Source/WebCore/dom/ScriptElement.cpp:297
> +        String consoleMessage = "Refused to load script from " + m_cachedScript->url().string() + " due to MIME type mismatch and X-Content-Type-Options header.\n";

No \n needed.
------- Comment #11 From 2011-11-09 16:55:13 PST -------
Tom, would you be willing to send a note to webkit-dev?  IMHO, this really a "feature", but there's no harm in sending a note.  We can also skip the ENABLE flag if that's easier.
------- Comment #12 From 2011-11-09 16:59:21 PST -------
(From update of attachment 114387 [details])
Attachment 114387 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10401131

New failing tests:
http/tests/security/nosniff/script-blocked-nosniff.html
------- Comment #13 From 2012-04-19 16:41:18 PST -------
(From update of attachment 114387 [details])
As per the webkit-dev discussion, the flag should be off for the non-chromium ports. Each port should enable it explicitly since they'll need to add extra port-specific code. The way you handle the tests for a disabled flag is to add the tests to the Skipped list of the ports that have it disabled.

Other than that and Adam's one comment about the newline, the patch looks fine to me.
------- Comment #14 From 2012-09-12 10:04:16 PST -------
(In reply to comment #13)
> (From update of attachment 114387 [details] [details])
> As per the webkit-dev discussion, the flag should be off for the non-chromium ports. Each port should enable it explicitly since they'll need to add extra port-specific code. The way you handle the tests for a disabled flag is to add the tests to the Skipped list of the ports that have it disabled.
> 
> Other than that and Adam's one comment about the newline, the patch looks fine to me.

Is this still open, Tom, or has it landed in some other form? If not, I can take a stab at rebasing/rewriting it onto the current ToT if that would be helpful.
------- Comment #15 From 2012-09-12 10:59:55 PST -------
Mike, feel free to take a stab.  But see http://code.google.com/p/chromium/issues/detail?id=95901 and https://bugs.webkit.org/show_bug.cgi?id=72414 for reasons why this has stalled.
------- Comment #16 From 2012-09-12 11:17:36 PST -------
(In reply to comment #15)
> Mike, feel free to take a stab.  But see http://code.google.com/p/chromium/issues/detail?id=95901 and https://bugs.webkit.org/show_bug.cgi?id=72414 for reasons why this has stalled.

I see. I missed the dependency... I have no idea how 304s work, but I'll take a look anyway. Might be interesting. :)
------- Comment #17 From 2013-02-05 04:45:37 PST -------
Tom, assuming that the patch in the blocking bugs goes through, do you want to pick this back up, or should I work through reanimating your patch?
------- Comment #18 From 2013-02-05 10:48:28 PST -------
(In reply to comment #17)
> Tom, assuming that the patch in the blocking bugs goes through, do you want to pick this back up, or should I work through reanimating your patch?

Mike, I'm a little tied up at the moment with other things; If you'd like to take a stab at it, I would appreciate it.  Thanks.
------- Comment #19 From 2013-02-05 10:59:53 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> > Tom, assuming that the patch in the blocking bugs goes through, do you want to pick this back up, or should I work through reanimating your patch?
> 
> Mike, I'm a little tied up at the moment with other things; If you'd like to take a stab at it, I would appreciate it.  Thanks.

Sure, I'll poke at it tomorrow.
------- Comment #20 From 2013-02-06 04:24:09 PST -------
Created an attachment (id=186822) [details]
Patch
------- Comment #21 From 2013-02-06 04:35:59 PST -------
(In reply to comment #20)
> Created an attachment (id=186822) [details] [details]
> Patch

Tom, Ojan, Adam: would you mind taking a quick look at this patch? I've split the ENABLE flag into a separate patch for clarity, and added some additional tests, but it's otherwise very similar to the original patch, so I hope the review will be straightforward.
------- Comment #22 From 2013-02-06 10:29:40 PST -------
(From update of attachment 186822 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=186822&action=review

As I mentioned on webkit-dev, we should check that the latest version of IE still has this behavior.

> Source/WebCore/loader/cache/CachedScript.cpp:134
> +            // List of types from http://msdn.microsoft.com/en-us/library/gg622941(v=vs.85).aspx

We already have a list of MIME types that we accept in the "type" attribute of the script tag.  Should we use that list instead?

> Source/WebCore/loader/cache/CachedScript.cpp:143
> +            permittedMimeTypes.add("text/vbs");
> +            permittedMimeTypes.add("text/vbscript");

We don't support vbscript, so presumably we wouldn't want have these on the list....
------- Comment #23 From 2013-02-06 11:01:17 PST -------
(In reply to comment #22)
> (From update of attachment 186822 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186822&action=review
> 
> As I mentioned on webkit-dev, we should check that the latest version of IE still has this behavior.

Thannks, I'll will respond on webkit-dev once I have some data.

> > Source/WebCore/loader/cache/CachedScript.cpp:134
> > +            // List of types from http://msdn.microsoft.com/en-us/library/gg622941(v=vs.85).aspx
> 
> We already have a list of MIME types that we accept in the "type" attribute of the script tag.  Should we use that list instead?

I love you, ScriptElement::isScriptTypeSupported is much simpler.
------- Comment #24 From 2013-02-07 02:20:00 PST -------
Created an attachment (id=187027) [details]
Patch
------- Comment #25 From 2013-02-07 02:21:19 PST -------
(In reply to comment #22)
> (From update of attachment 186822 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186822&action=review
> 
> As I mentioned on webkit-dev, we should check that the latest version of IE still has this behavior.

As I noted on the list, it looks like IE10 does have this behavior.

> > Source/WebCore/loader/cache/CachedScript.cpp:134
> > +            // List of types from http://msdn.microsoft.com/en-us/library/gg622941(v=vs.85).aspx
> 
> We already have a list of MIME types that we accept in the "type" attribute of the script tag.  Should we use that list instead?

Yes! MIMETypeRegistry FTW.

> > Source/WebCore/loader/cache/CachedScript.cpp:143
> > +            permittedMimeTypes.add("text/vbs");
> > +            permittedMimeTypes.add("text/vbscript");
> 
> We don't support vbscript, so presumably we wouldn't want have these on the list....

The list now matches the MIME types we currently accept, and I've updated the tests and ChangeLog to match.
------- Comment #26 From 2013-02-07 10:58:27 PST -------
(From update of attachment 187027 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=187027&action=review

The change LGTM, but can we wait until Tuesday to land it?  That means it will have a full Dev channel cycle to catch compat problems before being promoted to the Beta channel.

> Source/WebCore/loader/cache/CachedScript.cpp:134
> +        return false;
> +    return true;

There's no need to use an if statement.  You can just return the Boolean value directly.
------- Comment #27 From 2013-02-07 11:02:30 PST -------
(In reply to comment #26)
> (From update of attachment 187027 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187027&action=review
> 
> The change LGTM, but can we wait until Tuesday to land it?  That means it will have a full Dev channel cycle to catch compat problems before being promoted to the Beta channel.

No rush. I'll land after the Chromium branch.

> > Source/WebCore/loader/cache/CachedScript.cpp:134
> > +        return false;
> > +    return true;
> 
> There's no need to use an if statement.  You can just return the Boolean value directly.

/me facepalm.

Yes. I could do that, couldn't I?

Thanks!
------- Comment #28 From 2013-02-09 07:24:21 PST -------
Created an attachment (id=187432) [details]
Patch
------- Comment #29 From 2013-02-09 08:35:05 PST -------
(From update of attachment 187432 [details])
Attachment 187432 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16474391

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
------- Comment #30 From 2013-02-12 15:25:51 PST -------
(From update of attachment 187432 [details])
Chromium has branched; it should be safe to land this now. Throwing to the CQ in the hopes that it still applies cleanly. :)
------- Comment #31 From 2013-02-12 15:46:29 PST -------
(From update of attachment 187432 [details])
Clearing flags on attachment: 187432

Committed r142683: <http://trac.webkit.org/changeset/142683>
------- Comment #32 From 2013-02-12 15:46:36 PST -------
All reviewed patches have been landed.  Closing bug.