Bug 178060 – Sanitize URL in pasteboard for other applications and cross origin content

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

When putting URL into pasteboard, we should sanitize it for non-web contents and cross origin websites.
Otherwise, we run the risk of letting web contents expositing vulnerabilities in URL parsers in other applications.

[Ryosuke Niwa]

Created attachment 323120 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/34874518>

[Ryosuke Niwa]

Created attachment 323123 [details]
WIP2

[Ryosuke Niwa]

This includes the patch for https://webkit.org/b/178056.

[Ryosuke Niwa]

Created attachment 323141 [details]
WIP3

[Build Bot]

Comment on attachment 323141 [details]
WIP3

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

New failing tests:
editing/pasteboard/data-transfer-get-data-on-drop-custom.html

[Build Bot]

Created attachment 323146 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Ryosuke Niwa]

Created attachment 323147 [details]
WIP4

[Build Bot]

Comment on attachment 323147 [details]
WIP4

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

New failing tests:
http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html
editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in-null-origin.html
editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null-origin.html

[Build Bot]

Created attachment 323149 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Ryosuke Niwa]

Hm... the assertion failure here seems unrelated:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000106e67677 WTFCrash + 39 (Assertions.cpp:270)
1   com.apple.WebCore             	0x0000000110f18c89 WebCore::RenderBlockFlow::ensureLineBoxes() + 761 (RenderBlockFlow.cpp:3661)
2   com.apple.WebCore             	0x00000001111f2b90 WebCore::RenderText::ensureLineBoxes() + 80 (RenderText.cpp:1347)
3   com.apple.WebCore             	0x0000000110e4e0aa WebCore::ensureLineBoxesIfNeeded(WebCore::RenderObject&) + 90 (Position.cpp:641)
4   com.apple.WebCore             	0x0000000110e4c200 WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const + 656 (Position.cpp:825)
5   com.apple.WebCore             	0x000000010f454026 WebCore::previousVisuallyDistinctCandidate(WebCore::Position const&) + 182 (Editing.cpp:273)
6   com.apple.WebCore             	0x000000011196ed23 WebCore::VisiblePosition::previous(WebCore::EditingBoundaryCrossingRule, bool*) const + 147 (VisiblePosition.cpp:88)
7   com.apple.WebCore             	0x000000011197c0a0 WebCore::isStartOfDocument(WebCore::VisiblePosition const&) + 64 (VisibleUnits.cpp:1485)
8   com.apple.WebCore             	0x000000010f6e2e66 WebCore::FrameSelection::selectFrameElementInParentIfFullySelected() + 166 (FrameSelection.cpp:1872)
9   com.apple.WebCore             	0x000000010f6e2a40 WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 1232 (FrameSelection.cpp:341)
10  com.apple.WebCore             	0x000000010f6e1184 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 84 (FrameSelection.cpp:350)
11  com.apple.WebCore             	0x000000010f6eba69 WebCore::FrameSelection::selectAll() + 1161 (FrameSelection.cpp:1950)
12  com.apple.WebCore             	0x000000010f48abe8 WebCore::executeSelectAll(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 40 (EditorCommand.cpp:980)
13  com.apple.WebCore             	0x000000010f4867f9 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 217 (EditorCommand.cpp:1775)
14  com.apple.WebCore             	0x000000010f2c3ae9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 89 (Document.cpp:5016)
15  com.apple.WebCore             	0x000000010fed16b1 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&) + 689 (JSDocument.cpp:4893)
16  com.apple.WebCore             	0x000000010fec1697 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) + 615 (JSDOMOperation.h:53)
17  com.apple.WebCore             	0x000000010fea9cec WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 28 (JSDocument.cpp:4898)
18  ???                           	0x000048ef8d201028 0 + 80193702072360
19  com.apple.JavaScriptCore      	0x0000000105aaf061 llint_entry + 31259
20  com.apple.JavaScriptCore      	0x0000000105aaf061 llint_entry + 31259
21  com.apple.JavaScriptCore      	0x0000000105aa7427 vmEntryToJavaScript + 343

[Ryosuke Niwa]

Created attachment 323263 [details]
Implements the feature

[Build Bot]

Comment on attachment 323263 [details]
Implements the feature

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

New failing tests:
http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html
editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in-null-origin.html
editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null-origin.html

