|
|
DescriptionUse 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 #
Messages
Total messages: 54 (33 generated)
asanka@chromium.org changed reviewers: + elawrence@chromium.org
Could you take a look? I couldn't reproduce the issue for data: URLs, but this should defend against other custom URL schemes that may be mishandled.
asanka@chromium.org changed reviewers: + jschuh@chromium.org
+jschuh : Could you take a look? It looks like elawrence is going to be unavailable for a while. This CL doesn't go very far to determine a canonical origin for download attribution. That'll be possible once we start keeping around a resource attribution graph, but that's not here yet.
On 2016/06/13 20:24:41, asanka wrote: > +jschuh : Could you take a look? It looks like elawrence is going to be > unavailable for a while. This CL doesn't go very far to determine a canonical > origin for download attribution. That'll be possible once we start keeping > around a resource attribution graph, but that's not here yet. Seems fine, but I just don't know enough about the details of what the OS expects here to say if it's right. So, I can rubberstamp it generically if that's what you want. If not, you should to wait for elawrence@ to get back.
On 2016/06/14 at 16:20:06, jschuh wrote: > On 2016/06/13 20:24:41, asanka wrote: > > +jschuh : Could you take a look? It looks like elawrence is going to be > > unavailable for a while. This CL doesn't go very far to determine a canonical > > origin for download attribution. That'll be possible once we start keeping > > around a resource attribution graph, but that's not here yet. > > Seems fine, but I just don't know enough about the details of what the OS expects here to say if it's right. So, I can rubberstamp it generically if that's what you want. If not, you should to wait for elawrence@ to get back. Thanks! I think I'll wait for elawrence@. Although the code works as expected during testing, the about:internet behavior isn't well documented.
This changelist LGTM. While I'm confident that "about:internet" is the right fallback string to use, I will ping the URLMon owners just to verify that nothing has changed. https://codereview.chromium.org/2025103002/diff/40001/content/browser/downloa... File content/browser/download/quarantine_win_unittest.cc (right): https://codereview.chromium.org/2025103002/diff/40001/content/browser/downloa... content/browser/download/quarantine_win_unittest.cc:98: "https://exmaple.com/foo", I *think* "exmaple" is a harmless typo, but maybe fix it to reduce ambiguity.
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. This CL attempts to mitigate the repercussions of this by changing base_file_win.cc to use the referrer URL if one is available and is either http or https. If no suitable URL can be determined, base_file_win.cc uses "about:internet" as the source URL. In addition, safe_util_win.cc now also sets the Referrer field if a valid referrer is found. BUG=601538 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added a check for URLs exceeding INTERNET_MAX_URL_LENGTH as well. https://codereview.chromium.org/2025103002/diff/40001/content/browser/downloa... File content/browser/download/quarantine_win_unittest.cc (right): https://codereview.chromium.org/2025103002/diff/40001/content/browser/downloa... content/browser/download/quarantine_win_unittest.cc:98: "https://exmaple.com/foo", On 2016/08/02 at 21:09:22, elawrence wrote: > I *think* "exmaple" is a harmless typo, but maybe fix it to reduce ambiguity. Fixed :)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asanka@chromium.org changed reviewers: + sky@chromium.org
+sky for content/test/BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
LGTM
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2025103002/#ps160001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by asanka@chromium.org
Whoops. Looks like an error in setting the referrer can sabotage the whole IAttachmentExecute invocation. Thanks Eric for pointing this out! Eric: Could you take a look at the change in quarantine_win and the new test?
One nitpicky question about variable naming/ordering in the test. Otherwise, l-g-t-m (As a provisional committer, I'm apparently not supposed to use the dashless version) https://codereview.chromium.org/2025103002/diff/180001/content/browser/downlo... File content/browser/download/quarantine_win_unittest.cc (right): https://codereview.chromium.org/2025103002/diff/180001/content/browser/downlo... content/browser/download/quarantine_win_unittest.cc:140: std::string referrer_string = "http://example.com/"; Nit: Maybe name "huge_referrer_string" or similar to convey the purpose of this unique instance? Another nit: would it make sense to slightly reorder this to: const char * [] { examples } std::string huge_referer_string std::vector<std::string> unsafe_referrers unsafe_referrers.pushback huge_referer_string So that the variables are declared in the order of use, or even: const char * [] { examples } std::vector<std::string> unsafe_referrers std::string huge_referer_string unsafe_referrers.pushback huge_referer_string so that the variables are defined immediately before their only use?
Patchset #11 (id:200001) has been deleted
I think you can still say LGTM, it just won't satisfy the presubmit check :) https://codereview.chromium.org/2025103002/diff/180001/content/browser/downlo... File content/browser/download/quarantine_win_unittest.cc (right): https://codereview.chromium.org/2025103002/diff/180001/content/browser/downlo... content/browser/download/quarantine_win_unittest.cc:140: std::string referrer_string = "http://example.com/"; On 2016/09/23 at 20:27:25, elawrence wrote: > Nit: Maybe name "huge_referrer_string" or similar to convey the purpose of this unique instance? Done. > Another nit: would it make sense to slightly reorder this to: > > const char * [] { examples } > std::string huge_referer_string > std::vector<std::string> unsafe_referrers > unsafe_referrers.pushback huge_referer_string > > So that the variables are declared in the order of use, or even: > > const char * [] { examples } > std::vector<std::string> unsafe_referrers > > std::string huge_referer_string > unsafe_referrers.pushback huge_referer_string > > so that the variables are defined immediately before their only use? Done.
On 2016/09/23 20:41:15, asanka wrote: > I think you can still say LGTM, it just won't satisfy the presubmit check :) Okay lgtm then
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2025103002/#ps220001 (title: "Some cleanup")
On 2016/09/23 at 20:58:12, elawrence wrote: > On 2016/09/23 20:41:15, asanka wrote: > > I think you can still say LGTM, it just won't satisfy the presubmit check :) > > Okay lgtm then Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6ef77adf8c1f6ab03e8614646275d22b4fa3f448 Cr-Commit-Position: refs/heads/master@{#421000} |