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
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All Unspecified
: P2 Minor
Assigned To: Mike West
:
Depends on: 72414 109029
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-08 13:15 PST by Thomas Sepez
Modified: 2013-02-12 15:46 PST (History)
15 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 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 Adam Barth 2011-11-08 16:03:30 PST
We probably use put this behind an ENABLE(NOSNIFF) ifdef.
Comment 2 Thomas Sepez 2011-11-09 10:55:32 PST
Good point.  ENABLE(NOSNIFF) will reduce friction to approval.
Comment 3 Thomas Sepez 2011-11-09 15:11:40 PST
Created attachment 114373 [details]
Proposed patch.
Comment 4 WebKit Review Bot 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 Adam Barth 2011-11-09 15:20:46 PST
Comment on attachment 114373 [details]
Proposed patch.

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 Thomas Sepez 2011-11-09 16:04:01 PST
Created attachment 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 WebKit Review Bot 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 Thomas Sepez 2011-11-09 16:14:03 PST
Created attachment 114387 [details]
Patch
Comment 9 Alexey Proskuryakov 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 Adam Barth 2011-11-09 16:53:59 PST
Comment on attachment 114387 [details]
Patch

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 Adam Barth 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 WebKit Review Bot 2011-11-09 16:59:21 PST
Comment on attachment 114387 [details]
Patch

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 Ojan Vafai 2012-04-19 16:41:18 PDT
Comment on attachment 114387 [details]
Patch

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 Mike West 2012-09-12 10:04:16 PDT
(In reply to comment #13)
> (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.

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 Thomas Sepez 2012-09-12 10:59:55 PDT
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 Mike West 2012-09-12 11:17:36 PDT
(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 Mike West 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 Thomas Sepez 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 Mike West 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 Mike West 2013-02-06 04:24:09 PST
Created attachment 186822 [details]
Patch
Comment 21 Mike West 2013-02-06 04:35:59 PST
(In reply to comment #20)
> Created an attachment (id=186822) [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 Adam Barth 2013-02-06 10:29:40 PST
Comment on attachment 186822 [details]
Patch

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 Mike West 2013-02-06 11:01:17 PST
(In reply to comment #22)
> (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.

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 Mike West 2013-02-07 02:20:00 PST
Created attachment 187027 [details]
Patch
Comment 25 Mike West 2013-02-07 02:21:19 PST
(In reply to comment #22)
> (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.

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 Adam Barth 2013-02-07 10:58:27 PST
Comment on attachment 187027 [details]
Patch

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 Mike West 2013-02-07 11:02:30 PST
(In reply to comment #26)
> (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.

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 Mike West 2013-02-09 07:24:21 PST
Created attachment 187432 [details]
Patch
Comment 29 Build Bot 2013-02-09 08:35:05 PST
Comment on attachment 187432 [details]
Patch

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 Mike West 2013-02-12 15:25:51 PST
Comment on attachment 187432 [details]
Patch

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 WebKit Review Bot 2013-02-12 15:46:29 PST
Comment on attachment 187432 [details]
Patch

Clearing flags on attachment: 187432

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