Bug 163100 - [Mac] Write WebArchive to the pasteboard when copying image in WebKit
Summary: [Mac] Write WebArchive to the pasteboard when copying image in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 49141
  Show dependency treegraph
 
Reported: 2016-10-06 21:33 PDT by Chris Dumez
Modified: 2016-11-09 12:31 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.38 KB, patch)
2016-10-06 21:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.09 MB, application/zip)
2016-10-06 22:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.80 MB, application/zip)
2016-10-06 22:54 PDT, Build Bot
no flags Details
Patch (268.65 KB, patch)
2016-10-07 10:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.35 KB, patch)
2016-10-07 14:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-10-06 21:33:10 PDT
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.
Comment 1 Chris Dumez 2016-10-06 21:48:30 PDT
Created attachment 290892 [details]
Patch
Comment 2 Chris Dumez 2016-10-06 21:52:24 PDT
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.
Comment 3 Build Bot 2016-10-06 22:44:34 PDT
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
Comment 4 Build Bot 2016-10-06 22:44:37 PDT
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
Comment 5 Build Bot 2016-10-06 22:54:10 PDT
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
Comment 6 Build Bot 2016-10-06 22:54:13 PDT
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
Comment 7 Ryosuke Niwa 2016-10-06 23:15:41 PDT
You should look into why editing/pasteboard/copy-standalone-image.html is failing...
Comment 8 Chris Dumez 2016-10-07 10:03:35 PDT
Created attachment 290937 [details]
Patch
Comment 9 John Wilander 2016-10-07 10:29:01 PDT
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".)
Comment 10 Chris Dumez 2016-10-07 10:38:41 PDT
(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.
Comment 11 Chris Dumez 2016-10-07 10:41:52 PDT
(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.
Comment 12 Chris Dumez 2016-10-07 10:45:00 PDT
(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.
Comment 13 John Wilander 2016-10-07 10:54:08 PDT
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.
Comment 14 Chris Dumez 2016-10-07 11:58:55 PDT
(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.
Comment 15 John Wilander 2016-10-07 12:23:53 PDT
(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.
Comment 16 Chris Dumez 2016-10-07 12:29:34 PDT
(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.
Comment 17 John Wilander 2016-10-07 12:33:24 PDT
Ah, got it. Then there should not be an issue. I did not know whether we included the markup from loaded SVGs.
Comment 18 John Wilander 2016-10-07 12:34:41 PDT
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.
Comment 19 Chris Dumez 2016-10-07 12:35:14 PDT
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.
Comment 20 Chris Dumez 2016-10-07 14:23:20 PDT
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.
Comment 21 Chris Dumez 2016-10-07 14:54:23 PDT
Created attachment 290971 [details]
Patch
Comment 22 WebKit Commit Bot 2016-10-08 21:19:59 PDT
Comment on attachment 290971 [details]
Patch

Clearing flags on attachment: 290971

Committed r206965: <http://trac.webkit.org/changeset/206965>
Comment 23 WebKit Commit Bot 2016-10-08 21:20:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Michael Catanzaro 2016-10-09 13:26:11 PDT
editing/pasteboard/copy-standalone-image.html is failing on GTK now, see bug #163184.
Comment 25 Radar WebKit Bug Importer 2016-11-09 12:31:14 PST
<rdar://problem/29184049>