Bug 163100 – [Mac] Write WebArchive to the pasteboard when copying image in WebKit

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

[Chris Dumez]

Write HTML to the pasteboard when copying image in WebKit. This fixes pasting such images to a content editable field in WebKit because HTML takes priority over RTFD when reading from the pasteboard in WebKit. Using RTFD when pasting the image in WebKit was causing issues because:
1. The pasted image would not be displayed because our RTFD import code is buggy.
2. The pasted image URL was a webkit-fake-url://
3. Formatting associated to the image (e.g. inline style) would be lost

Firefox also writes HTML to the pasteboard on Mac when copying an image, in addition to the image.

[Chris Dumez]

Created attachment 290892 [details]
Patch

[Chris Dumez]

Manual test case:
1. open a page with images, e.g: http://www.bbc.com/news/science-environment-37511861 (or you can open google image search and type "landscape")
2. right click on any image and pick "Copy image"
3. open this fiddle: https://jsfiddle.net/madkdzre/ and paste the image into frame.

[Build Bot]

Comment on attachment 290892 [details]
Patch

Attachment 290892 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2235531

New failing tests:
editing/pasteboard/copy-standalone-image.html

[Build Bot]

Created attachment 290897 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290892 [details]
Patch

Attachment 290892 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2235537

New failing tests:
editing/pasteboard/copy-standalone-image.html

[Build Bot]

Created attachment 290899 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

You should look into why editing/pasteboard/copy-standalone-image.html is failing...

[Chris Dumez]

Created attachment 290937 [details]
Patch

[John Wilander]

We should probably block SVGs from HTML copy and set up a test case with this kind of image:

<?xml version="1.0" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

<svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
   <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900" stroke="#004400"/>
   <s cript type="text/javascript">
      alert('Hello!');
   </s cript>
</svg>

(Markup deliberately broken – "s cript".)

[Chris Dumez]

