Bug 113122 – Honor the setting for whether JavaScript markup is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Honor the setting for whether JavaScript markup is enabled

[Geoffrey Garen]

Created attachment 194673 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 194673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194673&action=review

Looks good but maybe eseidel or abarth can comment on the HTMLConstructionSite and HTMLTreeBuilder changes.

> LayoutTests/editing/unsupported-content/script-markup-enabled-setting-expected.txt:4
> +PASS: frames[0].document.getElementsByTagName("script").length should be 0 and is.
> +PASS: frames[1].document.getElementsByTagName("script").length should be 0 and is.

These output doesn't tell us what each line is testing. Why don't we give frames some descriptive ids such as
frameWithHTMLScriptElement and frameWithSVGScriptElement so that the output is self-evident.

> LayoutTests/editing/unsupported-content/script-markup-enabled-setting.html:10
> +function shouldBe(aDescription, a, b)

Why don't we just use js-test-pre.js?

> LayoutTests/editing/unsupported-content/script-markup-enabled-setting.html:14
> +		log("PASS: " + aDescription + " should be " + b + " and is.");
> +		return;

Nit: tab characters.

> LayoutTests/editing/unsupported-content/script-markup-enabled-setting.html:24
> +if (window.testRunner)
> +	testRunner.dumpAsText();
> +
> +if (window.internals)
> +    window.internals.settings.setScriptMarkupEnabled(false);

These two if's can be combined.

[Geoffrey Garen]

Created attachment 194705 [details]
Patch

[Geoffrey Garen]

> These output doesn't tell us what each line is testing. Why don't we give frames some descriptive ids such as
> frameWithHTMLScriptElement and frameWithSVGScriptElement so that the output is self-evident.

Fixed.

> Why don't we just use js-test-pre.js?

I think js-test-pre.js is an anti-pattern. It has two major problems:

(1) Everything executes through eval. This makes testing specific JavaScript details impossible.

(2) It's a huge pile of code. The purpose of a reduced test case is to be the opposite. The first thing I always have to do when I diagnose a test failure in a test that uses js-test-pre.js is to make a reduction of the test. We shouldn't have to make reductions of our reduced test cases!

> Nit: tab characters.

Fixed.

> These two if's can be combined.

Fixed.

[Eric Seidel (no email)]

Comment on attachment 194705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194705&action=review

Is this a setting only for WebKit embedders?  Only on Mac?  If so, it seems we should consider wrapping this in #ifdef MAC

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:466
> +    if (scriptingContentIsAllowed(m_parserContentPolicy) || !toScriptElement(element.get()))
> +        attachLater(currentNode(), element, token->selfClosing());

So the goal here is to drop the <script> tag on the floor when scripting content is disallowed?

[Eric Seidel (no email)]

Comment on attachment 194705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194705&action=review

> Source/WebCore/ChangeLog:20
> +        (This bug has existed for copy/paste for a long time, but other bugs and
> +        quirks in SVG copy/paste papered over it. It's a serious issue now
> +        that non-paste clients will rely on this mode.)

Could you talk a bit about what clients, and if these include browsers/the-web?

The idea of running the parser in a mode whereby it will not produce certain tags seems reasonable.  I'm not 100% convinced the parser wants the complexity, and that this is the right solution for your needs.  But we already do something a bit similar for the XSSAuditor, but that's for attributes (it produces, them but them empties them in a follow-up pass).

It's a little unfortunate that the parser has to know any of this, since it's job is really just to produce a DOM.  The TreeBuidler passes each sript over to the HTMLScriptRunner, which actually knows what to do with the script.  It doesn't currently modify script contents, but might be reasonable to make the HTMLScriptRunner instead just null out the innerHTML of the passed script tag.

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:466
>> +        attachLater(currentNode(), element, token->selfClosing());
> 
> So the goal here is to drop the <script> tag on the floor when scripting content is disallowed?

Maybe the ScriptRunner should do this instead of the construction site?  The construction site is a pretty low level primative.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2872
> +            if (scriptingContentIsAllowed(m_tree.parserContentPolicy()))
> +                m_scriptToProcess = m_tree.currentElement();

