Modifying the "dangerous download" algorithm.

Modifying the "dangerous download" algorithm.

Downloads are considered dangerous if:
a) The file is dangerous just by sitting on the drive, without needing to be clicked on e.g. dll, xbap
b) The file is executable and the download was not user initiated.
c) They are an extension that is not from the gallery

We have defined a user initiated download as 3 possible cases:
a) A user enters a URL into the address bar that is a file
b) A user left clicks on a URL that is a file
c) A user right clicks and does "Save As" on a URL that is a file. 

BUG=9044
TEST=Open a page with a download link to a dangerous file that is not an extension, e.g. an .exe file, and left click on the link. The download should proceed without a prompt.

[Jay Civelli, 2010-03-26 17:31:26]

http://codereview.chromium.org/1403001/diff/1/2
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/1/2#newcode669
chrome/browser/download/download_manager.cc:669: // b) Unprompted downloads are
saved without a prompt iff:
Nit: iff -> if

http://codereview.chromium.org/1403001/diff/1/2#newcode675
chrome/browser/download/download_manager.cc:675: // b) The file is an extension
that is not from the gallery.
This comment confuses me. It describes the cases for which we don't show a
prompt. It should probably start with when we show a prompt and then list the
exceptions.

http://codereview.chromium.org/1403001/diff/1/2#newcode1139
chrome/browser/download/download_manager.cc:1139: is_user_triggered_download =
contents->is_user_triggered_load();
Couldn't we simply return false if contents->is_user_triggered_load()?
Also, since this is called as a result of task being posted from the IO thread
to the UI thread, how can you be sure the user_triggered_load state is the right
one?
There could have been a new provisional load notification on the TabContents
since the task was posted. May be the state should be attached to the
DownloadCreateInfo somehow?

http://codereview.chromium.org/1403001/diff/1/7
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/1403001/diff/1/7#newcode1784
chrome/browser/tab_contents/tab_contents.cc:1784: 
Nit: Remove blank line.

[Avi (use Gerrit), 2010-03-26 17:34:45]

http://codereview.chromium.org/1403001/diff/1/2
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/1/2#newcode669
chrome/browser/download/download_manager.cc:669: // b) Unprompted downloads are
saved without a prompt iff:
On 2010/03/26 17:31:26, Jay Civelli wrote:
> Nit: iff -> if

iff = if and only if. Valid.

[jcampan, 2010-03-26 17:36:37]

>> Nit: iff -> if
>
> iff = if and only if. Valid.
Ah! Sorry for my ignorance.

Jay

[pierre.lafayette, 2010-03-26 19:29:01]

+pkasting

Peter can you offer some insight here? Thanks.

[Peter Kasting, 2010-03-26 20:03:40]

On 2010/03/26 19:29:01, pierre.lafayette wrote:
> +pkasting
> 
> Peter can you offer some insight here? Thanks.

Insight about what?  A more specific question would be useful.

[pierre.lafayette, 2010-03-26 20:47:21]

On 2010/03/26 20:03:40, Peter Kasting wrote:
> On 2010/03/26 19:29:01, pierre.lafayette wrote:
> > +pkasting
> > 
> > Peter can you offer some insight here? Thanks.
> 
> Insight about what?  A more specific question would be useful.

Sorry, I meant to reference Jay's comment:
> Also, since this is called as a result of task being posted from the IO thread
to the UI thread, how can you be sure the user_triggered_load state is the right
one?
> There could have been a new provisional load notification on the TabContents
since the task was posted.

If it's true that a new provisional load notification can make it through before
the DownloadManager starts its download for the previous load, is there a more
reliable way to determine that the user triggered the navigation?

[Peter Kasting, 2010-03-26 20:56:56]

On 2010/03/26 20:47:21, pierre.lafayette wrote:
> On 2010/03/26 20:03:40, Peter Kasting wrote:
> > On 2010/03/26 19:29:01, pierre.lafayette wrote:
> > > +pkasting
> > > 
> > > Peter can you offer some insight here? Thanks.
> > 
> > Insight about what?  A more specific question would be useful.
> 
> Sorry, I meant to reference Jay's comment:
> > Also, since this is called as a result of task being posted from the IO
thread
> to the UI thread, how can you be sure the user_triggered_load state is the
right
> one?
> > There could have been a new provisional load notification on the TabContents
> since the task was posted.
> 
> If it's true that a new provisional load notification can make it through
before
> the DownloadManager starts its download for the previous load, is there a more
> reliable way to determine that the user triggered the navigation?

