Bug 235723

Summary: Default to 32 bit refcount for CanMakeCheckedPtr
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Web Template FrameworkAssignee: Antti Koivisto <koivisto>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226158
Attachments:
Description Flags
Patch ews-feeder: commit-queue-

Description Antti Koivisto 2022-01-27 10:15:21 PST 
It is currently uint16_t for no good reason. We just hit the first bug caused by overrunning it (in iFC).
Comment 1 Antti Koivisto 2022-01-27 10:16:26 PST 
rdar://86602114
Comment 2 Antti Koivisto 2022-01-27 10:29:22 PST 
Created attachment 450153 [details]
Patch
Comment 3 Ryosuke Niwa 2022-01-27 13:36:39 PST 
I went conservative in size but going with 32-bit is quite reasonable given we also do that in RefCounted.
Comment 4 EWS 2022-01-27 13:38:42 PST 
Committed r288700 (246498@main): <https://commits.webkit.org/246498@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450153 [details].
Comment 5 Antti Koivisto 2022-01-27 22:14:37 PST 
(In reply to Ryosuke Niwa from comment #3)
> I went conservative in size but going with 32-bit is quite reasonable given
> we also do that in RefCounted.

An alternative would have been to just remove ASSERT(m_count) from decrement.
Comment 6 Ryosuke Niwa 2022-01-27 23:25:51 PST 
(In reply to Antti Koivisto from comment #5)
> (In reply to Ryosuke Niwa from comment #3)
> > I went conservative in size but going with 32-bit is quite reasonable given
> > we also do that in RefCounted.
> 
> An alternative would have been to just remove ASSERT(m_count) from decrement.

I don't think that would have been quite safe. If m_count had overflowed, then it's also possible for m_count to become 0 every 2^16 decrements and bypass CanMakeCheckedPtrBase::~CanMakeCheckedPtrBase's release assert even when there are outstanding CheckedPtr or CheckedRef to it. If the creation of CanMakeCheckedPtr objects can be influenced by scripts, it's possible for a malicious web page to control how many decrements would happen to m_count to bypass the release assert, thereby triggering UAF down the line.
Comment 7 Antti Koivisto 2022-01-27 23:46:53 PST 
Right, that's why I thought this was a better approach. Overrunning 32 bits by allocating CheckedPtrs is probably not possible in practice.
Comment 8 Ryosuke Niwa 2022-01-28 00:09:04 PST 
(In reply to Antti Koivisto from comment #7)
> Right, that's why I thought this was a better approach. Overrunning 32 bits
> by allocating CheckedPtrs is probably not possible in practice.

Right.