Summary: | [Mac] Write WebArchive to the pasteboard when copying image in WebKit | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, enrica, manian, mcatanzaro, rniwa, sam, webkit-bug-importer, wilander | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 49141 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2016-10-06 21:33:10 PDT
Created attachment 290892 [details]
Patch
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 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 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 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 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
You should look into why editing/pasteboard/copy-standalone-image.html is failing... Created attachment 290937 [details]
Patch
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".) (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. (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"> alert('Hello!');</s> It does not alert anything. (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"> 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. 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. (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. (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. (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. Ah, got it. Then there should not be an issue. I did not know whether we included the markup from loaded SVGs. 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 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. 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. Created attachment 290971 [details]
Patch
Comment on attachment 290971 [details] Patch Clearing flags on attachment: 290971 Committed r206965: <http://trac.webkit.org/changeset/206965> All reviewed patches have been landed. Closing bug. editing/pasteboard/copy-standalone-image.html is failing on GTK now, see bug #163184. |