From 0e76aa2dd07244acb1acf7efe58af0665f30caa4 Mon Sep 17 00:00:00 2001 From: Mia Alexiou Date: Thu, 30 Jul 2020 12:52:56 +0100 Subject: [PATCH 1/9] Test killing dos loads --- .../app/browser/BrowserWebViewClient.kt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index bcc61f97f5a0..e6e9493f1008 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -68,9 +68,16 @@ class BrowserWebViewClient( * API-agnostic implementation of deciding whether to override url or not */ private fun shouldOverride(webView: WebView, url: Uri): Boolean { - try { - Timber.v("shouldOverride $url") + Timber.v("shouldOverride $url") + + // Also validate on API 24 + if (urlIsGeneratingDos(url)) { + Timber.d("DOS TESTING - SHOULD OVERRIDE killing load") + return false + } + + try { return when (val urlType = specialUrlDetector.determineType(url)) { is SpecialUrlDetector.UrlType.Email -> { webViewClientListener?.sendEmailRequested(urlType.emailAddress) @@ -159,6 +166,12 @@ class BrowserWebViewClient( @WorkerThread override fun shouldInterceptRequest(webView: WebView, request: WebResourceRequest): WebResourceResponse? { + + if (request.isForMainFrame && urlIsGeneratingDos(request.url)) { + Timber.d("DOS TESTiNG - SHOULD INTERCEPT killing load") + return WebResourceResponse(null, null, null) + } + return runBlocking { try { val documentUrl = withContext(Dispatchers.Main) { webView.url } @@ -174,6 +187,20 @@ class BrowserWebViewClient( } } + // Hacky test implement properly + var count = 0 + var lastUrl: Uri? = null + private fun urlIsGeneratingDos(url: Uri?): Boolean { + if (lastUrl == url) { + count++ + } else { + count = 0 + } + lastUrl = url + Timber.d("DOS TESTING - count is $count") + return count >= 6 + } + @RequiresApi(Build.VERSION_CODES.O) override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean { Timber.w("onRenderProcessGone. Did it crash? ${detail?.didCrash()}") From 1573641f8a99f1d5422e8a611f71fa557039ca4e Mon Sep 17 00:00:00 2001 From: Mia Alexiou Date: Thu, 30 Jul 2020 13:00:50 +0100 Subject: [PATCH 2/9] Update comments --- .../java/com/duckduckgo/app/browser/BrowserWebViewClient.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index e6e9493f1008..c9a3e95debec 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -71,7 +71,7 @@ class BrowserWebViewClient( Timber.v("shouldOverride $url") - // Also validate on API 24 + // TODO: Also validate on API <24 and move into try block for exception monitoring if (urlIsGeneratingDos(url)) { Timber.d("DOS TESTING - SHOULD OVERRIDE killing load") return false @@ -167,6 +167,7 @@ class BrowserWebViewClient( @WorkerThread override fun shouldInterceptRequest(webView: WebView, request: WebResourceRequest): WebResourceResponse? { + //TODO: include this in exception handling (but not run blocking) if (request.isForMainFrame && urlIsGeneratingDos(request.url)) { Timber.d("DOS TESTiNG - SHOULD INTERCEPT killing load") return WebResourceResponse(null, null, null) @@ -187,7 +188,7 @@ class BrowserWebViewClient( } } - // Hacky test implement properly + // TODO: Hacky test implement properly var count = 0 var lastUrl: Uri? = null private fun urlIsGeneratingDos(url: Uri?): Boolean { From 5a6429b758e73c4fd0b5509c611fe2a05f485c91 Mon Sep 17 00:00:00 2001 From: Mia Alexiou Date: Thu, 6 Aug 2020 13:55:51 +0100 Subject: [PATCH 3/9] Committing hacky hacky first pass on timing --- .../app/browser/BrowserWebViewClient.kt | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index c9a3e95debec..140fb8b491e1 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -33,6 +33,7 @@ import com.duckduckgo.app.statistics.store.OfflinePixelCountDataStore import kotlinx.coroutines.* import timber.log.Timber import java.net.URI +import java.util.concurrent.Executors class BrowserWebViewClient( private val requestRewriter: RequestRewriter, @@ -188,18 +189,29 @@ class BrowserWebViewClient( } } - // TODO: Hacky test implement properly - var count = 0 var lastUrl: Uri? = null + var lastUrlLoadTime: Long? = null + + // TODO validate threading, may not even need this + var sharedThred = Executors.newFixedThreadPool(1).asCoroutineDispatcher() + private fun urlIsGeneratingDos(url: Uri?): Boolean { - if (lastUrl == url) { - count++ - } else { - count = 0 + return runBlocking(sharedThred) { + val lastKnownRefresh = lastUrlLoadTime + val now = System.currentTimeMillis() + if (lastKnownRefresh == null || now - lastKnownRefresh > 1000 || lastUrl != url) { + Timber.d("DOS TESTING: Valid INITIAL request for $url, permitting") + + lastUrl = url + lastUrlLoadTime = System.currentTimeMillis() + return@runBlocking false + } + lastUrl = url + lastUrlLoadTime = System.currentTimeMillis() + + Timber.d("DOS TESTING: Inalid request for $url, blocking") + return@runBlocking true } - lastUrl = url - Timber.d("DOS TESTING - count is $count") - return count >= 6 } @RequiresApi(Build.VERSION_CODES.O) From c75f943eb4d59c12269c8a60bff2d625ca295761 Mon Sep 17 00:00:00 2001 From: Marcos Holgado Date: Thu, 6 Aug 2020 15:04:39 +0100 Subject: [PATCH 4/9] Count how many reloads from the same url on the main thread have been requested. --- .../app/browser/BrowserWebViewClient.kt | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index 140fb8b491e1..d9b3307eb740 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -191,27 +191,32 @@ class BrowserWebViewClient( var lastUrl: Uri? = null var lastUrlLoadTime: Long? = null + var dosCount = 0 // TODO validate threading, may not even need this - var sharedThred = Executors.newFixedThreadPool(1).asCoroutineDispatcher() + var sharedThread = Executors.newFixedThreadPool(1).asCoroutineDispatcher() private fun urlIsGeneratingDos(url: Uri?): Boolean { - return runBlocking(sharedThred) { - val lastKnownRefresh = lastUrlLoadTime - val now = System.currentTimeMillis() - if (lastKnownRefresh == null || now - lastKnownRefresh > 1000 || lastUrl != url) { - Timber.d("DOS TESTING: Valid INITIAL request for $url, permitting") - - lastUrl = url - lastUrlLoadTime = System.currentTimeMillis() - return@runBlocking false - } - lastUrl = url - lastUrlLoadTime = System.currentTimeMillis() - - Timber.d("DOS TESTING: Inalid request for $url, blocking") - return@runBlocking true + val lastKnownRefresh = lastUrlLoadTime + val now = System.currentTimeMillis() + val diffInMs = lastKnownRefresh?.let { now - lastKnownRefresh } ?: 0 + Timber.d("DOS TESTING: Values are $lastKnownRefresh $diffInMs $lastUrl $dosCount") + val isUrlGeneratingDos = if (dosCount < 5 || lastKnownRefresh == null || diffInMs > 1000 || lastUrl != url) { + Timber.d("DOS TESTING: Valid INITIAL request for $url, permitting") + false + } else { + Timber.d("DOS TESTING: Invalid request for $url, blocking") + true } + if (lastUrl != url) { + dosCount = 0 + } else { + dosCount++ + } + + lastUrl = url + lastUrlLoadTime = System.currentTimeMillis() + return isUrlGeneratingDos } @RequiresApi(Build.VERSION_CODES.O) From 57b7de12909b3f9476d9d065810287ec6b5b112e Mon Sep 17 00:00:00 2001 From: Marcos Holgado Date: Mon, 10 Aug 2020 16:50:15 +0100 Subject: [PATCH 5/9] Load blank page if DoS attack detected --- .../app/browser/BrowserWebViewClientTest.kt | 4 +- .../duckduckgo/app/browser/DosDetectorTest.kt | 78 +++++++++++++++++++ .../app/browser/BrowserTabViewModel.kt | 5 ++ .../app/browser/BrowserWebViewClient.kt | 60 +++----------- .../com/duckduckgo/app/browser/DosDetector.kt | 50 ++++++++++++ .../app/browser/WebViewClientListener.kt | 1 + .../app/browser/di/BrowserModule.kt | 6 +- 7 files changed, 152 insertions(+), 52 deletions(-) create mode 100644 app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt create mode 100644 app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt index e0c82fd29b65..f319cbf8cd26 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt @@ -52,6 +52,7 @@ class BrowserWebViewClientTest { private val loginDetector: DOMLoginDetector = mock() private val offlinePixelCountDataStore: OfflinePixelCountDataStore = mock() private val uncaughtExceptionRepository: UncaughtExceptionRepository = mock() + private val dosDetector: DosDetector = DosDetector() @UiThreadTest @Before @@ -64,7 +65,8 @@ class BrowserWebViewClientTest { offlinePixelCountDataStore, uncaughtExceptionRepository, cookieManager, - loginDetector + loginDetector, + dosDetector ) testee.webViewClientListener = listener } diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt new file mode 100644 index 000000000000..4fe2dbf6cf2b --- /dev/null +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2020 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser + +import android.net.Uri +import com.duckduckgo.app.browser.DosDetector.Companion.MAX_REQUESTS_COUNT +import com.duckduckgo.app.browser.DosDetector.Companion.MIN_DIFF_IN_MS +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class DosDetectorTest { + + val testee: DosDetector = DosDetector() + + @Test + fun whenLessThanMaxRequestsCountCallsWithSameUrlThenReturnFalse() { + for (i in 0 until MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + } + + @Test + fun whenMoreThanMaxRequestsCountCallsWithSameUrlThenLastCallReturnsTrue() { + for (i in 0..MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + assertTrue(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + + @Test + fun whenMoreThanMaxRequestsCountCallsWithSameUrlAndDelayGreaterThanLimitThenReturnFalse() { + runBlocking { + for (i in 0..MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + delay((MIN_DIFF_IN_MS + 100).toLong()) + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + } + + @Test + fun whenMultipleRequestsFromDifferentUrlsThenReturnFalse() { + for (i in 0 until MAX_REQUESTS_COUNT * 2) { + if (i % 2 == 0) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } else { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example2.com"))) + } + } + } + + @Test + fun whenMaxRequestsReceivedConsecutivelyFromDifferentUrlsThenReturnFalse() { + for (i in 0 until MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + for (i in 0 until MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example2.com"))) + } + } +} diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index b77e991ee24c..5fe3c4a24a19 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -782,6 +782,11 @@ class BrowserTabViewModel( } } + override fun dosAttackDetected() { + invalidateBrowsingActions() + command.value = ShowErrorWithAction { this.onUserSubmittedQuery(url.orEmpty()) } + } + override fun titleReceived(newTitle: String) { site?.title = newTitle onSiteChanged() diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt index d9b3307eb740..058645fc458f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt @@ -33,7 +33,6 @@ import com.duckduckgo.app.statistics.store.OfflinePixelCountDataStore import kotlinx.coroutines.* import timber.log.Timber import java.net.URI -import java.util.concurrent.Executors class BrowserWebViewClient( private val requestRewriter: RequestRewriter, @@ -42,7 +41,8 @@ class BrowserWebViewClient( private val offlinePixelCountDataStore: OfflinePixelCountDataStore, private val uncaughtExceptionRepository: UncaughtExceptionRepository, private val cookieManager: CookieManager, - private val loginDetector: DOMLoginDetector + private val loginDetector: DOMLoginDetector, + private val dosDetector: DosDetector ) : WebViewClient() { var webViewClientListener: WebViewClientListener? = null @@ -53,7 +53,7 @@ class BrowserWebViewClient( */ override fun shouldOverrideUrlLoading(view: WebView, request: WebResourceRequest): Boolean { val url = request.url - return shouldOverride(view, url) + return shouldOverride(view, url, request.isForMainFrame) } /** @@ -62,23 +62,22 @@ class BrowserWebViewClient( @Suppress("OverridingDeprecatedMember") override fun shouldOverrideUrlLoading(view: WebView, urlString: String): Boolean { val url = Uri.parse(urlString) - return shouldOverride(view, url) + return shouldOverride(view, url, true) } /** * API-agnostic implementation of deciding whether to override url or not */ - private fun shouldOverride(webView: WebView, url: Uri): Boolean { + private fun shouldOverride(webView: WebView, url: Uri, isForMainFrame: Boolean): Boolean { Timber.v("shouldOverride $url") - - // TODO: Also validate on API <24 and move into try block for exception monitoring - if (urlIsGeneratingDos(url)) { - Timber.d("DOS TESTING - SHOULD OVERRIDE killing load") - return false - } - try { + if (isForMainFrame && dosDetector.isUrlGeneratingDos(url)) { + webView.loadUrl("about:blank") + webViewClientListener?.dosAttackDetected() + return false + } + return when (val urlType = specialUrlDetector.determineType(url)) { is SpecialUrlDetector.UrlType.Email -> { webViewClientListener?.sendEmailRequested(urlType.emailAddress) @@ -167,13 +166,6 @@ class BrowserWebViewClient( @WorkerThread override fun shouldInterceptRequest(webView: WebView, request: WebResourceRequest): WebResourceResponse? { - - //TODO: include this in exception handling (but not run blocking) - if (request.isForMainFrame && urlIsGeneratingDos(request.url)) { - Timber.d("DOS TESTiNG - SHOULD INTERCEPT killing load") - return WebResourceResponse(null, null, null) - } - return runBlocking { try { val documentUrl = withContext(Dispatchers.Main) { webView.url } @@ -189,36 +181,6 @@ class BrowserWebViewClient( } } - var lastUrl: Uri? = null - var lastUrlLoadTime: Long? = null - var dosCount = 0 - - // TODO validate threading, may not even need this - var sharedThread = Executors.newFixedThreadPool(1).asCoroutineDispatcher() - - private fun urlIsGeneratingDos(url: Uri?): Boolean { - val lastKnownRefresh = lastUrlLoadTime - val now = System.currentTimeMillis() - val diffInMs = lastKnownRefresh?.let { now - lastKnownRefresh } ?: 0 - Timber.d("DOS TESTING: Values are $lastKnownRefresh $diffInMs $lastUrl $dosCount") - val isUrlGeneratingDos = if (dosCount < 5 || lastKnownRefresh == null || diffInMs > 1000 || lastUrl != url) { - Timber.d("DOS TESTING: Valid INITIAL request for $url, permitting") - false - } else { - Timber.d("DOS TESTING: Invalid request for $url, blocking") - true - } - if (lastUrl != url) { - dosCount = 0 - } else { - dosCount++ - } - - lastUrl = url - lastUrlLoadTime = System.currentTimeMillis() - return isUrlGeneratingDos - } - @RequiresApi(Build.VERSION_CODES.O) override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean { Timber.w("onRenderProcessGone. Did it crash? ${detail?.didCrash()}") diff --git a/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt new file mode 100644 index 000000000000..00b9fb4d761b --- /dev/null +++ b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2020 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser + +import android.net.Uri +import javax.inject.Inject + +class DosDetector @Inject constructor() { + + var lastUrl: Uri? = null + var lastUrlLoadTime: Long? = null + var dosCount = 0 + + fun isUrlGeneratingDos(url: Uri?): Boolean { + val lastKnownRefresh = lastUrlLoadTime + val now = System.currentTimeMillis() + + val isUrlGeneratingDos = + !(dosCount < MAX_REQUESTS_COUNT || lastKnownRefresh == null || (now - lastKnownRefresh) > MIN_DIFF_IN_MS || lastUrl != url) + + if (lastUrl != url) { + dosCount = 0 + } else { + dosCount++ + } + + lastUrl = url + lastUrlLoadTime = System.currentTimeMillis() + return isUrlGeneratingDos + } + + companion object { + const val MIN_DIFF_IN_MS = 1_000 + const val MAX_REQUESTS_COUNT = 20 + } +} diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt index 00d872c40b94..163e268faedd 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt @@ -51,4 +51,5 @@ interface WebViewClientListener { fun surrogateDetected(surrogate: SurrogateResponse) fun loginDetected() + fun dosAttackDetected() } diff --git a/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt b/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt index 376ac08a0d85..bfce4a005dc4 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt @@ -83,7 +83,8 @@ class BrowserModule { offlinePixelCountDataStore: OfflinePixelCountDataStore, uncaughtExceptionRepository: UncaughtExceptionRepository, cookieManager: CookieManager, - loginDetector: DOMLoginDetector + loginDetector: DOMLoginDetector, + dosDetector: DosDetector ): BrowserWebViewClient { return BrowserWebViewClient( requestRewriter, @@ -92,7 +93,8 @@ class BrowserModule { offlinePixelCountDataStore, uncaughtExceptionRepository, cookieManager, - loginDetector + loginDetector, + dosDetector ) } From aac774e901a3ade0582dbf4dad08b0942c99836b Mon Sep 17 00:00:00 2001 From: Marcos Holgado Date: Tue, 11 Aug 2020 08:31:01 +0100 Subject: [PATCH 6/9] Change error message --- .../com/duckduckgo/app/browser/BrowserTabViewModelTest.kt | 6 ++++++ .../java/com/duckduckgo/app/browser/BrowserTabFragment.kt | 1 + .../com/duckduckgo/app/browser/BrowserTabViewModel.kt | 8 ++++---- app/src/main/res/values/string-untranslated.xml | 3 +++ 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 1ebcd0075aa5..c7cecc1d5b52 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -2309,6 +2309,12 @@ class BrowserTabViewModelTest { verify(mockPixel, never()).fire(Pixel.PixelName.UOA_VISITED) } + @Test + fun whenDosAttackDetectedThenErrorIsShown() { + testee.dosAttackDetected() + assertCommandIssued() + } + private inline fun assertCommandIssued(instanceAssertions: T.() -> Unit = {}) { verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) val issuedCommand = commandCaptor.allValues.find { it is T } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 628203add179..45e76aee4cac 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -595,6 +595,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope, DaxDialogLi private fun showErrorSnackbar(command: Command.ShowErrorWithAction) { // Snackbar is global and it should appear only the foreground fragment if (!errorSnackbar.view.isAttachedToWindow && isVisible) { + errorSnackbar.setText(command.textResId) errorSnackbar.setAction(R.string.crashedWebViewErrorAction) { command.action() }.show() } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index a06bd35331bb..c31cd861a3c4 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -243,7 +243,7 @@ class BrowserTabViewModel( object ShowWebContent : Command() class RefreshUserAgent(val host: String?, val isDesktop: Boolean) : Command() - class ShowErrorWithAction(val action: () -> Unit) : Command() + class ShowErrorWithAction(val textResId: Int, val action: () -> Unit) : Command() sealed class DaxCommand : Command() { object FinishTrackerAnimation : DaxCommand() class HideDaxDialog(val cta: Cta) : DaxCommand() @@ -809,7 +809,7 @@ class BrowserTabViewModel( override fun dosAttackDetected() { invalidateBrowsingActions() - command.value = ShowErrorWithAction { this.onUserSubmittedQuery(url.orEmpty()) } + showErrorWithAction(R.string.dosErrorMessage) } override fun titleReceived(newTitle: String) { @@ -1390,8 +1390,8 @@ class BrowserTabViewModel( ) } - private fun showErrorWithAction() { - command.value = ShowErrorWithAction { this.onUserSubmittedQuery(url.orEmpty()) } + private fun showErrorWithAction(errorMessage: Int = R.string.crashedWebViewErrorMessage) { + command.value = ShowErrorWithAction(errorMessage) { this.onUserSubmittedQuery(url.orEmpty()) } } private fun recoverTabWithQuery(query: String) { diff --git a/app/src/main/res/values/string-untranslated.xml b/app/src/main/res/values/string-untranslated.xml index 253bb78486f7..c985fa252e92 100644 --- a/app/src/main/res/values/string-untranslated.xml +++ b/app/src/main/res/values/string-untranslated.xml @@ -51,4 +51,7 @@ Success! %s has been added to your home screen. Checking your feed in DuckDuckGo is a great alternative to using the Facebook app!<br/><br/>But if the Facebook app is on your phone, it can make requests for data even when you\'re not using it.<br/><br/>Prevent this by deleting it now! + + Connection aborted. Website could be harmful to your device. + From cae2d5f0e5b4f86eb14691577255592bc22d607f Mon Sep 17 00:00:00 2001 From: Mia Alexiou Date: Tue, 11 Aug 2020 12:32:00 +0100 Subject: [PATCH 7/9] Tidy up and add count rest when timer exceeded --- .../com/duckduckgo/app/browser/DosDetector.kt | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt index 00b9fb4d761b..1a68a01e5724 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt @@ -26,21 +26,33 @@ class DosDetector @Inject constructor() { var dosCount = 0 fun isUrlGeneratingDos(url: Uri?): Boolean { - val lastKnownRefresh = lastUrlLoadTime - val now = System.currentTimeMillis() - val isUrlGeneratingDos = - !(dosCount < MAX_REQUESTS_COUNT || lastKnownRefresh == null || (now - lastKnownRefresh) > MIN_DIFF_IN_MS || lastUrl != url) + val currentUrlLoadTime = System.currentTimeMillis() - if (lastUrl != url) { - dosCount = 0 - } else { - dosCount++ + if (url != lastUrl) { + reset(url, currentUrlLoadTime) + return false } + if (!withinDosTimeWindow(currentUrlLoadTime)) { + reset(url, currentUrlLoadTime) + return false + } + + dosCount++ + lastUrlLoadTime = currentUrlLoadTime + return dosCount > MAX_REQUESTS_COUNT + } + + private fun reset(url: Uri?, currentLoadTime: Long) { + dosCount = 0 lastUrl = url - lastUrlLoadTime = System.currentTimeMillis() - return isUrlGeneratingDos + lastUrlLoadTime = currentLoadTime + } + + private fun withinDosTimeWindow(currentLoadTime: Long): Boolean { + val previousLoadTime = lastUrlLoadTime ?: return false + return (currentLoadTime - previousLoadTime) < MIN_DIFF_IN_MS } companion object { From 392d6020106a4fc926063db9aef81eb190123c35 Mon Sep 17 00:00:00 2001 From: Mia Alexiou Date: Tue, 11 Aug 2020 12:45:56 +0100 Subject: [PATCH 8/9] Add test for count reset when time exceeded --- .../com/duckduckgo/app/browser/DosDetectorTest.kt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt index 4fe2dbf6cf2b..5d73b117a807 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt @@ -55,6 +55,18 @@ class DosDetectorTest { } } + @Test + fun whenMoreThanMaxRequestsCountCallsWithSameUrlAndDelayGreaterThanLimitThenCountIsResetSoNextAndSusequentRequestsReturnFalse() { + runBlocking { + for (i in 0..MAX_REQUESTS_COUNT) { + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + delay((MIN_DIFF_IN_MS + 100).toLong()) + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) + } + } + @Test fun whenMultipleRequestsFromDifferentUrlsThenReturnFalse() { for (i in 0 until MAX_REQUESTS_COUNT * 2) { From 20178350f6ccb0c935b254f35bce93d8844d060d Mon Sep 17 00:00:00 2001 From: Marcos Holgado Date: Tue, 11 Aug 2020 12:47:47 +0100 Subject: [PATCH 9/9] Rename constant --- .../java/com/duckduckgo/app/browser/DosDetectorTest.kt | 8 ++++---- .../main/java/com/duckduckgo/app/browser/DosDetector.kt | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt index 5d73b117a807..5042570c5d09 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/DosDetectorTest.kt @@ -18,7 +18,7 @@ package com.duckduckgo.app.browser import android.net.Uri import com.duckduckgo.app.browser.DosDetector.Companion.MAX_REQUESTS_COUNT -import com.duckduckgo.app.browser.DosDetector.Companion.MIN_DIFF_IN_MS +import com.duckduckgo.app.browser.DosDetector.Companion.DOS_TIME_WINDOW_MS import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import org.junit.Assert.assertFalse @@ -50,18 +50,18 @@ class DosDetectorTest { for (i in 0..MAX_REQUESTS_COUNT) { assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) } - delay((MIN_DIFF_IN_MS + 100).toLong()) + delay((DOS_TIME_WINDOW_MS + 100).toLong()) assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) } } @Test - fun whenMoreThanMaxRequestsCountCallsWithSameUrlAndDelayGreaterThanLimitThenCountIsResetSoNextAndSusequentRequestsReturnFalse() { + fun whenMoreThanMaxRequestsCountCallsWithSameUrlAndDelayGreaterThanLimitThenCountIsResetSoNextAndSubsequentRequestsReturnFalse() { runBlocking { for (i in 0..MAX_REQUESTS_COUNT) { assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) } - delay((MIN_DIFF_IN_MS + 100).toLong()) + delay((DOS_TIME_WINDOW_MS + 100).toLong()) assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) assertFalse(testee.isUrlGeneratingDos(Uri.parse("http://example.com"))) } diff --git a/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt index 1a68a01e5724..87e6686c5b1f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/DosDetector.kt @@ -52,11 +52,11 @@ class DosDetector @Inject constructor() { private fun withinDosTimeWindow(currentLoadTime: Long): Boolean { val previousLoadTime = lastUrlLoadTime ?: return false - return (currentLoadTime - previousLoadTime) < MIN_DIFF_IN_MS + return (currentLoadTime - previousLoadTime) < DOS_TIME_WINDOW_MS } companion object { - const val MIN_DIFF_IN_MS = 1_000 + const val DOS_TIME_WINDOW_MS = 1_000 const val MAX_REQUESTS_COUNT = 20 } }