Implement navigation rate limiter to prevent recursive navigation crashes#52691
Conversation
|
EWS run on previous version of this PR (hash e87e08f) Details |
e87e08f to
f754826
Compare
|
EWS run on previous version of this PR (hash f754826) Details |
Source/WebCore/page/Navigation.h
Outdated
| @@ -230,6 +232,25 @@ class Navigation final : public RefCounted<Navigation>, public EventTarget, publ | |||
| RefPtr<NavigationAPIMethodTracker> m_upcomingNonTraverseMethodTracker; | |||
| HashMap<String, Ref<NavigationAPIMethodTracker>> m_upcomingTraverseMethodTrackers; | |||
| WeakHashSet<AbortHandler> m_abortHandlers; | |||
|
|
|||
| static constexpr int maxNavigateCalls { 64 }; | |||
There was a problem hiding this comment.
Arbitrary constant for now
There was a problem hiding this comment.
I'm not sure how this number is practical.
Safer C++ Build #60121 (f754826)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
basuke
left a comment
There was a problem hiding this comment.
As we talked offline, we should align the Chrome behavior. But crashing easily like this is not the great situation.
Source/WebCore/page/Navigation.h
Outdated
| @@ -230,6 +232,25 @@ class Navigation final : public RefCounted<Navigation>, public EventTarget, publ | |||
| RefPtr<NavigationAPIMethodTracker> m_upcomingNonTraverseMethodTracker; | |||
| HashMap<String, Ref<NavigationAPIMethodTracker>> m_upcomingTraverseMethodTrackers; | |||
| WeakHashSet<AbortHandler> m_abortHandlers; | |||
|
|
|||
| static constexpr int maxNavigateCalls { 64 }; | |||
There was a problem hiding this comment.
I'm not sure how this number is practical.
f754826 to
7882240
Compare
|
EWS run on previous version of this PR (hash 7882240) Details |
Safer C++ Build #62842 (7882240)❌ Found 2 failing files with 3 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
7882240 to
672f5e4
Compare
|
EWS run on previous version of this PR (hash 672f5e4) Details |
672f5e4 to
5f65d5c
Compare
|
EWS run on previous version of this PR (hash 5f65d5c) Details |
Safer C++ Build #63669 (5f65d5c)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
|
Hey, thanks for looking into this! Have we checked what Chrome does about this problem? Do they also limit the number of calls to a certain number (if so, what number)? Or do they have a different solution? |
5f65d5c to
610f1dc
Compare
|
EWS run on previous version of this PR (hash 610f1dc) Details |
|
@RupinMittal I'm not aware of any explicit cap by Chrome so the number chosen (currently 10) is entirely an implementation detail. On Chrome (using the layout test attached) the behavior is to navigate to about:blank#blocked from what I observed. |
|
I took a look into Chromium and they have a throttler for each frame that limit any sort of navigation 200 times per last 10 seconds. The best thing is to implement similar throttler or limiter. third_party/blink/renderer/core/frame/navigation_rate_limiter.cc |
basuke
left a comment
There was a problem hiding this comment.
As I wrote in the previous message, we should add navigation count limiter, not only for Navigation API.
| result.finished.catch(checkAbortError); | ||
| }); | ||
|
|
||
| const stream = new Blob().stream(); |
There was a problem hiding this comment.
What is this unused stream?
There was a problem hiding this comment.
It came from the original fuzzer test case that was supplied in the radar. Having this present was causing an assertion exception in ReadableStreamDefaultController once the navigation events started stack overflowing.
I'll move it to a separate test to be more explicit.
Source/WebCore/page/Navigation.h
Outdated
| @@ -230,6 +234,23 @@ class Navigation final : public RefCounted<Navigation>, public EventTarget, publ | |||
| RefPtr<NavigationAPIMethodTracker> m_upcomingNonTraverseMethodTracker; | |||
| HashMap<String, Ref<NavigationAPIMethodTracker>> m_upcomingTraverseMethodTrackers; | |||
| WeakHashSet<AbortHandler> m_abortHandlers; | |||
|
|
|||
| static constexpr int maxNavigateCalls { 10 }; | |||
There was a problem hiding this comment.
From the number from Chromium, 200 seems the good number.
Source/WebCore/page/Navigation.h
Outdated
| explicit NavigateCallCounter(Navigation& nav) | ||
| : m_navigation { nav } | ||
| { | ||
| m_navigation->m_navigateCalls++; |
There was a problem hiding this comment.
Just incrementing and no decrement. This logic seems broken.
There was a problem hiding this comment.
The decrement is in destructor in Navigation.cpp. Maybe I'll move the constructor there as well so that they will be next to each other.
610f1dc to
7b556d0
Compare
|
EWS run on previous version of this PR (hash 7b556d0) Details |
Safer C++ Build #64975 (7b556d0)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
7b556d0 to
c25adfa
Compare
|
EWS run on previous version of this PR (hash c25adfa) Details
|
c25adfa to
b0233a1
Compare
|
EWS run on previous version of this PR (hash b0233a1) Details |
Source/WebCore/page/Navigation.h
Outdated
| private: | ||
| // Sliding window rate limiter: allows 80 navigations per 4 second window (~20/sec sustained). | ||
| // Chromium uses 200 navigations per 10 seconds. Both prevent IPC flooding and stack overflow | ||
| // from recursive navigation patterns. |
There was a problem hiding this comment.
Why did you change the number from chromium? Didn't that prevent the crash? Also please add the url of chromium project that is the reason of this decision.
Source/WebCore/page/Navigation.h
Outdated
| @@ -195,6 +197,28 @@ class Navigation final : public RefCounted<Navigation>, public EventTarget, publ | |||
| static Vector<Ref<HistoryItem>> filterHistoryItemsForNavigationAPI(Vector<Ref<HistoryItem>>&&, HistoryItem&); | |||
|
|
|||
| private: | |||
| // Rate limiter to prevent excessive navigation requests. | |||
| class NavigationRateLimiter { | |||
There was a problem hiding this comment.
Please add one of the allocation macro. Read FastMalloc.h for the detail.
There was a problem hiding this comment.
Most classes should use WTF_MAKE_TZONE_ALLOCATED(ClassName).
Source/WebCore/page/Navigation.h
Outdated
| class NavigationRateLimiter { | ||
| public: | ||
| NavigationRateLimiter() = default; | ||
| NavigationRateLimiter(const Navigation&) = delete; |
There was a problem hiding this comment.
Use WTF_MAKE_NONCOPYABLE and WTF_MAKE_NONMOVABLE macros.
There was a problem hiding this comment.
These Navigation should be this class name, right? That's one of the reason we use macro.
Source/WebCore/page/Navigation.h
Outdated
| @@ -195,6 +197,28 @@ class Navigation final : public RefCounted<Navigation>, public EventTarget, publ | |||
| static Vector<Ref<HistoryItem>> filterHistoryItemsForNavigationAPI(Vector<Ref<HistoryItem>>&&, HistoryItem&); | |||
|
|
|||
| private: | |||
| // Rate limiter to prevent excessive navigation requests. | |||
| class NavigationRateLimiter { | |||
There was a problem hiding this comment.
The class name can be simply RateLimiter if you define this class as internal class.
LayoutTests/TestExpectations
Outdated
| @@ -8157,3 +8157,6 @@ imported/w3c/web-platform-tests/css/CSS2/visudet/replaced-elements-min-width-80. | |||
|
|
|||
| imported/w3c/web-platform-tests/css/CSS2/stacking-context/opacity-affects-block-in-inline.html [ ImageOnlyFailure ] | |||
| imported/w3c/web-platform-tests/css/CSS2/stacking-context/zindex-affects-block-in-inline.html [ ImageOnlyFailure ] | |||
|
|
|||
| # Navigation API rate limiting window reset test takes >10 seconds | |||
| navigation-api/navigation-api-rate-limit-window-reset.html [ Slow ] | |||
There was a problem hiding this comment.
We don't like this kind of slow test. We can add Internal method to change those numbers for testing and also force resetting the limiter.
Source/WebCore/page/Navigation.cpp
Outdated
| @@ -1267,4 +1271,32 @@ Vector<Ref<HistoryItem>> Navigation::filterHistoryItemsForNavigationAPI(Vector<R | |||
| return filteredItems; | |||
| } | |||
|
|
|||
| bool Navigation::NavigationRateLimiter::navigationAllowed(Navigation& navigation) | |||
There was a problem hiding this comment.
This argument only used to get Document from the navigation.window(). It's better to pass Document to the constructor of the RateLimiter.
Source/WebCore/page/Navigation.cpp
Outdated
| @@ -400,6 +400,10 @@ Navigation::Result Navigation::reload(ReloadOptions&& options, Ref<DeferredPromi | |||
| // https://html.spec.whatwg.org/multipage/nav-history-apis.html#dom-navigation-navigate | |||
| Navigation::Result Navigation::navigate(const String& url, NavigateOptions&& options, Ref<DeferredPromise>&& committed, Ref<DeferredPromise>&& finished) | |||
| { | |||
| // Enforce rate limiting to prevent excessive navigation requests. | |||
| if (RefPtr navigation = this; navigation && !m_rateLimiter.navigationAllowed(*navigation)) | |||
There was a problem hiding this comment.
It seems weird. We can simply check !m_rateLimiter.navigationAllowed() if we don't pass Navigation. More over, moving back the duty to send message to console from RateLimiter to this class, the limiter just don't care about reference counted objects. Let's move them here and add the method wasReported() and markReported() to manage that.
There was a problem hiding this comment.
Actually this is not the only place that invokes NavigateEvent. i.e. history.push(), setting location.href, etc. Look at innerDispatchNavigateEvent() that all invocation of NavigateEvent uses.
b0233a1 to
3c2fd15
Compare
|
EWS run on previous version of this PR (hash 3c2fd15) Details |
Safer C++ Build #65620 (3c2fd15)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
…shes https://bugs.webkit.org/show_bug.cgi?id=301144 rdar://161094454 Reviewed by NOBODY (OOPS!). Recursive navigation.navigate() calls from event listeners can trigger stack overflow. During resource cleanup for a ReadableStream, this causes an exception assertion in ReadableStreamDefaultController. However, clearing the exception locally simply reveals more downstream assertions, pointing to a need to prevent stack overflow from occuring instead. Add a sliding window rate limiter (100 navigations per 5 second window) to prevent IPC flooding and stack overflow from recursive calls. This matches Chromium's sustained rate (~20 navigations/second). Tests: navigation-api/navigation-api-rate-limit-exceeded.html navigation-api/navigation-api-rate-limit-readable-stream-no-crash.html navigation-api/navigation-api-rate-limit-window-reset.html * LayoutTests/navigation-api/navigation-api-rate-limit-exceeded-expected.txt: Added. * LayoutTests/navigation-api/navigation-api-rate-limit-exceeded.html: Added. * LayoutTests/navigation-api/navigation-api-rate-limit-readable-stream-no-crash-expected.txt: Added. * LayoutTests/navigation-api/navigation-api-rate-limit-readable-stream-no-crash.html: Added. * LayoutTests/navigation-api/navigation-api-rate-limit-window-reset-expected.txt: Added. * LayoutTests/navigation-api/navigation-api-rate-limit-window-reset.html: Added. * Source/WebCore/page/LocalDOMWindow.h: * Source/WebCore/page/Navigation.cpp: (WebCore::Navigation::innerDispatchNavigateEvent): (WebCore::Navigation::RateLimiter::navigationAllowed): * Source/WebCore/page/Navigation.h: * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::setNavigationRateLimiterParameters): (WebCore::Internals::resetNavigationRateLimiter): * Source/WebCore/testing/Internals.h: * Source/WebCore/testing/Internals.idl:
3c2fd15 to
b97666d
Compare
|
EWS run on current version of this PR (hash b97666d) Details |
|
Closing PR as it is associated with a fuzzer bug that is now resolved. |
🛠 ios-apple
b97666d
b97666d