Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(352)

Unified Diff: third_party/WebKit/Source/core/frame/History.cpp

Issue 2972073002: Mitigate the pushState IPC storm DoS. (Closed)
Patch Set: Simpler data model. Thanks dcheng! Created 3 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: third_party/WebKit/Source/core/frame/History.cpp
diff --git a/third_party/WebKit/Source/core/frame/History.cpp b/third_party/WebKit/Source/core/frame/History.cpp
index 30ebcc037c8ef77fc46f2cb63e954175ea56360a..a266652111fb43f00375630354d476c1e8a1118f 100644
--- a/third_party/WebKit/Source/core/frame/History.cpp
+++ b/third_party/WebKit/Source/core/frame/History.cpp
@@ -116,6 +116,26 @@ HistoryScrollRestorationType History::ScrollRestorationInternal() const {
: kScrollRestorationAuto;
}
+// TODO(crbug.com/394296): This is not the long-term fix to IPC flooding that we
+// need. However, it does somewhat mitigate the immediate concern of |pushState|
+// and |replaceState| DoS (assuming the renderer has not been compromised).
+bool History::IsFloodingState(const String& hostname) const {
Łukasz Anforowicz 2017/07/07 21:05:17 |hostname| parameter is unused now and can be remo
palmer 2017/07/07 21:38:17 Done.
+ const int kStateUpdateLimit = 50;
+
+ if (state_flood_guard.count > kStateUpdateLimit) {
+ constexpr auto kStateUpdateLimitResetInterval = TimeDelta::FromSeconds(10);
dcheng 2017/07/07 21:04:09 Nit: static
palmer 2017/07/07 21:38:17 Done.
+ const auto now = TimeTicks::Now();
+ if (now - state_flood_guard.last_updated > kStateUpdateLimitResetInterval) {
+ state_flood_guard.count = 0;
+ state_flood_guard.last_updated = now;
dcheng 2017/07/07 21:04:09 Do we want to allow in this case, since > 10 secon
Łukasz Anforowicz 2017/07/07 21:05:17 TL;DR: Do you need a |return false| statement abov
palmer 2017/07/07 21:38:17 Yep, done.
+ }
+ return true;
+ }
+
+ state_flood_guard.count++;
+ return false;
+}
+
bool History::stateChanged() const {
return last_state_object_requested_ != StateInternal();
}
@@ -216,6 +236,10 @@ void History::StateObjectAdded(PassRefPtr<SerializedScriptValue> data,
!GetFrame()->Loader().GetDocumentLoader())
return;
+ if (IsFloodingState(GetFrame()->GetDocument()->GetSecurityOrigin()->Host())) {
+ return;
+ }
+
KURL full_url = UrlForState(url_string);
if (!CanChangeToUrl(full_url, GetFrame()->GetDocument()->GetSecurityOrigin(),
GetFrame()->GetDocument()->Url())) {

Powered by Google App Engine
This is Rietveld 408576698