[Build Bot]

Created attachment 323281 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Ryosuke Niwa]

(In reply to Build Bot from comment #13)
> Comment on attachment 323263 [details]
> Implements the feature
> 
> Attachment 323263 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4807984
> 
> New failing tests:
> http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html
> editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in-
> null-origin.html
> editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null-
> origin.html

This is very mysterious... I can't reproduce these assertion failures at all on my local machine :(

[Ryosuke Niwa]

Created attachment 323285 [details]
Worked around the assertion failure in El Capitan

[Ryosuke Niwa]

The unrelated assertion failure I encountered is now tracked in https://bugs.webkit.org/show_bug.cgi?id=178116.

[Ryosuke Niwa]

Ping reviewers.

[Wenson Hsieh]

Comment on attachment 323285 [details]
Worked around the assertion failure in El Capitan

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

> Source/WebCore/ChangeLog:51
> +        Store the original value if the sanitization resuled in any difference.

resuled => resulted

> Source/WebCore/ChangeLog:55
> +        (WebCore::DataTransfer::createForDragStartEvent): Added Document as an arugment to compute the origin string.

arugment => argument

> Source/WebKit/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Perhaps a sentence here saying that you're plumbing an `origin` through pasteboard codepaths in the client layer?

> Source/WebCore/dom/DataTransfer.cpp:81
> +static String originForDocument(Document& document)

In the null origin case, the String we pass back isn't an origin at all, so I think the name originForDocument is slightly misleading. Maybe originIdentifierForDocument?

> Source/WebCore/dom/DataTransfer.cpp:162
> +    if (is<StaticPasteboard>(*m_pasteboard))

Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote).

> Source/WebCore/dom/DataTransfer.cpp:213
> +        if (url.isValid() && !url.isEmpty())

Can an empty URL be valid? If not, we can just shorten this to url.isValid().

> Source/WebCore/dom/Document.cpp:5314
> +        // happen to be identical within 1 miroseconds of one another for two documents with an identical pointer value.

miroseconds => microseconds

> Tools/TestWebKitAPI/Tests/WebKitCocoa/copy-url.html:20
> +editor.focus(); document.execCommand('selctAll');

"selectAll". Though, it looks like this isn't necessary anyways for the test, given that it passes anyways, since the copy event must be fired even if we don't have a range selection. I suppose focus()ing ensures that the selection is at the very least a caret, and not none, so we don't bail early in the copy?

[Ryosuke Niwa]

Comment on attachment 323285 [details]
Worked around the assertion failure in El Capitan

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

>> Source/WebCore/ChangeLog:51
>> +        Store the original value if the sanitization resuled in any difference.
> 
> resuled => resulted

Will fix.

>> Source/WebCore/ChangeLog:55
>> +        (WebCore::DataTransfer::createForDragStartEvent): Added Document as an arugment to compute the origin string.
> 
> arugment => argument

Will fix.

>> Source/WebKit/ChangeLog:7
>> +        Reviewed by NOBODY (OOPS!).
> 
> Perhaps a sentence here saying that you're plumbing an `origin` through pasteboard codepaths in the client layer?

Sure, will add.

>> Source/WebCore/dom/DataTransfer.cpp:81
>> +static String originForDocument(Document& document)
> 
> In the null origin case, the String we pass back isn't an origin at all, so I think the name originForDocument is slightly misleading. Maybe originIdentifierForDocument?

That's a good point. originIdentifierForDocument will do.

>> Source/WebCore/dom/DataTransfer.cpp:162
>> +    if (is<StaticPasteboard>(*m_pasteboard))
> 
> Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote).

Sure, will add a comment.

is<StaticPasteboard>() is the preferred method of checking types in WebKit
because we may decide to start RTTI instead of these isX() methods.

>> Source/WebCore/dom/DataTransfer.cpp:213
>> +        if (url.isValid() && !url.isEmpty())
> 
> Can an empty URL be valid? If not, we can just shorten this to url.isValid().

Oh, that's a good point. We probably don't need url.isEmpty() here.
I think I added this before I had "sanitizedData != data" check below for some reason.

>> Source/WebCore/dom/Document.cpp:5314
>> +        // happen to be identical within 1 miroseconds of one another for two documents with an identical pointer value.
> 
> miroseconds => microseconds

