Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mitigate crashes under Quirks::advancedPrivacyProtectionSubstituteDataURLForScriptWithFeatures() #21773

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Mitigate crashes under Quirks::advancedPrivacyProtectionSubstituteDataURLForScriptWithFeatures() #21773

merged 1 commit into from
Dec 15, 2023

Conversation

whsieh
Copy link
Member

@whsieh whsieh commented Dec 14, 2023 •
edited by webkit-early-warning-system
Loading

1bfda19

Mitigate crashes under Quirks::advancedPrivacyProtectionSubstituteDataURLForScriptWithFeatures()
https://bugs.webkit.org/show_bug.cgi?id=266380
rdar://118479646

Reviewed by Yusuke Suzuki.

Even after the mitigations in 269984@main, we're still sometimes crashing when attempting to
determine whether or not we should apply hard-coded canvas fingerprinting mitigations when advanced
privacy protections are enabled. From discussing with JSC folks, this seems to be due to the way in
which we're currently trying to walk the stack by traversing `callerFrame()`s:

```
while (!codeBlock) {
    callFrame = callFrame->callerFrame();
    if (!callFrame)
        break;
    codeBlock = callFrame->codeBlock();
}
```

Instead of implementing it this way, the JSC team recommended using `StackVisitor::visit` instead to
walk the stack, which is the de-facto mechanism used to perform similar stack traversals elsewhere
in the codebase. In addition, I'm also rearranging this check, so that we only ever attempt this
relatively more expensive stack walk in the case where the `lastDrawnText`, `canvasWidth` and
`canvasHeight` all match their expected values for the quirk.

* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::advancedPrivacyProtectionSubstituteDataURLForScriptWithFeatures const):

In my manual testing, I found that the source code length on some of the affected sites has been
changed slightly; adjust this quirk to match.

Canonical link: https://commits.webkit.org/272093@main

17a8d58

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
  🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh requested a review from cdumez as a code owner December 14, 2023 00:37
@whsieh whsieh self-assigned this Dec 14, 2023
@whsieh whsieh added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Dec 14, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 14, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 06fea41)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ⏳ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac   🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ⏳ 🧪 mac-wk1 ✅ 🛠 gtk
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios ⏳ 🧪 mac-wk2 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 api-gtk
⏳ 🛠 tv-sim ⏳ 🧪 mac-wk2-stress ⏳ 🛠 jsc-armv7
⏳ 🛠 watch ⏳ 🧪 jsc-armv7-tests
⏳ 🛠 watch-sim

@whsieh whsieh marked this pull request as draft December 14, 2023 00:37
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 14, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 43128a9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh marked this pull request as ready for review December 14, 2023 00:50
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 15, 2023 •
edited
Loading

EWS run on current version of this PR (hash 17a8d58)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
  🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

Constellation
Constellation approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Dec 15, 2023
@whsieh
Copy link
Member Author

whsieh commented Dec 15, 2023

Thanks for the review!

@whsieh
Mitigate crashes under Quirks::advancedPrivacyProtectionSubstituteDat…
1bfda19 
…aURLForScriptWithFeatures()

https://bugs.webkit.org/show_bug.cgi?id=266380
rdar://118479646

Reviewed by Yusuke Suzuki.

Even after the mitigations in 269984@main, we're still sometimes crashing when attempting to
determine whether or not we should apply hard-coded canvas fingerprinting mitigations when advanced
privacy protections are enabled. From discussing with JSC folks, this seems to be due to the way in
which we're currently trying to walk the stack by traversing `callerFrame()`s:

```
while (!codeBlock) {
    callFrame = callFrame->callerFrame();
    if (!callFrame)
        break;
    codeBlock = callFrame->codeBlock();
}
```

Instead of implementing it this way, the JSC team recommended using `StackVisitor::visit` instead to
walk the stack, which is the de-facto mechanism used to perform similar stack traversals elsewhere
in the codebase. In addition, I'm also rearranging this check, so that we only ever attempt this
relatively more expensive stack walk in the case where the `lastDrawnText`, `canvasWidth` and
`canvasHeight` all match their expected values for the quirk.

* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::advancedPrivacyProtectionSubstituteDataURLForScriptWithFeatures const):

In my manual testing, I found that the source code length on some of the affected sites has been
changed slightly; adjust this quirk to match.

Canonical link: https://commits.webkit.org/272093@main
@webkit-commit-queue
Copy link
Collaborator

Committed 272093@main (1bfda19): https://commits.webkit.org/272093@main

Reviewed commits have been landed. Closing PR #21773 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 1bfda19 into WebKit:main Dec 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 15, 2023
@whsieh whsieh deleted the eng/266380 branch December 15, 2023 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@hortont424 hortont424 Awaiting requested review from hortont424

@sysrqb sysrqb Awaiting requested review from sysrqb

@MenloDorian MenloDorian Awaiting requested review from MenloDorian

@charliewolfe charliewolfe Awaiting requested review from charliewolfe

Assignees

@whsieh whsieh

Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
4 participants
@whsieh @webkit-early-warning-system @webkit-commit-queue @Constellation