Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641063002:
Fix a problem that user validated dangerous download cannot resume (Closed)
DescriptionFix a problem that user validated dangerous download cannot resume
After a dangerous download become user validated, it should be treated as not dangerous.
However, Chrome currently still checks for downloadUrl and referer.
This CL fixes the problem and added a test for the case.
BUG=682468
Review-Url: https://codereview.chromium.org/2641063002
Cr-Commit-Position: refs/heads/master@{#444941}
Committed: https://chromium.googlesource.com/chromium/src/+/d5d9f828c9c9366fa219e6a7aad6eccfdd2dd1fd
Patch Set 1 : remove unused assert #Patch Set 2 : remove dependencies #
Messages
Total messages: 33 (17 generated)
qinmin@chromium.org changed reviewers: + asanka@chromium.org, dtrainor@chromium.org
asanka@, PTAL
lgtm
The only effect of this change is to skip the URL check if the download was validated by the user. Could you explain how the URL check was preventing an accepted download from being resumed? I don't disagree with the change. Just that I'm not sure I understand what the underlying issue is. https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_target_determiner_unittest.cc (right): https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_target_determiner_unittest.cc:1934: // ALLOW_ON_USER_GESTURE. This test doesn't seem to rely on this assumption (which is a good thing).
When resuming a dangerous download after browser crash, we will call DTD to re-determine the download target. If the download is previously validated, the danger_type_ should be DOWNLOAD_DANGER_TYPE_USER_VALIDATED. However, checking download url and the referer can potentially change the danger_type_ back to something unsafe. That causes the DownloadController::OnDangerousDownload() to get called when download item updates its observers. The DownloadController requires the download item to have a valid webContent to show the dangerous infobar, which is not applicable to a download item that is loaded from the history service. This causes the download resumption to silently fail. On 2017/01/19 22:12:41, asanka wrote: > The only effect of this change is to skip the URL check if the download was > validated by the user. Could you explain how the URL check was preventing an > accepted download from being resumed? > > I don't disagree with the change. Just that I'm not sure I understand what the > underlying issue is. > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > File chrome/browser/download/download_target_determiner_unittest.cc (right): > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > ALLOW_ON_USER_GESTURE. > This test doesn't seem to rely on this assumption (which is a good thing).
On 2017/01/19 at 23:43:35, qinmin wrote: > When resuming a dangerous download after browser crash, we will call DTD to re-determine the download target. > If the download is previously validated, the danger_type_ should be DOWNLOAD_DANGER_TYPE_USER_VALIDATED. > However, checking download url and the referer can potentially change the danger_type_ back to something unsafe. > That causes the DownloadController::OnDangerousDownload() to get called when download item updates its observers. > The DownloadController requires the download item to have a valid webContent to show the dangerous infobar, which is not applicable to a download item that is loaded from the history service. > This causes the download resumption to silently fail. > > On 2017/01/19 22:12:41, asanka wrote: > > The only effect of this change is to skip the URL check if the download was > > validated by the user. Could you explain how the URL check was preventing an > > accepted download from being resumed? > > > > I don't disagree with the change. Just that I'm not sure I understand what the > > underlying issue is. > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > File chrome/browser/download/download_target_determiner_unittest.cc (right): > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > > ALLOW_ON_USER_GESTURE. > > This test doesn't seem to rely on this assumption (which is a good thing). Got it. LGTM.
On 2017/01/19 at 23:48:54, asanka wrote: > On 2017/01/19 at 23:43:35, qinmin wrote: > > When resuming a dangerous download after browser crash, we will call DTD to re-determine the download target. > > If the download is previously validated, the danger_type_ should be DOWNLOAD_DANGER_TYPE_USER_VALIDATED. > > However, checking download url and the referer can potentially change the danger_type_ back to something unsafe. > > That causes the DownloadController::OnDangerousDownload() to get called when download item updates its observers. > > The DownloadController requires the download item to have a valid webContent to show the dangerous infobar, which is not applicable to a download item that is loaded from the history service. > > This causes the download resumption to silently fail. > > > > On 2017/01/19 22:12:41, asanka wrote: > > > The only effect of this change is to skip the URL check if the download was > > > validated by the user. Could you explain how the URL check was preventing an > > > accepted download from being resumed? > > > > > > I don't disagree with the change. Just that I'm not sure I understand what the > > > underlying issue is. > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > File chrome/browser/download/download_target_determiner_unittest.cc (right): > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > > > ALLOW_ON_USER_GESTURE. > > > This test doesn't seem to rely on this assumption (which is a good thing). > > Got it. LGTM. Could you add a note to the bug explaining the circumstances under which the resumption will fail? The bug seems to imply that all downloads are affected. In general, checking the download URL on each resumption is intentional. It's meant to account for cases where the download URL is added to the SB lists after the download was started.
One interesting thing is that DoCheckDownloadUrl() can change danger_type_ from DOWNLOAD_DANGER_TYPE_USER_VALIDATED to DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, that causes DownloadTargetDeterminer::DoCheckVisitedReferrerBefore() to calculate the danger_level_ and cause the danger_type_ to be set to a dangerous value later. BTW, danger type change currently is not marked as requiring immediate db commit. This means that if user validates a dangerous download, then immediately kills Chrome, the download will fail when user resumes the download later. On 2017/01/19 23:43:35, qinmin wrote: > When resuming a dangerous download after browser crash, we will call DTD to > re-determine the download target. > If the download is previously validated, the danger_type_ should be > DOWNLOAD_DANGER_TYPE_USER_VALIDATED. > However, checking download url and the referer can potentially change the > danger_type_ back to something unsafe. > That causes the DownloadController::OnDangerousDownload() to get called when > download item updates its observers. > The DownloadController requires the download item to have a valid webContent to > show the dangerous infobar, which is not applicable to a download item that is > loaded from the history service. > This causes the download resumption to silently fail. > > On 2017/01/19 22:12:41, asanka wrote: > > The only effect of this change is to skip the URL check if the download was > > validated by the user. Could you explain how the URL check was preventing an > > accepted download from being resumed? > > > > I don't disagree with the change. Just that I'm not sure I understand what the > > underlying issue is. > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > File chrome/browser/download/download_target_determiner_unittest.cc (right): > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > > ALLOW_ON_USER_GESTURE. > > This test doesn't seem to rely on this assumption (which is a good thing).
On 2017/01/19 at 23:58:37, qinmin wrote: > One interesting thing is that DoCheckDownloadUrl() can change danger_type_ from DOWNLOAD_DANGER_TYPE_USER_VALIDATED to DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, > that causes DownloadTargetDeterminer::DoCheckVisitedReferrerBefore() to calculate the danger_level_ and cause the danger_type_ to be set to a dangerous value later. > > BTW, danger type change currently is not marked as requiring immediate db commit. This means that if user validates a dangerous download, then immediately kills Chrome, the download will fail when user resumes the download later. > Sure. But that window is going to be there anyway since the DB write can be delayed arbitrarily even if it were going to be committed immediately after the write or not. BTW, is this an issue that users are running into in the wild? The URL check indicates that the site is a known malware host, or the URL is known to contain malware. Either way, the user is very likely downloading malware. > On 2017/01/19 23:43:35, qinmin wrote: > > When resuming a dangerous download after browser crash, we will call DTD to > > re-determine the download target. > > If the download is previously validated, the danger_type_ should be > > DOWNLOAD_DANGER_TYPE_USER_VALIDATED. > > However, checking download url and the referer can potentially change the > > danger_type_ back to something unsafe. > > That causes the DownloadController::OnDangerousDownload() to get called when > > download item updates its observers. > > The DownloadController requires the download item to have a valid webContent to > > show the dangerous infobar, which is not applicable to a download item that is > > loaded from the history service. > > This causes the download resumption to silently fail. > > > > On 2017/01/19 22:12:41, asanka wrote: > > > The only effect of this change is to skip the URL check if the download was > > > validated by the user. Could you explain how the URL check was preventing an > > > accepted download from being resumed? > > > > > > I don't disagree with the change. Just that I'm not sure I understand what the > > > underlying issue is. > > > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > File chrome/browser/download/download_target_determiner_unittest.cc (right): > > > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > > > ALLOW_ON_USER_GESTURE. > > > This test doesn't seem to rely on this assumption (which is a good thing).
On 2017/01/20 00:08:33, asanka wrote: > On 2017/01/19 at 23:58:37, qinmin wrote: > > One interesting thing is that DoCheckDownloadUrl() can change danger_type_ > from DOWNLOAD_DANGER_TYPE_USER_VALIDATED to DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, > > that causes DownloadTargetDeterminer::DoCheckVisitedReferrerBefore() to > calculate the danger_level_ and cause the danger_type_ to be set to a dangerous > value later. > > > > BTW, danger type change currently is not marked as requiring immediate db > commit. This means that if user validates a dangerous download, then > immediately kills Chrome, the download will fail when user resumes the download > later. > > > > Sure. But that window is going to be there anyway since the DB write can be > delayed arbitrarily even if it were going to be committed immediately after the > write or not. > > BTW, is this an issue that users are running into in the wild? The URL check > indicates that the site is a known malware host, or the URL is known to contain > malware. Either way, the user is very likely downloading malware. > Downloading any apk files may cause this problem. So this may be pretty common on Android. > > On 2017/01/19 23:43:35, qinmin wrote: > > > When resuming a dangerous download after browser crash, we will call DTD to > > > re-determine the download target. > > > If the download is previously validated, the danger_type_ should be > > > DOWNLOAD_DANGER_TYPE_USER_VALIDATED. > > > However, checking download url and the referer can potentially change the > > > danger_type_ back to something unsafe. > > > That causes the DownloadController::OnDangerousDownload() to get called when > > > download item updates its observers. > > > The DownloadController requires the download item to have a valid webContent > to > > > show the dangerous infobar, which is not applicable to a download item that > is > > > loaded from the history service. > > > This causes the download resumption to silently fail. > > > > > > On 2017/01/19 22:12:41, asanka wrote: > > > > The only effect of this change is to skip the URL check if the download > was > > > > validated by the user. Could you explain how the URL check was preventing > an > > > > accepted download from being resumed? > > > > > > > > I don't disagree with the change. Just that I'm not sure I understand what > the > > > > underlying issue is. > > > > > > > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > > File chrome/browser/download/download_target_determiner_unittest.cc > (right): > > > > > > > > > > > > https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... > > > > chrome/browser/download/download_target_determiner_unittest.cc:1934: // > > > > ALLOW_ON_USER_GESTURE. > > > > This test doesn't seem to rely on this assumption (which is a good thing).
updated the crbug about the scenario this might happen https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_target_determiner_unittest.cc (right): https://codereview.chromium.org/2641063002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_target_determiner_unittest.cc:1934: // ALLOW_ON_USER_GESTURE. On 2017/01/19 22:12:41, asanka wrote: > This test doesn't seem to rely on this assumption (which is a good thing). removed.
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2641063002/#ps20001 (title: "remove unused assert")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2641063002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2641063002/#ps40001 (title: "remove dependencies")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2641063002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by qinmin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2641063002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2641063002/#ps60001 (title: "remove dependencies")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2641063002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484873872216250,
"parent_rev": "b8a6df2fa4a38a9ca7f47141df6dec1b61352274", "commit_rev":
"d5d9f828c9c9366fa219e6a7aad6eccfdd2dd1fd"}
Message was sent while issue was closed.
Description was changed from ========== Fix a problem that user validated dangerous download cannot resume After a dangerous download become user validated, it should be treated as not dangerous. However, Chrome currently still checks for downloadUrl and referer. This CL fixes the problem and added a test for the case. BUG=682468 ========== to ========== Fix a problem that user validated dangerous download cannot resume After a dangerous download become user validated, it should be treated as not dangerous. However, Chrome currently still checks for downloadUrl and referer. This CL fixes the problem and added a test for the case. BUG=682468 Review-Url: https://codereview.chromium.org/2641063002 Cr-Commit-Position: refs/heads/master@{#444941} Committed: https://chromium.googlesource.com/chromium/src/+/d5d9f828c9c9366fa219e6a7aad6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d5d9f828c9c9366fa219e6a7aad6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
