New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Yesterday
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug

Blocked on:
issue 809775



Sign in to add a comment

Browser hang in M67 with excessive download attempts

Project Member Reported by gab@chromium.org, Jul 3

Issue description

Similar to  issue 809775 , the browser hangs when the attached repro is run.

As mentioned @  crbug.com/809775#c39 , this is a new instance of browser process DoS. The long-term solution will be to manage incoming requests on BrowserThread::IO. In the meantime hotfixes will be required.

On M67-M69, the issue with this specific repro is that the download UI goes nuts trying to refresh and is no longer able to process input as a result (thus the hang is unstoppable). See attached flame graph (even though the warning dialog for too many downloads comes up, chrome still goes into an infinite queue of processing download messages). Even killing the bad renderer results in minutes of 100% CPU processing queued requests.

@qinmin as download/OWNERS
 
repro.html
574 bytes View Download
chrome.exe_13284_13288.svg
4.1 MB Download
I think the ultimate fix should be in the renderer side to detect such malicious page as it fires tons of request for the same URL.
Once all those requests reaches the browser, it will definitely slow everything down. 
Download uses DownloadRequestLimiter to throttle download, but even if user doesn't accept the prompt dialog, the number of request to handle on the browser side can still consume all the CPUs.

Cc: robliao@chromium.org
From robliao@ offline :

"""
I did some targeted lightspeeds (e.g. no-oping selected areas) in the downloads code on the UI thread on where we spend a bit of time and the resulting effect seems to be to make more time for the incoming requests with limited improvement in responsiveness. 

Rate-limiting the downloads as well as splitting the incoming tasks to the UI thread would also help in preventing this hang.
"""
The ultimate fix can't be strictly in the renderer IMO as our security model is for the browser to assume the renderer has gone rogue.

Having SequenceManager on the UI and IO threads will allow us to solve this nicely IMO.

If there isn't anything we can do to hotfix downloads, feel free to reassign to me.
The examples I have seen are using the download attribute, which could mean it was probably fixed by https://chromium-review.googlesource.com/758236 and reintroduced by https://chromium-review.googlesource.com/992322
Cc: jochen@chromium.org
Interesting, @jochen any idea if r548465 could have caused this?
Cc: mek@chromium.org
it didn't cause this, but as carewolf@ said reintroduced.

I wonder whether https://chromium-review.googlesource.com/c/chromium/src/+/899666 is related?

At this point, this extra hop is no longer required

/cc mek
Cc: qin...@chromium.org
Owner: gab@chromium.org
i don't think there are any hotfixes we can do at download side.
DownloadResourceThrottle already provides such protection by allowing only one download to take place from the page.  
However, if user allows multiple download to happen on the page, then the UI thread will become swamped by all the DownloadShelf updates. 
@qinmin : did you run the repro? It hangs even if user blocks multiple downloads. The whole UI is hung, even the page unresponsive dialog won't draw (see it as a transparent window in the taskbar).
Yes, I run it on my linux desktop. 
If user blocks multiple downloads, i can switch to another tab and open settings etc.
If user don't do anything, a pop up dialog will be shown, and the whole browser become unresponsive and switching tabs are not possible. I can still go to terminal to kill the browser process though.
If user allows multiple download, the download shelf will go crazy and the whole machine starts becoming sluggish. Shutting down the browser process also becomes a uneasy task.

If we don't want too much stress on the UI thread, we have to move a lot DownloadRequestLimiter work onto the IO thread. But since content settings/webcontents/NavigationHandle are all accessed on the UI thread, there is a lot of stuff needs to be reworked. I don't think a short term hotfix will be possible though.
Ok, would some sort of backpressure system on the download IPC work?

Le jeu. 5 juill. 2018 16 h 54, qinmin via monorail <
monorail+v2.951229368@chromium.org> a écrit :
Yes, if we can rate limit calls to RenderFrameMessageFilter::DownloadUrl(), that would definitely free things up on the UI thread
Agreed. The easiest solution at the moment is to impose a rate limit (akin to a QPS limit) on multiple download attempts.
Owner: qin...@chromium.org
ok, assigning this to me as i will work on a temporary rate limit fix
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6

commit a13cb2a1020b933c070d9db84fbc48dae5b0e1c6
Author: Min Qin <qinmin@chromium.org>
Date: Wed Jul 11 04:53:36 2018

Throttle url download requests from a RenderFrame

RenderFrameImpl::DownloadUrl() can be abused to trigger a large number
of download to overwhelm the browser process.
This CL rate limits the number of download to a maximum of 10 per second.

Bug:  860045 
Change-Id: I490ae3a99eb59aef709b9f1e6be5eb0f787354f4
Reviewed-on: https://chromium-review.googlesource.com/1132485
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574078}
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl.h
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl_browsertest.cc

Labels: Merge-Request-68
Status: Started (was: Assigned)
tested on an very old windows 7 machine (bought in 2009) with the latest canary build, browser is still responsive even after i clicked accept all download button from the malicious page.
Project Member

Comment 16 by sheriffbot@chromium.org, Jul 12

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 17 by abdulsyed@google.com, Jul 13 (5 days ago)

How safe is this merge overall?

Comment 18 by qin...@chromium.org, Jul 13 (5 days ago)

Should be quite safe, i think. If there are pages that issuing a lot of downloads within a second, they will be affected anyways when M69 is released.

Comment 19 by abdulsyed@google.com, Jul 16 (2 days ago)

Labels: -Merge-Review-68 Merge-Approved-68
Approving merge to M68. Branch:3440
Project Member

Comment 20 by bugdroid1@chromium.org, Yesterday (38 hours ago)

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d44d2be0eb693d9b477b476072011cc3b852a78

commit 4d44d2be0eb693d9b477b476072011cc3b852a78
Author: Min Qin <qinmin@chromium.org>
Date: Tue Jul 17 20:41:16 2018

Throttle url download requests from a RenderFrame

RenderFrameImpl::DownloadUrl() can be abused to trigger a large number
of download to overwhelm the browser process.
This CL rate limits the number of download to a maximum of 10 per second.

Bug:  860045 
Change-Id: I490ae3a99eb59aef709b9f1e6be5eb0f787354f4
Reviewed-on: https://chromium-review.googlesource.com/1132485
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#574078}(cherry picked from commit a13cb2a1020b933c070d9db84fbc48dae5b0e1c6)
Reviewed-on: https://chromium-review.googlesource.com/1140457
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#703}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl.h
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl_browsertest.cc

Comment 21 by qin...@chromium.org, Yesterday (38 hours ago)

Status: Fixed (was: Started)

Sign in to add a comment