I think teaching the HTMLScriptRunner about this instead of the parser would make it easier to make sure you don't miss some later script tag.

[Eric Seidel (no email)]

I guess I find it hard to engage about this change.  There was an off-on-the-wrong-foot webkit-dev thread about this change.  I feel like I'm not really needed here, as I get the sense there is time-pressure for this change, and you don't need my approval to twiddle bits on your own servers. :)

My limited understanding is that you're trying to fix potential security bugs in Mail.  Which seems like a reasonable desire.  I'm not sure this is the right place to fix the behavior.  I wonder how gmail strips script tags. :)  Presumably not in WebKit.

I'm happy to offer further thoughts, but it may be easier for all involved if I just de-CC myself and live a happier life.

[Ryosuke Niwa]

Comment on attachment 194705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194705&action=review

>>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:466
>>> +        attachLater(currentNode(), element, token->selfClosing());
>> 
>> So the goal here is to drop the <script> tag on the floor when scripting content is disallowed?
> 
> Maybe the ScriptRunner should do this instead of the construction site?  The construction site is a pretty low level primative.

HTMLConstructionSite::insertScriptElement, which is the function that appears right above this one, does this check already.

But unifying these scriptingContentIsAllowed checks at one place like ScriptRunner makes a lot of sense since they're scattered
across HTML & XML parsers at the moment. Perhaps we should do it in a separate patch?

[Ryosuke Niwa]

(In reply to comment #7)
> I guess I find it hard to engage about this change.  There was an off-on-the-wrong-foot webkit-dev thread about this change.  I feel like I'm not really needed here, as I get the sense there is time-pressure for this change, and you don't need my approval to twiddle bits on your own servers. :)

My understanding is that this patch is nothing to do with that webkit-dev thread. That one was a discussion about implementing CSP or features like CSP by stripping scripts at parsing time.

This patch is about fixing an existing feature in WebKit so that other applications such as Mail can use it as we've exposed it via settings in the bug 112999.

[Ryosuke Niwa]

I'll note that the "existing feature" is stripping scripting contents when pasting contents from clipboard.

[Ryosuke Niwa]

I'll further note that this is about fixing a bug that already exists for pasting from clipboard case. For whatever reason, we could not come up with a reduction for erroneously executing scripts when pasting from clipboard because of some bug in editing code or parsing SVG (Geoff would know).

[Geoffrey Garen]

> Is this a setting only for WebKit embedders?  Only on Mac?  If so, it seems we should consider wrapping this in #ifdef MAC

This is a setting that's appropriate for mail, newsfeed, and chat apps, which digest markup but don't want to risk script injection attacks. I don't think there's anything special about Mac apps that make them want protection from script injection any more than Linux or Windows apps. Do you?

A side-benefit of this setting is that it allows us to regression test WebKit's copy/paste and drag/drop script injection protection without going through contortions in the test to copy the content and then paste it. For example, that's how I discovered the SVG script injection bug, even though it can't be reproduced by a copy operation in WebKit. I don't think WebKit wants less regression testing on Linux or Windows than on Mac. Do you?

> So the goal here is to drop the <script> tag on the floor when scripting content is disallowed?

That's right: We want to parse exactly "as if" the tag had been there, obeying whatever effect it would have had on the tag soup, without ever executing it or inserting it into the DOM.

[Geoffrey Garen]

> Could you talk a bit about what clients, and if these include browsers/the-web?

The parsing feature -- stripping scripts to avoid script injection attacks -- is used by browsers when executing paste and drop commands. See our regression tests in LayoutTests/editing/pasteboard for some examples. WebKit browsers have worked this way since 2010. The bug fix in this feature -- the one for SVG scripts -- is intended for all WebKit clients, including browsers.

The setting, which allows a client to opt into script injection protection all the time, instead of just when pasting or dropping, is intended for mail, newsfeed, and chat apps, which digest markup but don't want to risk script injection attacks.

> The idea of running the parser in a mode whereby it will not produce certain tags seems reasonable.
> I'm not 100% convinced the parser wants the complexity, and that this is the right solution for your needs.