I'm sorry, I have no idea how any of this works.  You may want brettw.

[pierre.lafayette, 2010-03-26 21:22:45]

Hi Brett, can you comment on the following?

Referencing Jay's comment:
> Also, since this is called as a result of task being posted from the IO thread
to the UI thread, how can you be sure the user_triggered_load state is the right
one?
> There could have been a new provisional load notification on the TabContents
since the task was posted.

If it's true that a new provisional load notification can make it through before
the DownloadManager starts its download for the previous load, is there a more
reliable way to determine that the user triggered the navigation?

[pierre.lafayette, 2010-03-27 21:37:16]

Hi Jay, see my comments below.
Thanks!

http://codereview.chromium.org/1403001/diff/1/2
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/1/2#newcode1139
chrome/browser/download/download_manager.cc:1139: is_user_triggered_download =
contents->is_user_triggered_load();
On 2010/03/26 17:31:26, Jay Civelli wrote:
> Couldn't we simply return false if contents->is_user_triggered_load()?

Yes, you are correct I am violating our return early policy.

> Also, since this is called as a result of task being posted from the IO thread
> to the UI thread, how can you be sure the user_triggered_load state is the
right
> one?
> There could have been a new provisional load notification on the TabContents
> since the task was posted. May be the state should be attached to the
> DownloadCreateInfo somehow?

The only case we are concerned about for this is if a download that is NOT
triggered by a user skips the dangerous file prompt due to a new provisional
load notification that changes the user_triggered_load state. For this to
happen, the user would have to click on a link in the TabContents that generated
the download request. I've tried hard to make this case happen on my build but
it seems that while in theory it may be possible, there is a ridiculously low
probability that a user's click request would change the state before the
download hits the StartDownload code. Add that with the low probability that a
user would immediately click on another link after having clicked to download a
file and I believe that we should be able to sleep soundly on these matters.
Correct me if my reasoning is flawed, but note that in testing I could not
reproduce this false-negative case.

[pierre.lafayette, 2010-03-27 21:43:01]

On 2010/03/27 21:37:16, pierre.lafayette wrote:
> Add that with the low probability that a user would immediately click on
another link after having clicked to download a file and I believe that we
should be able to sleep soundly on these matters.

Sorry, I meant the low probability that a user would click on a link at the
precise time the page triggers a download. That and the click not causing the
page to navigate before the download even starts.

[Peter Kasting, 2010-03-27 22:00:26]

On 2010/03/27 21:37:16, pierre.lafayette wrote:
> I've tried hard to make this case happen on my build but
> it seems that while in theory it may be possible, there is a ridiculously low
> probability that a user's click request would change the state before the
> download hits the StartDownload code.

Why not just include the trigger information in the DownloadCreateInfo as Jay
suggested?  Then you won't need to worry about probabilities.

[Aaron Boodman, 2010-03-28 00:39:08]

Sorry to butt in, but we thought really hard about this case for extensions.

http://codereview.chromium.org/1403001/diff/1/2
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/1/2#newcode669
chrome/browser/download/download_manager.cc:669: // b) Unprompted downloads are
saved without a prompt iff:
I think "unprompted download" is confusing terminology. IIUC, you're using it to
refer to a download that wasn't initiated by user-gesture, right? Since we're
also talking about a different kind of prompt here -- the UI -- I think it would
be simpler to refer to the two kinds of downloads as user-initiated and
not-user-initiated.

http://codereview.chromium.org/1403001/diff/1/2#newcode1148
chrome/browser/download/download_manager.cc:1148: return
!is_user_triggered_download && is_dangerous_file;
It doesn't seem like this does what the bug describes. The bug says:

* Unprompted downloads are saved without a prompt iff:
** This is the first such download since the user has done an explicit 
navigation, AND
** The filetype is not "executable".

But here we are allowing the download if the filetype is executable, so long as
there was an explicit user gesture.

I am particularly not enthused about having this behavior for extensions.

[pierre.lafayette, 2010-03-28 06:13:56]

http://codereview.chromium.org/1403001/diff/1/2
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/1/2#newcode669
chrome/browser/download/download_manager.cc:669: // b) Unprompted downloads are
saved without a prompt iff:
On 2010/03/28 00:39:08, Aaron Boodman wrote:
> I think "unprompted download" is confusing terminology. IIUC, you're using it
to
> refer to a download that wasn't initiated by user-gesture, right? Since we're
> also talking about a different kind of prompt here -- the UI -- I think it
would
> be simpler to refer to the two kinds of downloads as user-initiated and
> not-user-initiated.