(In reply to comment #9)
> We should probably block SVGs from HTML copy and set up a test case with
> this kind of image:
> 
> <?xml version="1.0" standalone="no"?>
> <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
> "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> 
> <svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
>    <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900"
> stroke="#004400"/>
>    <s cript type="text/javascript">
>       alert('Hello!');
>    </s cript>
> </svg>
> 
> (Markup deliberately broken – "s cript".)

John, how does this relate to my change? This is not an image that we are copying.

The pasting of HTML into WebKit is pre-existing code and we already support this (I also know there are safety measures in place so that we do not import scripts).

The only part that is new in this patch is that copying an image in WebKit will now also add its HTML representation to the clipboard and that this HTML representation will be used by WebKit when pasting the image.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > We should probably block SVGs from HTML copy and set up a test case with
> > this kind of image:
> > 
> > <?xml version="1.0" standalone="no"?>
> > <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
> > "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > 
> > <svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
> >    <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900"
> > stroke="#004400"/>
> >    <s cript type="text/javascript">
> >       alert('Hello!');
> >    </s cript>
> > </svg>
> > 
> > (Markup deliberately broken – "s cript".)
> 
> John, how does this relate to my change? This is not an image that we are
> copying.
> 
> The pasting of HTML into WebKit is pre-existing code and we already support
> this (I also know there are safety measures in place so that we do not
> import scripts).
> 
> The only part that is new in this patch is that copying an image in WebKit
> will now also add its HTML representation to the clipboard and that this
> HTML representation will be used by WebKit when pasting the image.

FYI, I have tried copy/pasting the code you provided in WebKit with and without my patch. The result is that it pastes:
<s cript="" type="text/javascript">&nbsp;alert('Hello!');</s>

It does not alert anything.

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > We should probably block SVGs from HTML copy and set up a test case with
> > > this kind of image:
> > > 
> > > <?xml version="1.0" standalone="no"?>
> > > <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
> > > "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > > 
> > > <svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
> > >    <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900"
> > > stroke="#004400"/>
> > >    <s cript type="text/javascript">
> > >       alert('Hello!');
> > >    </s cript>
> > > </svg>
> > > 
> > > (Markup deliberately broken – "s cript".)
> > 
> > John, how does this relate to my change? This is not an image that we are
> > copying.
> > 
> > The pasting of HTML into WebKit is pre-existing code and we already support
> > this (I also know there are safety measures in place so that we do not
> > import scripts).
> > 
> > The only part that is new in this patch is that copying an image in WebKit
> > will now also add its HTML representation to the clipboard and that this
> > HTML representation will be used by WebKit when pasting the image.
> 
> FYI, I have tried copy/pasting the code you provided in WebKit with and
> without my patch. The result is that it pastes:
> <s cript="" type="text/javascript">&nbsp;alert('Hello!');</s>
> 
> It does not alert anything.

This is not in this patch but HTML is imported from the pasteboard using:
createFragmentFromMarkup(document, stringOmittingMicrosoftPrefix, emptyString(), DisallowScriptingAndPluginContent);

Notice the DisallowScriptingAndPluginContent parameter.

[John Wilander]

The deliberate separation into "s cript" was to not XSS Bugzilla. I don't think we have such vulnerabilities but I try not to paste script into web forms. When you test with it you should remove the extra space.

The reason why I bring this up is that such SVGs are allowed in img elements where they are blocked from executing scripts. However, we could end up being vulnerable if the SVG is pasted in another context.

[Chris Dumez]

(In reply to comment #13)
> The deliberate separation into "s cript" was to not XSS Bugzilla. I don't
> think we have such vulnerabilities but I try not to paste script into web
> forms. When you test with it you should remove the extra space.
> 
> The reason why I bring this up is that such SVGs are allowed in img elements
> where they are blocked from executing scripts. However, we could end up
> being vulnerable if the SVG is pasted in another context.

Not sure I understand. Having an SVG under an IMG is not valid HTML (IMG does not have a closing tag). Even if you might be able to construct such a tree using JS, the children of the IMG would be stripped out when calling outerHTML() on the IMG.

Also, as pointed out before, we already strip out scripts when pasting HTML in WebKit.

[John Wilander]

(In reply to comment #14)
> (In reply to comment #13)
> > The deliberate separation into "s cript" was to not XSS Bugzilla. I don't
> > think we have such vulnerabilities but I try not to paste script into web
> > forms. When you test with it you should remove the extra space.
> > 
> > The reason why I bring this up is that such SVGs are allowed in img elements
> > where they are blocked from executing scripts. However, we could end up
> > being vulnerable if the SVG is pasted in another context.
> 
> Not sure I understand. Having an SVG under an IMG is not valid HTML (IMG
> does not have a closing tag). Even if you might be able to construct such a
> tree using JS, the children of the IMG would be stripped out when calling
> outerHTML() on the IMG.

<img src="image.svg">

> Also, as pointed out before, we already strip out scripts when pasting HTML
> in WebKit.

We should have a test case for it. Attackers will convince victims to paste images into social media pages if there's a way to get SVG scripts to execute that way.

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > The deliberate separation into "s cript" was to not XSS Bugzilla. I don't
> > > think we have such vulnerabilities but I try not to paste script into web
> > > forms. When you test with it you should remove the extra space.
> > > 
> > > The reason why I bring this up is that such SVGs are allowed in img elements
> > > where they are blocked from executing scripts. However, we could end up
> > > being vulnerable if the SVG is pasted in another context.
> > 
> > Not sure I understand. Having an SVG under an IMG is not valid HTML (IMG
> > does not have a closing tag). Even if you might be able to construct such a
> > tree using JS, the children of the IMG would be stripped out when calling
> > outerHTML() on the IMG.
> 
> <img src="image.svg">

But image.svg is an external resource here, it does not get copy/pasted. What gets copied is the string '<img src="http://url-to/image.svg">'.

I don't mind writing a test case, I'd just like to understand exactly what I am supposed to test.

[John Wilander]

Ah, got it. Then there should not be an issue. I did not know whether we included the markup from loaded SVGs.

[John Wilander]

HTML clipboard data has been abused from many sources such as MS Word documents and PDFs. You get a victim to paste in for instance Gmail's new message form and boom.

[Chris Dumez]

Comment on attachment 290937 [details]
Patch

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

> Source/WebCore/editing/mac/EditorMac.mm:439
> +    pasteboardImage.html = imageElement.outerHTML();

This is actually broken for relative URLs :/ We need to resolve the absolute URL when we generate the HTML.

[Chris Dumez]

Ryosuke suggested we write a WebArchive to the pasteboard instead of HTML because this is what WebKit usually does when copy/pasting HTML content. It also has the benefit of working for local file URLs.

[Chris Dumez]

Created attachment 290971 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290971 [details]
Patch

Clearing flags on attachment: 290971

Committed r206965: <http://trac.webkit.org/changeset/206965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

editing/pasteboard/copy-standalone-image.html is failing on GTK now, see bug #163184.

[Radar WebKit Bug Importer]

<rdar://problem/29184049>