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-

Antti Koivisto
Reported 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).
Attachments
Patch (26.32 KB, patch)
2022-01-27 10:29 PST, Antti Koivisto 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Antti Koivisto
Comment 1 2022-01-27 10:16:26 PST
rdar://86602114
Antti Koivisto
Comment 2 2022-01-27 10:29:22 PST
Created attachment 450153 [details] Patch
Ryosuke Niwa
Comment 3 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.
EWS
Comment 4 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].
Antti Koivisto
Comment 5 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.
Ryosuke Niwa
Comment 6 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.
Antti Koivisto
Comment 7 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.
Ryosuke Niwa
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.