Sure that would indeed be clearer; this comment is actually taken from Peter's
description in the original bug (http://crbug.com/9044).

http://codereview.chromium.org/1403001/diff/1/2#newcode1148
chrome/browser/download/download_manager.cc:1148: return
!is_user_triggered_download && is_dangerous_file;
On 2010/03/28 00:39:08, Aaron Boodman wrote:
> It doesn't seem like this does what the bug describes. The bug says:
> 
> * Unprompted downloads are saved without a prompt iff:
> ** This is the first such download since the user has done an explicit 
> navigation, AND
> ** The filetype is not "executable".
> 
> But here we are allowing the download if the filetype is executable, so long
as
> there was an explicit user gesture.

You are forgetting the first part of the algorithm: 
a) User-triggered downloads are saved without a prompt.
I.e. all user-initiated downloads should go through without a prompt. The second
point is only for non user-initiated downloads and non-executable files. So for
the case of extensions, the prompt would always show up unless the user
initiated the download. The "first such download" part is covered by the
DownloadRequestManager.

> I am particularly not enthused about having this behavior for extensions.

Now that I think of it, perhaps you are correct. However, extensions already
have the dialog box prompt in addition to  the download bar prompt. But I do
think it makes sense that extensions should always have the download bar prompt.

[Aaron Boodman, 2010-03-28 06:25:39]

On 2010/03/28 06:13:56, pierre.lafayette wrote:
> Now that I think of it, perhaps you are correct. However, extensions already
> have the dialog box prompt in addition to  the download bar prompt. But I do
> think it makes sense that extensions should always have the download bar
prompt.

I don't want this change for extensions -- the behavior should stay the same as
it is now.

I don't think the change is good for executable files either, but that is less
my decision.

[pierre.lafayette, 2010-04-03 06:10:43]

Adding the navigation type information to the DownloadCreateInfo. This version
of the patch requires a small change in WebKit
(https://bugs.webkit.org/show_bug.cgi?id=37057). For now, this patch will add
everything we need to get this working once the WebKit dependency goes through.

Downloading extensions returns to the previous approach of always showing the
download prompt when the extension is not from the gallery.

Waiting for a decision on regarding the issues raised in http://crbug.com/9044
concerning DLLs and the like.

[Aaron Boodman, 2010-04-03 06:29:46]

lgtm wrt extensions

[Peter Kasting, 2010-07-27 19:27:33]

Going through my old reviews, this issue died.

It seems like the next step was supposed to be to address jcivelli's
asynchronicity concern.  The step after that was to split the "dangerous
filetype" list into two lists, one that can be no-prompt-downloaded from a user
gesture, the other that can't.  I need to check whether things other than DLL
are in the second list once we reach that point.

Pierre, are you able to resurrect this?

[pierre.lafayette, 2010-09-07 18:16:25]

On 2010/07/27 19:27:33, Peter Kasting wrote:
> Going through my old reviews, this issue died.
> 
> It seems like the next step was supposed to be to address jcivelli's
> asynchronicity concern.  The step after that was to split the "dangerous
> filetype" list into two lists, one that can be no-prompt-downloaded from a
user
> gesture, the other that can't.  I need to check whether things other than DLL
> are in the second list once we reach that point.
> 
> Pierre, are you able to resurrect this?

Sorry I didn't see this. The email got filtered out of my sight. 

There were objections to the WebKit side of this patch:
https://bugs.webkit.org/show_bug.cgi?id=37057. Without the NavigationType in the
request, I couldn't see a way to attach the state to the DownloadCreateInfo.
That and DLL issue caused this work to halt.

I'll look again, but at the time it didn't seem like there would be a way to
reliably get the click type without that WebKit change.

[Peter Kasting, 2010-09-07 18:22:53]

I suggest contacting someone like darin, abarth, or dglazkov to help you figure
out a solution to any loader problems.

[pierre.lafayette, 2010-10-21 05:17:02]

New patch. The issue with the previous patch was that resource requests and
navigations are separate concepts and as such it was incorrect to add
navigational state to the resource request. The webkit client should be
responsible for this. 

This implementation stores the user initiated state in the TabContents as well
as the provisional load URL associated with the user gesture. That way we can
verify that the download URL against the provisional load URL.

Based on discussions in http://crbug.com/9044, the algorithm for dangerous
downloads is now:

Downloads are considered dangerous if:
a) The file is a DLL.
b) The file is executable and the download was not user initiated.
c) They are an extension that is not from the gallery

