Bug 235723 - Default to 32 bit refcount for CanMakeCheckedPtr
Summary: Default to 32 bit refcount for CanMakeCheckedPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-27 10:15 PST by Antti Koivisto
Modified: 2022-01-28 00:09 PST (History)
6 users (show)

See Also:


Attachments
Patch (26.32 KB, patch)
2022-01-27 10:29 PST, Antti Koivisto
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

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