Bug 71851 – Implement script MIME restrictions for X-Content-Type-Options: nosniff

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thomas Sepez]

Prevent executing script along the lines of: http://msdn.microsoft.com/en-us/library/gg622941(v=vs.85).aspx

[Adam Barth]

We probably use put this behind an ENABLE(NOSNIFF) ifdef.

[Thomas Sepez]

Good point.  ENABLE(NOSNIFF) will reduce friction to approval.

[Thomas Sepez]

Created attachment 114373 [details]
Proposed patch.

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Thomas Sepez]

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?

[WebKit Review Bot]

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.

[Thomas Sepez]

Created attachment 114387 [details]
Patch

[Alexey Proskuryakov]

> Q. ENABLE_NOSNIFF is on by default

New ENABLE macros and their default values need to be discussed on webkit-dev.

[Adam Barth]

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.

[Adam Barth]

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.

[WebKit Review Bot]

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

[Ojan Vafai]

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.

[Mike West]

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

[Thomas Sepez]

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.

[Mike West]

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

[Mike West]

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?

[Thomas Sepez]

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

[Mike West]

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

[Mike West]

Created attachment 186822 [details]
Patch

[Mike West]

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

[Adam Barth]

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

[Mike West]

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

[Mike West]

Created attachment 187027 [details]
Patch

[Mike West]

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

[Adam Barth]

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.

[Mike West]

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

[Mike West]

Created attachment 187432 [details]
Patch

[Build Bot]

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

[Mike West]

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. :)

[WebKit Review Bot]

Comment on attachment 187432 [details]
Patch

Clearing flags on attachment: 187432

Committed r142683: <http://trac.webkit.org/changeset/142683>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.