We have defined a user initiated download as 3 possible cases:
a) A user enters a URL into the address bar that is a file
b) A user left clicks on a URL that is a file
c) A user right clicks and does "Save As" on a URL that is a file

JavaScript and meta refresh navigations should not count as user initiated. This
patch depends on the work done in https://svn.webkit.org/changeset/69924 and
https://bugs.webkit.org/show_bug.cgi?id=47817 to fix the user gesture state
handling in WebKit.

[Paweł Hajdan Jr., 2010-10-21 06:56:07]

Drive-by with download comments. Please let me take another look before
committing.

http://codereview.chromium.org/1403001/diff/49001/50001
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/1403001/diff/49001/50001#newcode723
chrome/browser/download/download_util.cc:723: bool is_user_initiated_download =
!contents ?
This is way too much confusing. Could you add something like bool
IsUserInitiatedDownload helper to the anonymous namespace?

http://codereview.chromium.org/1403001/diff/49001/50001#newcode735
chrome/browser/download/download_util.cc:735: && !is_user_initiated_download) {
nit: Please read the style guide how to format this.

[pierre.lafayette, 2010-10-21 13:52:11]

On 2010/10/21 06:56:07, Paweł Hajdan Jr. wrote:
> Drive-by with download comments. Please let me take another look before
> committing.
> 
> http://codereview.chromium.org/1403001/diff/49001/50001
> File chrome/browser/download/download_util.cc (right):
> 
> http://codereview.chromium.org/1403001/diff/49001/50001#newcode723
> chrome/browser/download/download_util.cc:723: bool is_user_initiated_download
=
> !contents ?
> This is way too much confusing. Could you add something like bool
> IsUserInitiatedDownload helper to the anonymous namespace?
>

Done. Plus there was a bug I had overlooked.
 
> http://codereview.chromium.org/1403001/diff/49001/50001#newcode735
> chrome/browser/download/download_util.cc:735: && !is_user_initiated_download)
{
> nit: Please read the style guide how to format this.

Done. Sometimes I get confused between WebKit and Chromium style :p

[Paweł Hajdan Jr., 2010-10-21 13:58:02]

Code I commented in the drive-by LGTM with a comment. Thanks!

http://codereview.chromium.org/1403001/diff/55001/56001
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/1403001/diff/55001/56001#newcode94
chrome/browser/download/download_util.cc:94: }
nit: // namespace

[brettw, 2010-10-22 20:04:50]

http://codereview.chromium.org/1403001/diff/62001/63006
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1403001/diff/62001/63006#newcode276
chrome/browser/tab_contents/tab_contents.h:276: const GURL&
provisional_load_url() const { return provisional_load_url_; }
The way that this currently works is that these values will be true for the rest
of the lifecycle of the page, even when it's not a provisional load. Do you need
this behavior, or can we somehow clear these out when the load commits?

What about when you type in the URL bar? Will the user initiated flag be correct
in this case? Since this flag comes from WebKit, it looks like it may not be
set, which I think would make this very-general-sounding flag potentially very
confusing.

[pierre.lafayette, 2010-10-23 07:53:22]

http://codereview.chromium.org/1403001/diff/62001/63006
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/1403001/diff/62001/63006#newcode276
chrome/browser/tab_contents/tab_contents.h:276: const GURL&
provisional_load_url() const { return provisional_load_url_; }
On 2010/10/22 20:04:51, brettw wrote:
> The way that this currently works is that these values will be true for the
rest
> of the lifecycle of the page, even when it's not a provisional load. Do you
need
> this behavior, or can we somehow clear these out when the load commits?
> 
RenderView::didCommitProvisionalLoad only gets called for web page navigations
and not for downloads but we can clear the provisional_load_url_ when the
download finishes in DownloadManager::OnAllDataSaved. Does that work?

> What about when you type in the URL bar? Will the user initiated flag be
correct
> in this case? Since this flag comes from WebKit, it looks like it may not be
> set, which I think would make this very-general-sounding flag potentially very
> confusing.

RenderView::OnNavigate, which gets called for address bar navigations, will call
WebFrameImpl::loadRequest which eventually will trigger a provisional load. So
the code path still gets hit. We can definitely change the name of
is_user_initiated_load_ to is_user_initiated_provisional_load_ since that will
be more clear.

[brettw, 2010-10-23 22:25:07]

