Summary: | [CoreIPC] null-ptr in WebCore::TextIndicatorWindow::setTextIndicator | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Rob Buis <rbuis> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, simon.fraser, svillar, thorton, webkit-bug-importer, wenson_hsieh, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-01-08 18:01:11 PST
Created attachment 417323 [details]
Test (requires WTR in macOS)
Created attachment 417610 [details]
Patch
This does not seem like a security problem to me. I do wonder where a test would go? (In reply to Rob Buis from comment #3) > This does not seem like a security problem to me. I do wonder where a test > would go? LayoutTests/ipc probably. Created attachment 417685 [details]
Patch
Created attachment 417700 [details]
Patch
(In reply to Ryosuke Niwa from comment #4) > (In reply to Rob Buis from comment #3) > > This does not seem like a security problem to me. I do wonder where a test > > would go? > > LayoutTests/ipc probably. It seems this will only work for ASAN/Debug builds. Instead I added a unit test. I've had success making similar layout tests like this: <!DOCTYPE html><!-- webkit-test-runner [ IPCTestingAPIEnabled=true ] --> <html> <body> <script> if (window.testRunner) testRunner.dumpAsText(); if (window.IPC) IPC.sendMessage(...); </script> This test passes if it does not crash. </body> </html> It has the same output in debug and release. It only did anything in the debug/asan builds, but it's better than nothing, and I think it's more sustainable than adding API tests for this. (In reply to Alex Christensen from comment #8) > I've had success making similar layout tests like this: > > <!DOCTYPE html><!-- webkit-test-runner [ IPCTestingAPIEnabled=true ] --> > <html> > <body> > <script> > if (window.testRunner) > testRunner.dumpAsText(); > if (window.IPC) > IPC.sendMessage(...); > </script> > This test passes if it does not crash. > </body> > </html> > > It has the same output in debug and release. It only did anything in the > debug/asan builds, but it's better than nothing, and I think it's more > sustainable than adding API tests for this. Thanks, looks like I missed the if (window.IPC) part, will redo my patch. Created attachment 417733 [details]
Patch
Comment on attachment 417733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417733&action=review > Source/WebCore/page/mac/TextIndicatorWindow.mm:164 > + if (_textIndicator->contentImage()) { if (auto* image = _textIndicator->contentImage()) { ... } Also, is there a possibility that we have contentImage() which is null, but either contentImageWithHighlight or contentImageWithoutSelection which are not null? Created attachment 417873 [details]
Patch
Comment on attachment 417733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417733&action=review >> Source/WebCore/page/mac/TextIndicatorWindow.mm:164 >> + if (_textIndicator->contentImage()) { > > if (auto* image = _textIndicator->contentImage()) { > ... > } > Also, is there a possibility that we have contentImage() which is null, but either contentImageWithHighlight or contentImageWithoutSelection which are not null? Done. The test IPC can be so formed AFAIK that all three can be null. So I null checked contentImageWithHighlight in this same method, but it looks like contentImageWithoutSelection is not exposed with a getter. Comment on attachment 417873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417873&action=review > LayoutTests/ChangeLog:8 > + Add test for this. Might not be needed. > LayoutTests/ipc/set-text-indicator.html:8 > + IPC.sendMessage('UI', IPC.webPageProxyID, IPC.messages.WebPageProxy_SetTextIndicator.name, [{type: 'float', value: 598}, {type: 'float', value: 352}, {type: 'float', value: 597}, {type: 'float', value: 947.9962715781635}, {type: 'float', value: 0}, {type: 'float', value: 942}, {type: 'float', value: 233.5502697674895}, {type: 'float', value: 3103.98319661806}, {type: 'Vector', value: [[{type: 'float', value: 262}, {type: 'float', value: 1691.8218636988486}, {type: 'float', value: 390}, {type: 'float', value: 3255.5734463359736}], [{type: 'float', value: 245}, {type: 'float', value: 2805.6525082537914}, {type: 'float', value: 1587.8585914997557}, {type: 'float', value: 205}], [{type: 'float', value: 668}, {type: 'float', value: 1930.6632966420898}, {type: 'float', value: 286.4821983543896}, {type: 'float', value: 107}], [{type: 'float', value: 210}, {type: 'float', value: 21.44449100848898}, {type: 'float', value: 252.77757277214582}, {type: 'float', value: 736.6757847636829}], [{type: 'float', value: 1380.3264570234599}, {type: 'float', value: 387.09616515322466}, {type: 'float', value: 316}, {type: 'float', value: 366}], [{type: 'float', value: 1321.8796914377176}, {type: 'float', value: 1464.9902412682602}, {type: 'float', value: 327}, {type: 'float', value: 569.3400524583093}], [{type: 'float', value: 644.0885995610852}, {type: 'float', value: 1953.1505093058456}, {type: 'float', value: 511.1745496780984}, {type: 'float', value: 42}], [{type: 'float', value: 168}, {type: 'float', value: 617}, {type: 'float', value: 964.3373436967936}, {type: 'float', value: 756}], [{type: 'float', value: 385.12263561140804}, {type: 'float', value: 167.97526671216778}, {type: 'float', value: 154}, {type: 'float', value: 533}], [{type: 'float', value: 198.85771539870234}, {type: 'float', value: 315.55225980593696}, {type: 'float', value: 864}, {type: 'float', value: 1164.3220920210106}], [{type: 'float', value: 1402.4797727647326}, {type: 'float', value: 491}, {type: 'float', value: 482}, {type: 'float', value: 224}], [{type: 'float', value: 269.17031456719684}, {type: 'float', value: 544}, {type: 'float', value: 602}, {type: 'float', value: 287.3884180705148}], [{type: 'float', value: 179}, {type: 'float', value: 251.6630350908998}, {type: 'float', value: 149}, {type: 'float', value: 489}]]}, {type: 'float', value: 927}, {type: 'float', value: 705}, {type: 'float', value: 498}, {type: 'float', value: 830}, {type: 'float', value: 58}, {type: 'bool', value: 1}, {type: 'float', value: 341}, {type: 'float', value: 1203.8009453924515}, {type: 'float', value: 1291.9587067321006}, {type: 'float', value: 1073.9720902903282}, {type: 'uint8_t', value: 2}, {type: 'uint8_t', value: 2}, {type: 'uint16_t', value: 12367}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'uint64_t', value: 641}]); How does this work exactly? If we change TextIndicatorData, will the test start to fail. Or will it be simply rejected as IPC decoding will fail? I am guessing there is no way to add an Internals API that would create a carefully crafted TextIndicatorData to repro this issue? Can the test be reduced a bit? Created attachment 418041 [details]
Patch
Comment on attachment 417873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417873&action=review >> LayoutTests/ipc/set-text-indicator.html:8 >> + IPC.sendMessage('UI', IPC.webPageProxyID, IPC.messages.WebPageProxy_SetTextIndicator.name, [{type: 'float', value: 598}, {type: 'float', value: 352}, {type: 'float', value: 597}, {type: 'float', value: 947.9962715781635}, {type: 'float', value: 0}, {type: 'float', value: 942}, {type: 'float', value: 233.5502697674895}, {type: 'float', value: 3103.98319661806}, {type: 'Vector', value: [[{type: 'float', value: 262}, {type: 'float', value: 1691.8218636988486}, {type: 'float', value: 390}, {type: 'float', value: 3255.5734463359736}], [{type: 'float', value: 245}, {type: 'float', value: 2805.6525082537914}, {type: 'float', value: 1587.8585914997557}, {type: 'float', value: 205}], [{type: 'float', value: 668}, {type: 'float', value: 1930.6632966420898}, {type: 'float', value: 286.4821983543896}, {type: 'float', value: 107}], [{type: 'float', value: 210}, {type: 'float', value: 21.44449100848898}, {type: 'float', value: 252.77757277214582}, {type: 'float', value: 736.6757847636829}], [{type: 'float', value: 1380.3264570234599}, {type: 'float', value: 387.09616515322466}, {type: 'float', value: 316}, {type: 'float', value: 366}], [{type: 'float', value: 1321.8796914377176}, {type: 'float', value: 1464.9902412682602}, {type: 'float', value: 327}, {type: 'float', value: 569.3400524583093}], [{type: 'float', value: 644.0885995610852}, {type: 'float', value: 1953.1505093058456}, {type: 'float', value: 511.1745496780984}, {type: 'float', value: 42}], [{type: 'float', value: 168}, {type: 'float', value: 617}, {type: 'float', value: 964.3373436967936}, {type: 'float', value: 756}], [{type: 'float', value: 385.12263561140804}, {type: 'float', value: 167.97526671216778}, {type: 'float', value: 154}, {type: 'float', value: 533}], [{type: 'float', value: 198.85771539870234}, {type: 'float', value: 315.55225980593696}, {type: 'float', value: 864}, {type: 'float', value: 1164.3220920210106}], [{type: 'float', value: 1402.4797727647326}, {type: 'float', value: 491}, {type: 'float', value: 482}, {type: 'float', value: 224}], [{type: 'float', value: 269.17031456719684}, {type: 'float', value: 544}, {type: 'float', value: 602}, {type: 'float', value: 287.3884180705148}], [{type: 'float', value: 179}, {type: 'float', value: 251.6630350908998}, {type: 'float', value: 149}, {type: 'float', value: 489}]]}, {type: 'float', value: 927}, {type: 'float', value: 705}, {type: 'float', value: 498}, {type: 'float', value: 830}, {type: 'float', value: 58}, {type: 'bool', value: 1}, {type: 'float', value: 341}, {type: 'float', value: 1203.8009453924515}, {type: 'float', value: 1291.9587067321006}, {type: 'float', value: 1073.9720902903282}, {type: 'uint8_t', value: 2}, {type: 'uint8_t', value: 2}, {type: 'uint16_t', value: 12367}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'bool', value: 0}, {type: 'uint64_t', value: 641}]); > > How does this work exactly? If we change TextIndicatorData, will the test start to fail. Or will it be simply rejected as IPC decoding will fail? > I am guessing there is no way to add an Internals API that would create a carefully crafted TextIndicatorData to repro this issue? > Can the test be reduced a bit? Yes, the Vector is now reduced and less precision is needed, but I think the remaining fields are still mandatory. Committed r271694: <https://trac.webkit.org/changeset/271694> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418041 [details]. Hmm, I don't quite understand how you addressed Youenn's concern. The minute someone changes the layout of TextIndicatorData (which is totally not unusual) this test is going to become completely useless (won't crash, but because the decode fails). (In reply to Tim Horton from comment #18) > Hmm, I don't quite understand how you addressed Youenn's concern. The minute > someone changes the layout of TextIndicatorData (which is totally not > unusual) this test is going to become completely useless (won't crash, but > because the decode fails). I don't think there is really a way around it. (In reply to Ryosuke Niwa from comment #19) > (In reply to Tim Horton from comment #18) > > Hmm, I don't quite understand how you addressed Youenn's concern. The minute > > someone changes the layout of TextIndicatorData (which is totally not > > unusual) this test is going to become completely useless (won't crash, but > > because the decode fails). > > I don't think there is really a way around it. Not really sure what the point of the test is, then. (in that, it will promptly become useless dead weight) (In reply to Tim Horton from comment #21) > (in that, it will promptly become useless dead weight) True. Maybe we need to have a wider discussion about this. Maybe the right course of action is not to add a test in these cases. |