I want to clarify that the parser has had this feature since 2010, in order to solve a general problem in WebKit. If there's a better way to do things, let's do it, and make WebKit better.

> It's a little unfortunate that the parser has to know any of this, since it's job is really just to produce a DOM.

I might be upside-down about this, but that's actually why I chose the parser: The goal of the feature is to change the generated DOM, by eliminating tags and attributes that map to scripts at runtime.

The reason we want to remove these from the DOM is that down the line, after parsing, we don't want an editing operation, or a DOM clone or move operation, to accidentally inject that script-y DOM, thereby allowing it to execute.

>  The TreeBuidler passes each sript over to the HTMLScriptRunner, which actually knows what to do with the script.  It doesn't currently modify script contents, but might be reasonable to make the HTMLScriptRunner instead just null out the innerHTML of the passed script tag.

I have to admit that I tried a bunch of different options before I settled on the parser, so I won't argue that the current solution is self-evident. The HTMLScriptRunner appealed to me at one point, but I ran into two challenges:

(1) It's a little insecure to leave even an empty script element in the DOM. If an editing operation accidentally inserted text into that element's text content, it could execute as script. I tried to improve this by just having the script runner remove the whole element from its parent, but that sometimes happens at an inconvenient time for the parser's state machine, causing crashes.

(2) The HTMLScriptRunner doesn't control script-y attributes getting added to DOM nodes. Once I decided that the parser had to be in charge of the attributes part of this feature, it seemed natural to keep responsibility for the elements part of this feature in the same place.

(3) When parsing document fragments (paste and drop commands), there is no HTMLScriptRunner.

Given these challenges, do you still think the HTMLScriptRunner is the best place to do this? How would you address these issues?

> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2872
> > +            if (scriptingContentIsAllowed(m_tree.parserContentPolicy()))
> > +                m_scriptToProcess = m_tree.currentElement();
> 
> I think teaching the HTMLScriptRunner about this instead of the parser would make it easier to make sure you don't miss some later script tag.

I share this concern. 

I wonder if there's a reasonable belt-and-suspenders approach here, that takes advantage of HTMLScriptRunner being a bottleneck for script execution at parse time. For example, we could arrange for HTMLScriptRunner to always be NULL if script markup is disabled, instead of only when parsing document fragments, or we could add a RELEASE_ASSERT to HTMLScriptRunner. What do you think?

[Geoffrey Garen]

