Bug 180307 - IPC code doesn't understand NSDictionaries with non-NSString keys
Summary: IPC code doesn't understand NSDictionaries with non-NSString keys
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-01 22:50 PST by Myles C. Maxfield
Modified: 2018-09-10 19:56 PDT (History)
7 users (show)

See Also:

Attachments
Needs test (2.01 KB, patch)
2017-12-01 22:50 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (4.50 KB, patch)
2017-12-12 13:58 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.