Bug 178422

Summary: Don't expose raw HTML in pasteboard to the web content
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, rniwa, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178606
https://bugs.webkit.org/show_bug.cgi?id=178828
Attachments:
Description Flags
WIP
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Implements the feature
none
Fixed the test wenson_hsieh: review+

Description Ryosuke Niwa 2017-10-17 18:49:59 PDT
HTML in the system pasteboard may contain privacy sensitive information such has file URLs.
Use the HTML sanitization mechanism added in https://trac.webkit.org/r223440 to do this.

Also, start using blob URLs in the pasted images.
Comment 1 Ryosuke Niwa 2017-10-17 18:50:51 PDT
Created attachment 324081 [details]
WIP
Comment 2 Build Bot 2017-10-17 18:53:28 PDT
Attachment 324081 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:166:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2017-10-17 19:48:09 PDT
Comment on attachment 324081 [details]
WIP

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

New failing tests:
http/tests/security/clipboard/copy-paste-html-cross-origin-iframe-across-origin.html
Comment 4 Build Bot 2017-10-17 19:48:10 PDT
Created attachment 324087 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-10-17 20:04:31 PDT
Comment on attachment 324081 [details]
WIP

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

New failing tests:
fast/events/ondrop-text-html.html
Comment 6 Build Bot 2017-10-17 20:04:32 PDT
Created attachment 324092 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-10-17 20:18:55 PDT
Comment on attachment 324081 [details]
WIP

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

New failing tests:
fast/events/ondrop-text-html.html
Comment 8 Build Bot 2017-10-17 20:18:57 PDT
Created attachment 324094 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-10-17 20:26:07 PDT
Comment on attachment 324081 [details]
WIP

Attachment 324081 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4894263

New failing tests:
editing/pasteboard/copy-paste-wraps-position-absolute.html
editing/pasteboard/paste-text-013.html
editing/pasteboard/paste-double-nested-blockquote-before-blockquote.html
editing/pasteboard/paste-blockquote-and-paragraph-break.html
editing/pasteboard/paste-list-003.html
editing/pasteboard/paste-text-012.html
editing/pasteboard/paste-list-002.html
editing/pasteboard/paste-text-016.html
editing/pasteboard/paste-text-014.html
editing/pasteboard/paste-text-017.html
editing/pasteboard/copy-paste-inserts-clearing-div.html
editing/pasteboard/block-wrappers-necessary.html
editing/pasteboard/paste-blockquote-before-blockquote.html
editing/pasteboard/copy-paste-converts-fixed.html
editing/pasteboard/paste-table-001.html
editing/pasteboard/4922709.html
editing/pasteboard/pasting-into-p-should-not-nest-p.html
editing/pasteboard/paste-text-005.html
editing/pasteboard/paste-table-003.html
editing/pasteboard/data-transfer-get-data-on-paste-rich-text.html
editing/pasteboard/paste-text-004.html
editing/pasteboard/paste-text-006.html
Comment 10 Build Bot 2017-10-17 20:26:09 PDT
Created attachment 324095 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 11 Ryosuke Niwa 2017-10-17 22:27:00 PDT
<rdar://problem/34567052>
Comment 12 Ryosuke Niwa 2017-10-17 22:47:56 PDT
Comment on attachment 324081 [details]
WIP

Obviously not up for a review.
Comment 13 Ryosuke Niwa 2017-10-17 22:48:40 PDT
Created attachment 324098 [details]
Implements the feature
Comment 14 Ryosuke Niwa 2017-10-17 23:44:18 PDT
Created attachment 324100 [details]
Fixed the test
Comment 15 Wenson Hsieh 2017-10-18 19:27:54 PDT
Comment on attachment 324100 [details]
Fixed the test

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

> Source/WebCore/editing/WebContentReader.cpp:44
>      return frame.document() && frame.document()->originIdentifierForPasteboard() != contentOrigin;

I'm not sure checking frame.document() is needed here, since we just assume document exists (*frame.document()) afterwards in both branches.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:288
> +    [representationsToRegister addData:customData.createSharedBuffer()->createNSData().get() forType:@(PasteboardCustomData::cocoaType())];

Just to make sure — it looks like we don't need to worry about also adding this to teamData here because we only need to know the origin for markup sanitization on drop?

> Source/WebCore/platform/mac/PasteboardWriter.mm:121
> +        [pasteboardItem setData:customData.createSharedBuffer()->createNSData().get() forType:toUTIUnlessAlreadyUTI(String(PasteboardCustomData::cocoaType())).get()];

Do we need to go through toUTIUnlessAlreadyUTI() here? We already know PasteboardCustomData::cocoaType() is a custom type (not one of the declared CoreServices UTIs).

> LayoutTests/http/tests/misc/copy-resolves-urls.html:45
> +    results.appendChild(document.createTextNode(pasteHere.innerHTML.replace(/blob\:http\:\/\/localhost\:8080\/[a-z0-9\-]+/, 'blob:://localhost:8080/...')));

Nit - extra : after the blob: here.

> LayoutTests/http/tests/security/clipboard/drag-drop-html-cross-origin-iframe-in-same-origin.html:24
> +setTimeout(finishJSTest, 3000);

I'm guessing this was just for debugging?
Comment 16 Ryosuke Niwa 2017-10-18 22:44:03 PDT
Thanks for the review.

(In reply to Wenson Hsieh from comment #15)
> Comment on attachment 324100 [details]
> Fixed the test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324100&action=review
> 
> > Source/WebCore/editing/WebContentReader.cpp:44
> >      return frame.document() && frame.document()->originIdentifierForPasteboard() != contentOrigin;
> 
> I'm not sure checking frame.document() is needed here, since we just assume
> document exists (*frame.document()) afterwards in both branches.

Good point. Replaced it with an assertion.
 
> > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:288
> > +    [representationsToRegister addData:customData.createSharedBuffer()->createNSData().get() forType:@(PasteboardCustomData::cocoaType())];
> 
> Just to make sure — it looks like we don't need to worry about also adding
> this to teamData here because we only need to know the origin for markup
> sanitization on drop?

Yes, we don't need to put the type into the team data.

> > Source/WebCore/platform/mac/PasteboardWriter.mm:121
> > +        [pasteboardItem setData:customData.createSharedBuffer()->createNSData().get() forType:toUTIUnlessAlreadyUTI(String(PasteboardCustomData::cocoaType())).get()];
> 
> Do we need to go through toUTIUnlessAlreadyUTI() here? We already know
> PasteboardCustomData::cocoaType() is a custom type (not one of the declared
> CoreServices UTIs).

So LocalPasteboard.mm in DumpRenderTree has an assertion for checking that either UTI is a registered type or a dynamic type, and we have to call toUTIUnlessAlreadyUTI in order to not hit that assertion,

> > LayoutTests/http/tests/misc/copy-resolves-urls.html:45
> > +    results.appendChild(document.createTextNode(pasteHere.innerHTML.replace(/blob\:http\:\/\/localhost\:8080\/[a-z0-9\-]+/, 'blob:://localhost:8080/...')));
> 
> Nit - extra : after the blob: here.

Oops, fixed.

> > LayoutTests/http/tests/security/clipboard/drag-drop-html-cross-origin-iframe-in-same-origin.html:24
> > +setTimeout(finishJSTest, 3000);
> 
> I'm guessing this was just for debugging?

Yup, removed.
Comment 17 Ryosuke Niwa 2017-10-18 22:44:35 PDT
Committed r223678: <https://trac.webkit.org/changeset/223678>