On 2010/10/23 07:53:22, pierre.lafayette wrote:
> RenderView::OnNavigate, which gets called for address bar navigations, will
call
> WebFrameImpl::loadRequest which eventually will trigger a provisional load. So
> the code path still gets hit. We can definitely change the name of
> is_user_initiated_load_ to is_user_initiated_provisional_load_ since that will
> be more clear.

Sorry, I wasn't clear. I mean, will the value of the "user initiated" flag be
correct if I type in the URL bar? That load is user initiated, but for this code
path, I suspect it might just know about mouse clicks on the previous page
rather than user-initiated browser navigations. That's what I was concerned
about being confusing, since a URL bar navigation is definitely user initiated,
so it would be bad if the "user initiated navigation" flag was false for those
loads.

[pierre.lafayette, 2010-10-25 14:34:56]

On 2010/10/23 22:25:07, brettw wrote:
> On 2010/10/23 07:53:22, pierre.lafayette wrote:
> > RenderView::OnNavigate, which gets called for address bar navigations, will
> call
> > WebFrameImpl::loadRequest which eventually will trigger a provisional load.
So
> > the code path still gets hit. We can definitely change the name of
> > is_user_initiated_load_ to is_user_initiated_provisional_load_ since that
will
> > be more clear.
> 
> Sorry, I wasn't clear. I mean, will the value of the "user initiated" flag be
> correct if I type in the URL bar? That load is user initiated, but for this
code
> path, I suspect it might just know about mouse clicks on the previous page
> rather than user-initiated browser navigations. That's what I was concerned
> about being confusing, since a URL bar navigation is definitely user
initiated,
> so it would be bad if the "user initiated navigation" flag was false for those
> loads.

Downloads initiated by typing a URL into the address bar do set the user
initiated flag to true, i.e. WebFrameImpl::isProcessingUserGesture() will be
true. 

I've updated the patch to clear the load URL and the gesture state after a
download.

[brettw, 2010-11-01 23:01:18]

I talked to Peter for a while about this. He thought there was a WebKit bug
where you were trying to track user initiated flags on the download there but
some WebKit folks thought that it was a bad idea?

Adding this tracking to the browser side is a bit messy. It seems ideally to me
WebKit would know whether a download was the result of a user gesture and we
would just send that information when we know about the download to the download
manager. To what extent have you looked at doing this already?

BTW Sorry I took so long to get back to you.

http://codereview.chromium.org/1403001/diff/53002/68001
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/1403001/diff/53002/68001#newcode61
chrome/browser/download/download_manager.cc:61:
contents->set_is_user_initiated_provisional_load(false);
I don't think the download manager should be in charge of clearing this, it
seems like the provisional load URL and the user initiated flag should just
always be maintained for all URLs properly by the TabContents/RenderViewHost
layer.

[pierre.lafayette, 2010-11-02 17:43:08]

On 2010/11/01 23:01:18, brettw wrote:
> I talked to Peter for a while about this. He thought there was a WebKit bug
> where you were trying to track user initiated flags on the download there but
> some WebKit folks thought that it was a bad idea?
> 

Yes that was what I originally wanted to do in
https://bugs.webkit.org/show_bug.cgi?id=37057; which they didn't like because a
ResourceRequest isn't necessarily related to any particular navigation (e.g.
XMLHttpRequest). I spoke to Adam about it and he said: "Navigations and resource
requests are separate concepts. Why should the network stack care if something
is user-initiated? That seems like something the webkit client should care
about." Which makes sense. That's why I switched to tracking it on the browser
side.

> Adding this tracking to the browser side is a bit messy. It seems ideally to
me
> WebKit would know whether a download was the result of a user gesture and we
> would just send that information when we know about the download to the
download
> manager. To what extent have you looked at doing this already?
> 
> BTW Sorry I took so long to get back to you.
> 
> http://codereview.chromium.org/1403001/diff/53002/68001
> File chrome/browser/download/download_manager.cc (right):
> 
> http://codereview.chromium.org/1403001/diff/53002/68001#newcode61
> chrome/browser/download/download_manager.cc:61:
> contents->set_is_user_initiated_provisional_load(false);
> I don't think the download manager should be in charge of clearing this, it
> seems like the provisional load URL and the user initiated flag should just
> always be maintained for all URLs properly by the TabContents/RenderViewHost
> layer.

Actually we can just clear it in TabContents::OnStartDownload which happens
after the download gets committed to the history service. Does that work?

[pierre.lafayette, 2010-11-07 11:15:32]

This new patch puts the user gesture flag in the DownloadCreateInfo now that the
ResourceRequest have a m_hasUserGesture flag.