(In reply to comment #9)
> My understanding is that this patch is nothing to do with that webkit-dev thread.

This is a bit of an overstatement.

I started the webkit-dev thread because Maciej convinced me that stripping script markup was more secure than leaving it in the DOM, and it occurred to me that it would be a security improvement, and a reduction in complexity, to use this technique in all cases where WebKit wanted to disable scripts, instead of just these two or three cases.

So, this patch is related because it motivated me to start that discussion.

However, Jochen Eisinger pointed out that we can't use a single technique everywhere because Chromium's script blocking UI requires special hooks in WebKit. And Adam Barth pointed out that we can't use a single technique everywhere because the more secure technique is incompatible with the current draft spec for CSP.

So, now this patch returns to its humble roots, and fixes the original bug I intended to fix, without making any broader changes in WebKit.

[Ryosuke Niwa]

Created attachment 194719 [details]
Attempt to create a reduction with copy & paste

There appears to be some other mechanism by which we're stripping script content.

Maybe inferno and others in the security team can help us making an actual reduction for this case.
I've started to wonder if this bug should be moved to the security component or not but I guess
we can wait 'til we get an actual reduction?

[Ryosuke Niwa]

+dcheng & +tony^work since they've worked on the clipboardData/dataTransfer code recently.

[Elliott Sprehn]

(In reply to comment #13)
>...
> 
> I have to admit that I tried a bunch of different options before I settled on the parser, so I won't argue that the current solution is self-evident. The HTMLScriptRunner appealed to me at one point, but I ran into two challenges:
> 
> (1) It's a little insecure to leave even an empty script element in the DOM. If an editing operation accidentally inserted text into that element's text content, it could execute as script. I tried to improve this by just having the script runner remove the whole element from its parent, but that sometimes happens at an inconvenient time for the parser's state machine, causing crashes.
> 

This is definitely not the case. A script element can only run once, per the "already started" flag, as specified in the HTML5 spec.

http://www.w3.org/html/wg/drafts/html/master/scripting-1.html#prepare-a-script

This flag even persists through cloning.

[Maciej Stachowiak]

(In reply to comment #17)
> (In reply to comment #13)
> >...
> > 
> > I have to admit that I tried a bunch of different options before I settled on the parser, so I won't argue that the current solution is self-evident. The HTMLScriptRunner appealed to me at one point, but I ran into two challenges:
> > 
> > (1) It's a little insecure to leave even an empty script element in the DOM. If an editing operation accidentally inserted text into that element's text content, it could execute as script. I tried to improve this by just having the script runner remove the whole element from its parent, but that sometimes happens at an inconvenient time for the parser's state machine, causing crashes.
> > 
> 
> This is definitely not the case. A script element can only run once, per the "already started" flag, as specified in the HTML5 spec.
> 
> http://www.w3.org/html/wg/drafts/html/master/scripting-1.html#prepare-a-script
> 
> This flag even persists through cloning.

It doesn't persist through serializing/deserializing though, so if you accidentally put text in a <script>, it's no longer safe to copy/paste the content containing it. Or if you remove and reinsert this script by assigning its parents innerHTML to itself.

In any case, since stripping scripting constructs at parse time is an existing WebKit feature, and this patch just exposes it for a new use, proposals for how to change the way we strip scripting constructs maybe should go in another bug.

[Elliott Sprehn]

(In reply to comment #18)
> ...
> > This is definitely not the case. A script element can only run once, per the "already started" flag, as specified in the HTML5 spec.
> > 
> > http://www.w3.org/html/wg/drafts/html/master/scripting-1.html#prepare-a-script
> > 
> > This flag even persists through cloning.
> 
> It doesn't persist through serializing/deserializing though, so if you accidentally put text in a <script>, it's no longer safe to copy/paste the content containing it. Or if you remove and reinsert this script by assigning its parents innerHTML to itself.
> 

Scripts inserted through innerHTML never run. It's not clear to me why we don't just drop the text from inside script elements and attributes in the deserializer (or just disable all inline script like CSP).

> In any case, since stripping scripting constructs at parse time is an existing WebKit feature, and this patch just exposes it for a new use, proposals for how to change the way we strip scripting constructs maybe should go in another bug.

I really don't think this is the right way to fix the issue for desktop apps, but it's important to note that doing this on the open web can, and does, break pages:

<script></script>
<span>...</span>
<style>script + div { color: red; }</script>

or even more common:

<div>
  <script></script>
  <span>...</span>
</div>

<style>div:nth-child(2) { color: red; }</style>

Why doesn't Mail or Adium just use CSP though, or a sandboxed iframe that doesn't allow scripts?

[Ryosuke Niwa]

(In reply to comment #18)
> (In reply to comment #17)
> >
> > This is definitely not the case. A script element can only run once, per the "already started" flag, as specified in the HTML5 spec.
> > 
> > http://www.w3.org/html/wg/drafts/html/master/scripting-1.html#prepare-a-script
> > 
> > This flag even persists through cloning.
> 
> It doesn't persist through serializing/deserializing though, so if you accidentally put text in a <script>, it's no longer safe to copy/paste the content containing it.

It is. Our paste code disables script elements.

> Or if you remove and reinsert this script by assigning its parents innerHTML to itself.

Apparently we don't do this. setInnerHTML strips the script element's content so the script element doesn't do anything.

(In reply to comment #19)
> It's not clear to me why we don't just drop the text from inside script elements and attributes in the deserializer (or just disable all inline script like CSP).

We do do that.

> > In any case, since stripping scripting constructs at parse time is an existing WebKit feature, and this patch just exposes it for a new use, proposals for how to change the way we strip scripting constructs maybe should go in another bug.
> 
> I really don't think this is the right way to fix the issue for desktop apps, but it's important to note that doing this on the open web can, and does, break pages:
> 
> <script></script>
> <span>...</span>
> <style>script + div { color: red; }</script>
> 
> or even more common:
> 
> <div>
>   <script></script>
>   <span>...</span>
> </div>
> 
> <style>div:nth-child(2) { color: red; }</style>

That's a very good point. This will indeed be a problem for Mail.

[Maciej Stachowiak]

(In reply to comment #20)
> > 
> > I really don't think this is the right way to fix the issue for desktop apps, but it's important to note that doing this on the open web can, and does, break pages:
> > 
> > <script></script>
> > <span>...</span>
> > <style>script + div { color: red; }</script>
> > 
> > or even more common:
> > 
> > <div>
> >   <script></script>
> >   <span>...</span>
> > </div>
> > 
> > <style>div:nth-child(2) { color: red; }</style>
> 
> That's a very good point. This will indeed be a problem for Mail.

As a point of comparison, in GMail and MobileMe Mail (I cold not get YahooMail to work) both strip red from minimally corrected versions of the above snippets. Is such a construct actually involving the presence of scripts common on the web? Evidence seems to be that it's not common enough that mail clients care more about it than about stripping script tags.

 - Maciej

<div>...</div>

[Elliott Sprehn]

(In reply to comment #21)
> ...
> > 
> > That's a very good point. This will indeed be a problem for Mail.
> 
> As a point of comparison, in GMail and MobileMe Mail (I cold not get YahooMail to work) both strip red from minimally corrected versions of the above snippets. Is such a construct actually involving the presence of scripts common on the web? Evidence seems to be that it's not common enough that mail clients care more about it than about stripping script tags.
> 
>  - Maciej
> 
> <div>...</div>

Outlook 2007+ has such crippled CSS support that I don't know if people depend on that much. I have seen some internal apps at Google break when Google Feedback's page copier which is used to make an inert copy of a page removed script tags though.

rniwa and I have debunked the theoretical exploits here: innerHTML never runs scripts, editing drops the contents of scripts on paste, scripts never run more than once in the document.

I don't understand what this patch is attempting to solve, nor is it clear why Mail isn't using a sandboxed iframe if you were really paranoid that all the protections we already have are not enough.

[Ryosuke Niwa]

We can't always use sandboxed iframe for this feature because some OS X applications inject scripts into the document.

[Geoffrey Garen]

Created attachment 194769 [details]
Script injection attack examples

Test case showing some script injection attacks, and how CSP and other mechanisms do not protect against them.

[Geoffrey Garen]

> A script element can only run once, per the "already started" flag, as specified in the HTML5 spec.

As Maciej mentioned, the already started flag does not protect against serialization / deserialization attacks. See the attached test case.

> Why doesn't Mail or Adium just use CSP though, or a sandboxed iframe that doesn't allow scripts?

(1) CSP blocks user/host-injected JavaScript, which breaks apps that implement features in JavaScript.

(2) CSP is subject to script injection attacks. See the attached test case.

> doing this on the open web can, and does, break pages

It sounds like you're objecting to WebKit's existing behavior for paste and drop.  I would be surprised if it broke the web, since we haven't gotten any bug reports about it over the past three years. That said, the specifics of how we should handle paste and drop are not really germane to this bug, which just exposes WebKit's paste/drop protection as a setting for native apps.

> I have seen some internal apps at Google break when Google Feedback's page copier which is used to make an inert copy of a page removed script tags though.

Fair enough. But that's not the open web. Maciej's examples seem to indicate that the de fact standard on the open web is for mail clients to wholly strip script elements. Do you have any examples of web-based mail clients that leave script elements in the document?

[Ryosuke Niwa]

(In reply to comment #25)
>
> > doing this on the open web can, and does, break pages
> 
> It sounds like you're objecting to WebKit's existing behavior for paste and drop.  I would be surprised if it broke the web, since we haven't gotten any bug reports about it over the past three years. That said, the specifics of how we should handle paste and drop are not really germane to this bug, which just exposes WebKit's paste/drop protection as a setting for native apps.

It's probably safer to strip the contents of the script element and leave empty script elements alone just like what we do in setInnerHTML than removing the script elements entirely per Elliot's examples.

On the other hand, editing code is almost completely broken for cases where we have such style rules so I'm not certain if it's something we should really be concerned about.

[Geoffrey Garen]

Created attachment 194891 [details]
Specific demonstration of why an empty script element is dangerous

[Geoffrey Garen]

> It's probably safer to strip the contents of the script element and leave empty script elements alone just like what we do in setInnerHTML than removing the script elements entirely per Elliot's examples.

I've attached an additional test case demonstrating the specific attack an empty script element opens our users up to.

Which approach is more compatible with the web all depends on what other mail, chat, etc. content viewers do on the web. So far, we have two examples of mail content viewers that strip the script element, and zero examples of mail content viewers that preserve it. The evidence says that stripping the script element is both more secure and more compatible.

All of that being said, I'll repeat: This patch just exposes WebKit's existing behavior as a setting, and fixes an oversight that prevented that behavior from covering svg script elements. This patch did not invent the behavior, and ideas for changing the behavior are not germane to this patch.

[Geoffrey Garen]

Ryosuke mentioned that some Google engineers said on IRC that they thought stripping scripts from emails was something unique to Apple Mail. Let me re-re-state: GMail does it too.

[Elliott Sprehn]

(In reply to comment #29)
> Ryosuke mentioned that some Google engineers said on IRC that they thought stripping scripts from emails was something unique to Apple Mail. Let me re-re-state: GMail does it too.

That's not what I said. I said that the examples given here are effectively eval() and that the whole argument of this bug is that Apple's Mail.app engineers are apparently incapable of writing safe JS so we need to protect them at the platform level. Making every nested iframe CSP, or having Mail.app put the sandbox attribute on every nested iframe, and CSP at the top level (which seems totally reasonable) also fixes this bug.

I can't fathom a reason that Mail.app would put the HTML contents of an email into anything _but_ a sandboxed iframe. :)

Anyway, I don't want to block this patch, but I do object to claiming that Adium needs to turn this feature on to be safe. They can be just as safe by using CSP and sandbox and not have the surprising parser magic, and indeed this is what Chrome OS does to be secure.

[Geoffrey Garen]

> Making every nested iframe CSP, or having Mail.app put the sandbox attribute on every nested iframe, and CSP at the top level (which seems totally reasonable) also fixes this bug.

I guess you didn't notice my comments above, where I explained why that wouldn't work.

[Ryosuke Niwa]

Give that Gmail also strips script elements, the point Elliot made against this approach that some CSS rules may apply differently is now an argument for this approach. If we didn't strip the script element, Maill will render an email differently than on Gmail.

[Elliott Sprehn]

(In reply to comment #32)
> Give that Gmail also strips script elements, the point Elliot made against this approach that some CSS rules may apply differently is now an argument for this approach. If we didn't strip the script element, Maill will render an email differently than on Gmail.

Seems reasonable.

[Ryosuke Niwa]

I'll also note that clipboard API working draft has HTML paste sanitation algorithm (http://www.w3.org/TR/clipboard-apis/#cross-origin-html-paste-sanitization-algorithm). This algorithm has been removed from the latest editor's draft due to lack of implementations but our behavior matches this algorithm's behavior at least for script element and on* attributes.

I've emailed public-webapps to define what UAs are allowed to address security concerns so that, specifically, things like CSS rules dependent on script element being present in the pasted content won't be an issue for us.

[Geoffrey Garen]

Committed r147018: <http://trac.webkit.org/changeset/147018>

[Maciej Stachowiak]

(In reply to comment #32)
> Give that Gmail also strips script elements, the point Elliot made against this approach that some CSS rules may apply differently is now an argument for this approach. If we didn't strip the script element, Maill will render an email differently than on Gmail.

I did more testing - in addition to Gmail, the following all completely strip scripts in mail messages: iCloud Mail, YahooMail, Microsoft Live Mail.

None of these appear to put HTML emails in a sandboxed iframe, nor do they appear to use CSP, even on browsers that support these features.

As others have concluded, it's probably better for Mail.app to be compatible with popular webmail clients than to do something different and more complicated.

[Ryosuke Niwa]

Debug tester fixed in http://trac.webkit.org/changeset/147050.