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

Issue 2025103002: Use better fallback URLs when calling AVScanFile(). (Closed)

Created:
4 years, 6 months ago by asanka
Modified:
4 years, 2 months ago
Reviewers:
jschuh, elawrence, sky
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use better fallback URLs when calling AVScanFile(). Windows Attachment Services defaults to treating downloads of data: URLs and other unknown URL schemes as being trustworthy. In addition, source URLs that are longer than INTERNET_MAX_URL_LENGTH could also cause the mark-of-the-web to not be applied. This CL attempts to mitigate these cases by changing base_file_win.cc to use the referrer URL if one is available and is either http or https. Furthermore, if the source URL is too long or is empty, quarantine_win.cc will use the safe fallback "about:internet" as the source URL. In addition, quarantine_win.cc now also sets the Referrer field if a valid referrer is found. BUG=601538 Committed: https://crrev.com/6ef77adf8c1f6ab03e8614646275d22b4fa3f448 Cr-Commit-Position: refs/heads/master@{#421000}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Catch up with upstream changes. #

Total comments: 2

Patch Set 4 : Also use our safe fallback if the source URL is super long #

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : . #

Patch Set 8 : . #

Patch Set 9 : . #

Patch Set 10 : A failure while setting the referrer can sabotage the IAE object. #

Total comments: 2

Patch Set 11 : Some cleanup #

Unified diffs Side-by-side diffs Delta from patch set Stats (+334 lines, -29 lines) Patch
M content/browser/download/base_file.cc View 1 2 3 4 2 chunks +37 lines, -2 lines 0 comments Download
A content/browser/download/base_file_win_unittest.cc View 1 2 3 4 5 1 chunk +115 lines, -0 lines 0 comments Download
M content/browser/download/quarantine_win.cc View 1 2 3 4 5 6 7 8 9 5 chunks +36 lines, -7 lines 0 comments Download
M content/browser/download/quarantine_win_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +145 lines, -20 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 54 (33 generated)
asanka
Could you take a look? I couldn't reproduce the issue for data: URLs, but this ...
4 years, 6 months ago (2016-05-31 22:08:44 UTC) #2
asanka
+jschuh : Could you take a look? It looks like elawrence is going to be ...
4 years, 6 months ago (2016-06-13 20:24:41 UTC) #4
jschuh
On 2016/06/13 20:24:41, asanka wrote: > +jschuh : Could you take a look? It looks ...
4 years, 6 months ago (2016-06-14 16:20:06 UTC) #5
asanka
On 2016/06/14 at 16:20:06, jschuh wrote: > On 2016/06/13 20:24:41, asanka wrote: > > +jschuh ...
4 years, 6 months ago (2016-06-14 16:24:08 UTC) #6
elawrence
This changelist LGTM. While I'm confident that "about:internet" is the right fallback string to use, ...
4 years, 4 months ago (2016-08-02 21:09:23 UTC) #7
asanka
Added a check for URLs exceeding INTERNET_MAX_URL_LENGTH as well. https://codereview.chromium.org/2025103002/diff/40001/content/browser/download/quarantine_win_unittest.cc File content/browser/download/quarantine_win_unittest.cc (right): https://codereview.chromium.org/2025103002/diff/40001/content/browser/download/quarantine_win_unittest.cc#newcode98 content/browser/download/quarantine_win_unittest.cc:98: ...
4 years, 3 months ago (2016-09-23 01:34:53 UTC) #19
asanka
+sky for content/test/BUILD.gn
4 years, 3 months ago (2016-09-23 01:35:57 UTC) #23
sky
LGTM
4 years, 3 months ago (2016-09-23 17:33:48 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2025103002/160001
4 years, 3 months ago (2016-09-23 18:48:38 UTC) #33
asanka
Whoops. Looks like an error in setting the referrer can sabotage the whole IAttachmentExecute invocation. ...
4 years, 3 months ago (2016-09-23 20:03:45 UTC) #35
elawrence
One nitpicky question about variable naming/ordering in the test. Otherwise, l-g-t-m (As a provisional committer, ...
4 years, 3 months ago (2016-09-23 20:27:25 UTC) #36
asanka
I think you can still say LGTM, it just won't satisfy the presubmit check :) ...
4 years, 2 months ago (2016-09-23 20:41:15 UTC) #38
elawrence
On 2016/09/23 20:41:15, asanka wrote: > I think you can still say LGTM, it just ...
4 years, 2 months ago (2016-09-23 20:58:12 UTC) #39
asanka
On 2016/09/23 at 20:58:12, elawrence wrote: > On 2016/09/23 20:41:15, asanka wrote: > > I ...
4 years, 2 months ago (2016-09-23 22:18:00 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2025103002/220001
4 years, 2 months ago (2016-09-23 22:18:24 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/299112)
4 years, 2 months ago (2016-09-24 00:46:10 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2025103002/220001
4 years, 2 months ago (2016-09-26 16:58:52 UTC) #47
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/286237)
4 years, 2 months ago (2016-09-26 18:42:22 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2025103002/220001
4 years, 2 months ago (2016-09-26 19:28:02 UTC) #51
commit-bot: I haz the power
Committed patchset #11 (id:220001)
4 years, 2 months ago (2016-09-26 21:37:04 UTC) #52
commit-bot: I haz the power
4 years, 2 months ago (2016-09-26 21:40:39 UTC) #54
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/6ef77adf8c1f6ab03e8614646275d22b4fa3f448
Cr-Commit-Position: refs/heads/master@{#421000}

Powered by Google App Engine
This is Rietveld 408576698