Will fix.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/copy-url.html:20
>> +editor.focus(); document.execCommand('selctAll');
> 
> "selectAll". Though, it looks like this isn't necessary anyways for the test, given that it passes anyways, since the copy event must be fired even if we don't have a range selection. I suppose focus()ing ensures that the selection is at the very least a caret, and not none, so we don't bail early in the copy?

Yeah, I guess so. Will remove.

[Wenson Hsieh]

Comment on attachment 323285 [details]
Worked around the assertion failure in El Capitan

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

> Source/WebCore/ChangeLog:15
> +        the origin of a originating document into our custom pasteboard data. Note that we expose any URL which

...of "an" originating document...

> Source/WebCore/ChangeLog:19
> +        and a milisecond precision timestamp, and the memory address of the document. For two null-origin documents

milisecond => millisecond

> Source/WebCore/ChangeLog:30
> +        to diverse between null origin and non-null origin documents.

diverse => diverge?

> Source/WebCore/ChangeLog:95
> +        off the team data. Don't expose custom types that written by cross origin documents.

"that written" => "that are written"

> Source/WebCore/ChangeLog:96
> +        (WebCore::PlatformPasteboard::write): Add the orign string with custom pasteboard types in the team data.

"orign" => "origin"

> Source/WebCore/dom/DataTransfer.cpp:218
> +    if (sanitizedData != data)

Interesting. So if we're writing a value for "text/uri-list" and the URLParser spits out a slightly different URL than what was supplied, then we'll write "text/uri-list" into both the custom data blob, as well as the platform pasteboard? It seems like we'd only need to put it in the platform pasteboard, but perhaps I'm missing something here...

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:540
> +        if (![plist isKindOfClass:[NSArray class]] || [(NSArray *)plist count] < 2)

We cast (NSArray *)plist a lot here :P can we make a local NSArray * variable instead, after checking -isKindOfClass:?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CopyURL.mm:67
> +    EXPECT_WK_STREQ(@"1", [webView stringByEvaluatingJavaScript:@"window.didCopy"]);

I'd prefer using -boolValue and EXPECT_TRUE here instead of checking for the string "1", but up to you.

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-copying-pasting-malformed-url-in-same-document-expected.txt:8
> +FAIL JSON.stringify(Array.from(event.clipboardData.items).map((item) => {kind: item.kind, type: item.type})) should be [{"kind":"string","type":"text/uri-list"}]. Threw exception SyntaxError: Unexpected token ':'

Is this test case intended to throw a syntax error?

> LayoutTests/http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html:35
> +setTimeout(() => finishJSTest(), 3000);

This can just be setTimeout(finishJSTest, 3000)

...speaking of which, do we know exactly what we're waiting on here? I wonder if we can do this in a way that avoids a hard timeout period :P I've had bad experiences with tests

[Wenson Hsieh]

> ...speaking of which, do we know exactly what we're waiting on here? I
> wonder if we can do this in a way that avoids a hard timeout period :P I've
> had bad experiences with tests

...didn't quite finish that thought. [...] bad experience with tests that try to wait for a set amount of time.

[Chris Dumez]

Comment on attachment 323285 [details]
Worked around the assertion failure in El Capitan

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

A few drive-by comments for now.

> Source/WebCore/ChangeLog:13
> +        Because the Web compatbility requires that DataTransfer exposes the original URL to any document in the

typo: compatbility

>>> Source/WebCore/dom/DataTransfer.cpp:162
>>> +    if (is<StaticPasteboard>(*m_pasteboard))
>> 
>> Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote).
> 
> Sure, will add a comment.
> 
> is<StaticPasteboard>() is the preferred method of checking types in WebKit
> because we may decide to start RTTI instead of these isX() methods.

I agree that using is<>() is our preferred pattern.

> Source/WebCore/dom/DataTransfer.cpp:212
> +        auto url = URLParser(data).result();

So we need to worry about username / password in URLs?

> Source/WebCore/dom/Document.cpp:5310
> +String Document::uniqueIdentifier()

Can we use createCanonicalUUIDString() ?

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #21)
> Comment on attachment 323285 [details]
> Worked around the assertion failure in El Capitan
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323285&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        the origin of a originating document into our custom pasteboard data. Note that we expose any URL which
> 
> ...of "an" originating document...

