Summary: | [CoreIPC] Nullptr crash in IPC::Decoder::decode<WebCore::ResourceError> | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | WebKit2 | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, andersca, bfulgham, cdumez, cgarcia, darin, ddkilzer, ews-feeder, fred.wang, gpoo, product-security, rbuis, sam, svillar, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-02-04 18:07:43 PST
The bug here is in decodeNSError. The problem here is that the hasUnderlyingError support does not work properly when the userInfo dictionary is nullptr. Trivial to fix. I’d be happy to fix this but I don’t know how to make regression tests for an issue like this. I’m going to grab this and fix it, but I don’t plan to add a regression test. Ryosuke, let me know if that’s not right. I read the encode function more carefully and it’s clear it won’t ever encode a null, so the fix here is just to add a null check. But I also did a tiny refactor to get rid of the need to use WARN_UNUSED_RETURN. Since this is just a null pointer dereference, I am thinking that maybe it’s not security sensitive? Created attachment 419459 [details]
Patch
Committed r272450: <https://trac.webkit.org/changeset/272450> (In reply to Darin Adler from comment #4) > Since this is just a null pointer dereference, I am thinking that maybe it’s > not security sensitive? Doesn't seem like it. Maybe we can add a test then. (In reply to Darin Adler from comment #2) > I’m going to grab this and fix it, but I don’t plan to add a regression test. > > Ryosuke, let me know if that’s not right. So if it's just a nullptr crash without any security implications, we want to add a test like any other bug. The only way to test it is the IPC fuzzing; not sure. Hm... this IPC test might be a bit problematic in that it may break right away when the format of WebConnection_HandleMessage changes so maybe it's okay not to add it. (In reply to Darin Adler from comment #9) > The only way to test it is the IPC fuzzing; not sure. FWIW, IPC fuzzing API is available in debug & ASAN release builds so if we added API, we can test it but it's true that this input is probably not going to be valid in a long term. |