Depends on https://bugs.webkit.org/show_bug.cgi?id=37057 and
https://bugs.webkit.org/show_bug.cgi?id=47817.

[brettw, 2010-11-09 20:51:48]

Thanks a lot for spending the time to clean this up from the WebKit side. This
new patch is much nicer!

http://codereview.chromium.org/1403001/diff/119001/120001
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/1403001/diff/119001/120001#newcode732
chrome/browser/download/download_util.cc:732: } else if
(IsExecutableFile(info->suggested_path.BaseName()) &&
It would be nice if this could be a little bit more general. I don't think the
previous code was particularly nice, and now we're adding more complexity to it.

What do you think about this approach: Rename IsExecutableFile to
GetDownloadDangerLeverl (or something, feel free to suggest a better name) that
returns an enum with "not dangerous", "allow on user gesture", and "dangerous".
I guess the list in downloads/download_exe would instead have a pair of {
extension, enum value }.

[pierre.lafayette, 2010-11-11 14:40:15]

Made requested changes to add danger level enum for checking downloaded files.

[bkr, 2010-11-17 02:30:04]

Some comments about extensions

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:59: static const struct Executables {
jnlp is pretty shady and should likely be included here.  It's included by
default with JRE 1.4 and >.  http://en.wikipedia.org/wiki/Java_Web_Start says it
all

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:65: { "html", AllowOnUserGesture },
Anything that is rendered as html (htm, html, shtm, etc.) is pretty dangerous. 
Running local attacker controlled html usually means arbitrary file/cookie theft
(I guess chrome has some protection against this).  Running attacker controlled
content also allows gives an attacker an opportunity to bypass any other file
extension restriction through the use of an EMBED or OBJECT tag.  Most plugins
will ignore extensions if explicitly called from an EMBED or OBJECT.

The security models of browser plugins is complicated, especially when being run
from a local context.  For example, if I run local html which loads a local SWF
I'll have local file access regardless of chrome local html restrictions.  Using
a local html file to load the local SWF means I can use an arbitrary extension
for the SWF file.  The same is true with JAR files (although the consequences
are greater with JAR)

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:87: #if defined(OS_WIN)
chm (compiled help files)
chi (collection/index of chms)
lnk <--like shortcuts... but better ;)
mmc (management console files)

[Peter Kasting, 2010-11-17 02:36:07]

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:65: { "html", AllowOnUserGesture },
On 2010/11/17 02:30:04, bkr wrote:
> Anything that is rendered as html (htm, html, shtm, etc.) is pretty dangerous.

It's at worst as dangerous as .exe and other "has to be manually run" types,
though, which means AllowOnUserGesture is sufficient.

The "Dangerous" value should be reserved for things that are dangerous just
sitting on the drive, like DLLs.

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:87: #if defined(OS_WIN)
On 2010/11/17 02:30:04, bkr wrote:
> chm (compiled help files)

That one is present already.

[brettw, 2010-11-18 02:13:29]

I'm currently on vacation, will look at this next week. Sorry for the
delay, please feel free to Ping me if you don't hear back next Tues.

Brett

On Thursday, November 11, 2010,  <pierre.lafayette@gmail.com> wrote:
> Made requested changes to add danger level enum for checking downloaded files.
>
> http://codereview.chromium.org/1403001/
>

[Chris Evans, 2010-11-19 02:55:53]

Some initial comments on which file types are really dangerous.

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:66: { "jar", AllowOnUserGesture },
Mark jar (as well as jnlp) dangerous.

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:76: { "rb", AllowOnUserGesture },
.pl .py .rb and any other script extensions -> dangerous.

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:96: { "bat", AllowOnUserGesture },
bat -> dangerous surely?

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:99: { "com", AllowOnUserGesture },
com == exe, no? -> dangerous!!

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:103: { "exe", AllowOnUserGesture },
Yowch. exe is terribly dangerous :)

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:131: { "mht", AllowOnUserGesture },
I don't know what half of these Windows things are. We need a Windows security
expert to cast an eye over the list before we land this.

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:166: { "xbap", AllowOnUserGesture },
xbap is a disaster! -> dangerous

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:170: { "dmg", AllowOnUserGesture },
I don't know much about Mac. Does it adequately warn upon clicking on a
downloaded app / dmg in the download shelf?

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:176: { "exe", AllowOnUserGesture },
exe seems to be repeated here?