Fixed.

> > Source/WebCore/ChangeLog:19
> > +        and a milisecond precision timestamp, and the memory address of the document. For two null-origin documents
> 
> milisecond => millisecond

Fixed.

> > Source/WebCore/ChangeLog:30
> > +        to diverse between null origin and non-null origin documents.
> 
> diverse => diverge?

Fixed.

> > Source/WebCore/ChangeLog:95
> > +        off the team data. Don't expose custom types that written by cross origin documents.
> 
> "that written" => "that are written"

Fixed.

> > Source/WebCore/ChangeLog:96
> > +        (WebCore::PlatformPasteboard::write): Add the orign string with custom pasteboard types in the team data.
> 
> "orign" => "origin"

Fixed.

> > Source/WebCore/dom/DataTransfer.cpp:218
> > +    if (sanitizedData != data)
> 
> Interesting. So if we're writing a value for "text/uri-list" and the
> URLParser spits out a slightly different URL than what was supplied, then
> we'll write "text/uri-list" into both the custom data blob, as well as the
> platform pasteboard? It seems like we'd only need to put it in the platform
> pasteboard, but perhaps I'm missing something here...

We'd need the original URL for the same origin content whereas the native type will be read by other applications and cross origin content.

> > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:540
> > +        if (![plist isKindOfClass:[NSArray class]] || [(NSArray *)plist count] < 2)
> 
> We cast (NSArray *)plist a lot here :P can we make a local NSArray *
> variable instead, after checking -isKindOfClass:?

Will fix.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/CopyURL.mm:67
> > +    EXPECT_WK_STREQ(@"1", [webView stringByEvaluatingJavaScript:@"window.didCopy"]);
> 
> I'd prefer using -boolValue and EXPECT_TRUE here instead of checking for the
> string "1", but up to you.

Will fix.

> > LayoutTests/editing/pasteboard/data-transfer-get-data-on-copying-pasting-malformed-url-in-same-document-expected.txt:8
> > +FAIL JSON.stringify(Array.from(event.clipboardData.items).map((item) => {kind: item.kind, type: item.type})) should be [{"kind":"string","type":"text/uri-list"}]. Threw exception SyntaxError: Unexpected token ':'
> 
> Is this test case intended to throw a syntax error?

Oops, clearly not. Will fix.

> > LayoutTests/http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html:35
> > +setTimeout(() => finishJSTest(), 3000);
> 
> This can just be setTimeout(finishJSTest, 3000)
> 
> ...speaking of which, do we know exactly what we're waiting on here? I
> wonder if we can do this in a way that avoids a hard timeout period :P I've
> had bad experiences with tests

Oops, this was a debugging code. Will remove.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #23)
> Comment on attachment 323285 [details]
> Worked around the assertion failure in El Capitan
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323285&action=review
> 
> A few drive-by comments for now.
> 
> > Source/WebCore/ChangeLog:13
> > +        Because the Web compatbility requires that DataTransfer exposes the original URL to any document in the
> 
> typo: compatbility

Will fix.

> > Source/WebCore/dom/DataTransfer.cpp:212
> > +        auto url = URLParser(data).result();
> 
> So we need to worry about username / password in URLs?

I thought we should but then noticed that this is the case of the user copying URL from the web content so I don't think we really need to strip them away.

When the user is pasting URL into the web content, the user will know/see that the URL contains username & password, so again, we probably don't need to strip them away.

> > Source/WebCore/dom/Document.cpp:5310
> > +String Document::uniqueIdentifier()
> 
> Can we use createCanonicalUUIDString() ?

Will do.

[Ryosuke Niwa]

Created attachment 323390 [details]
Addressed review comments

[Ryosuke Niwa]

Created attachment 323391 [details]
Addressed review comments

[Ryosuke Niwa]

Created attachment 323393 [details]
Rebaseline one test in WK1

[Wenson Hsieh]

Comment on attachment 323393 [details]
Rebaseline one test in WK1

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

> Source/WebCore/dom/DataTransfer.h:128
> +    String m_origin;

Maybe m_originIdentifier would be a more accurate name for this member?

[Ryosuke Niwa]

Committed r223195: <https://trac.webkit.org/changeset/223195>