Bug 180307

Summary: IPC code doesn't understand NSDictionaries with non-NSString keys
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: WebKit2Assignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, ap, beidson, bfulgham, commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188008
Attachments:
Description Flags
Needs test
none
Patch none

Description Myles C. Maxfield 2017-12-01 22:50:16 PST
IPC code doesn't understand NSDictionaries with non-NSString keys
Comment 1 Myles C. Maxfield 2017-12-01 22:50:35 PST
Created attachment 328214 [details]
Needs test
Comment 2 Myles C. Maxfield 2017-12-01 22:51:23 PST
<rdar://problem/35812382>
Comment 3 Myles C. Maxfield 2017-12-01 22:52:43 PST
Is there a direct way to test these? Or do I have to use EventSender?
Comment 4 Alexey Proskuryakov 2017-12-02 11:42:29 PST
We usually send WTF objects over CoreIPC. Sending Foundation objects just has too many pitfalls, including correctness, performance and security.

I recommend looking into removing the existing code here, not into extending it.
Comment 5 Brady Eidson 2017-12-02 15:14:08 PST
Without even looking at the patch, I agree with Alexey.

Attempts to serialize Cocoa types are often wrought with problems and are not entirely within our control. Better to use WTF/WebCore/WebKit types when possible
Comment 6 Myles C. Maxfield 2017-12-04 09:26:35 PST
So it's better to convert to WTF types, send across the wire, then convert back to NS types, rather than just fixing a bug in our existing converters?
Comment 7 Alexey Proskuryakov 2017-12-04 10:23:09 PST
Yes. Or better yet, not use NS types in WebCore at all.

The security aspect of it is not just about what you do, but also about what malicious code running in WebContent can do to escape the sandbox. Any deserialization of NS types performed in UI process adds a large attack surface.
Comment 8 Alexey Proskuryakov 2017-12-04 10:30:39 PST
Oh wait, this doesn't actually use Foundation serialization. What I said is mostly irrelevant, I should have studied the patch more closely. Freaked out because of the recent NSSecureCoding fixes.
Comment 9 Myles C. Maxfield 2017-12-12 13:58:34 PST
Created attachment 329151 [details]
Patch
Comment 10 WebKit Commit Bot 2017-12-12 15:11:23 PST
Comment on attachment 329151 [details]
Patch

Clearing flags on attachment: 329151

Committed r225811: <https://trac.webkit.org/changeset/225811>
Comment 11 WebKit Commit Bot 2017-12-12 15:11:25 PST
All reviewed patches have been landed.  Closing bug.