http://codereview.chromium.org/1403001/diff/132001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:179: { "sh", AllowOnUserGesture },
.sh is certainly dangeous, as is .ksh, .tcsh, etc.
Plus, what about a Windows system with Cygwin installed? Does that add file
mappings for .sh etc. on the Windows system?

[Peter Kasting, 2010-11-19 02:58:11]

On 2010/11/19 02:55:53, Chris Evans wrote:
> Some initial comments on which file types are really dangerous.

Almost all of these changes defeat the entire point of this change.  Please see
my message on the email thread about this.

[brettw, 2010-11-22 22:43:02]

LGTM. Peter, do you have any more comments or is this good to go?

Pierre: Do you think you can do a follow-up patch to rename download_exe to
something more meaningful, like maybe download_extensions.cc? It doesn't seem
like the old name applies any more, but I don't want to delay this super-long
patch any longer.

It also seems like the headers for the functions in that file should be in their
own download_extensions.h header rather than in download_util with all the rest
of the stuff.

[Peter Kasting, 2010-11-22 23:07:01]

If you're OK, I'm OK.  I likely don't understand the plumbing anyway.

[pierre.lafayette, 2010-11-23 14:06:09]

On 2010/11/22 22:43:02, brettw wrote:
> LGTM. Peter, do you have any more comments or is this good to go?
> 
> Pierre: Do you think you can do a follow-up patch to rename download_exe to
> something more meaningful, like maybe download_extensions.cc? It doesn't seem
> like the old name applies any more, but I don't want to delay this super-long
> patch any longer.
> 
> It also seems like the headers for the functions in that file should be in
their
> own download_extensions.h header rather than in download_util with all the
rest
> of the stuff.

Done.

[pierre.lafayette, 2010-11-24 18:57:16]

On 2010/11/22 22:43:02, brettw wrote:
> LGTM. Peter, do you have any more comments or is this good to go?
> 
> Pierre: Do you think you can do a follow-up patch to rename download_exe to

I guess you meant do a separate CR for the refactoring. Whoops. This still needs
a review for those changes.


> something more meaningful, like maybe download_extensions.cc? It doesn't seem
> like the old name applies any more, but I don't want to delay this super-long
> patch any longer.
> 
> It also seems like the headers for the functions in that file should be in
their
> own download_extensions.h header rather than in download_util with all the
rest
> of the stuff.

[brettw, 2010-11-24 21:57:29]

I don't see the gyp changes for the new file add/removes. Did you forget to add
them to this CL?

http://codereview.chromium.org/1403001/diff/149001/chrome/browser/download/do...
File chrome/browser/download/download_exe.cc (left):

http://codereview.chromium.org/1403001/diff/149001/chrome/browser/download/do...
chrome/browser/download/download_exe.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
This file doesn't say "D" for deleted, just make sure it's actually deleted in
your CL.

[pierre.lafayette, 2010-11-25 03:56:06]

On 2010/11/24 21:57:29, brettw wrote:
> I don't see the gyp changes for the new file add/removes. Did you forget to
add
> them to this CL?
> 

Right. Added.

>
http://codereview.chromium.org/1403001/diff/149001/chrome/browser/download/do...
> File chrome/browser/download/download_exe.cc (left):
> 
>
http://codereview.chromium.org/1403001/diff/149001/chrome/browser/download/do...
> chrome/browser/download/download_exe.cc:1: // Copyright (c) 2010 The Chromium
> Authors. All rights reserved.
> This file doesn't say "D" for deleted, just make sure it's actually deleted in
> your CL.

It may be because git sees a rename from download_exe.cc to
download_extensions.cc. If you look at the raw patch, it looks fine.

[brettw, 2010-11-25 17:04:50]

LGTM

[pierre.lafayette, 2010-11-25 18:47:15]

On 2010/11/25 17:04:50, brettw wrote:
> LGTM

Thanks for the reviews! Needs a land :)

[cevans, 2010-11-28 21:29:19]

On Thu, Nov 25, 2010 at 10:47 AM, <pierre.lafayette@gmail.com> wrote:

> On 2010/11/25 17:04:50, brettw wrote:
>
>> LGTM
>>
>
> Thanks for the reviews! Needs a land :)


Let's give this just a couple more days whilst we finalize if there are any
additional file extensions that deserve to go on the "dangerous" list.

Cheers
Chris


>
>
> http://codereview.chromium.org/1403001/
>

[pierre.lafayette, 2010-12-02 17:05:51]

On 2010/11/28 21:29:19, cevans_google.com wrote:
> On Thu, Nov 25, 2010 at 10:47 AM, <mailto:pierre.lafayette@gmail.com> wrote:
> 
> > On 2010/11/25 17:04:50, brettw wrote:
> >
> >> LGTM
> >>
> >
> > Thanks for the reviews! Needs a land :)
> 
> 
> Let's give this just a couple more days whilst we finalize if there are any
> additional file extensions that deserve to go on the "dangerous" list.

Find anything?

> 
> Cheers
> Chris
> 
> 
> >
> >
> > http://codereview.chromium.org/1403001/
> >

[bkr, 2010-12-02 17:42:24]

.sys should also be added to the dangerous list.  It is has DLL like
behaviors

BK


On Thu, Dec 2, 2010 at 9:05 AM, <pierre.lafayette@gmail.com> wrote:

> On 2010/11/28 21:29:19, cevans_google.com wrote:
>
>  On Thu, Nov 25, 2010 at 10:47 AM, <mailto:pierre.lafayette@gmail.com>
>> wrote:
>>
>
>  > On 2010/11/25 17:04:50, brettw wrote:
>> >
>> >> LGTM
>> >>
>> >
>> > Thanks for the reviews! Needs a land :)
>>
>
>
>  Let's give this just a couple more days whilst we finalize if there are
>> any
>> additional file extensions that deserve to go on the "dangerous" list.
>>
>
> Find anything?
>
>
>
>  Cheers
>> Chris
>>
>
>
>  >
>> >
>> > http://codereview.chromium.org/1403001/
>> >
>>
>
>
>
> http://codereview.chromium.org/1403001/
>

[Peter Kasting, 2010-12-02 19:06:02]

On 2010/12/02 17:42:24, bkr wrote:
> .sys should also be added to the dangerous list.  It is has DLL like
> behaviors

Also .drv.

[pierre.lafayette, 2010-12-03 14:56:42]

On 2010/12/02 19:06:02, Peter Kasting wrote:
> On 2010/12/02 17:42:24, bkr wrote:
> > .sys should also be added to the dangerous list.  It is has DLL like
> > behaviors
> 
> Also .drv.

Done.

[Peter Kasting, 2010-12-03 17:19:45]

bkr: Is landing this kosher?

[bkr, 2010-12-03 23:11:29]

So, if we're treating files that are dangerous merely by the fact that they
are on the local file system (which I believe is the intent of the CL) then
these are the right files for now:  .dll, .drv, .sys, .xbap

I hope we're not lulled into a sense that we're protected from 0-click
attacks just because we've changed the behaviors for downloading files that
can be used in passive attacks made possible by the operating system
(dll/drv/sys preloading, or automatic previewing of xbap).  There are other
scenarios (which I discussed during out lunch meeting) that involve 0-click
interaction with the user + various plugins that will result in compromise
of a user.  These attacks are made possible due to Chrome's auto download
capabilities and loose defenses from browser plugins.  I think the most
classic example is Java.  If I can plant a class file or a JAR file onto the
local filesystem (to a location that I can guess), I can use java to load
that class/JAR file.  Java doesn't have a strict respect file extensions, so
I can basically use any file extension in place of .class/.jar.  After we
bashed on Java for a bit, but these behaviors are not limited to Java.  The
same behis true for numerous other plugins, exacerbated by the fact that I
can typically specify which program should handle the file through an OBJECT
or EMBED (by explicitly setting the TYPE parameter to the plugin I want to
load the file).  I think someone at the lunch stated it best:

"When it comes to plugins, all file extensions are dangerous"

Chrome's stack resists these attacks very well, but when we introduce
plugins (I have real attacks using java, Flash, and PDF reader) our
cumulative defenses can be bypassed.  As long as there are plugins that
don't enforce the same strict remote/local defenses as chrome, we will be
vulnerable to these types of attacks (some of which require no user
interaction, other than browsing to a web page).  I also believe Justin
summed our current state very well.  Chrome has a great long term strategy
for plugins.  A strategy that will help prevent these attacks... however
we're a longs ways out before we'll see actual implementation (realistically
~2 years).  I'm wondering what we can do until we wrangle in plugin
behavior.

Billy


On Fri, Dec 3, 2010 at 9:19 AM, <pkasting@chromium.org> wrote:

> bkr: Is landing this kosher?
>
>
> http://codereview.chromium.org/1403001/
>

[Peter Kasting, 2010-12-06 22:22:27]

I'm closing this in favor of http://codereview.chromium.org/5603008/ , where I'm
running this through the trybots in preparation for landing.