Added dangerous download prompting.

This CL adds prompting for dangerous types of files (executable) when they are automatically downloaded.

The file is saved with a temporary name (dangerous_download_xxxx.download) in the download directory and the user is presented (in the download shelf and the download tab if opened) with a warning message and buttons to save/discard the download.
If discarded the download is removed (and its file deleted).
If saved, download goes as usual.
Dangerous downloads not confirmed by the user are deleted on shutdown.

TEST=Download a small exe file, try using the save/discard button from the download shelf and from the download tab (the intent is that the file has been entirely downloaded by the time you take action). Try again with a slow/big download (that time the download is expected not to be finished when approved/discarded).



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3228

[willdye_gmail.com, 2008-10-01 22:11:33]

I'm concerned that this feature will clutter up the base code with anti-malware
heuristics.  Such efforts tend to be rather platform-specific (some consider it
a misfeature that Windows ever trusted filename extensions).  Furthermore, the
efforts tend to be fuzzy, never-ending battles as the malware producers come up
with new tricks, prompting yet another software update.  For example, a while
back malware coders discovered that screensaver files could be used for
mischief, but the extension was not as widely recognized as .exe and .bat as
being dangerous.  Another problem is that if a Windows program becomes popular
that uses, say, ".spam" for program files, then ".spam" effectively becomes a
dangerous extension, and Chrome will be under pressure to add it to the list of
dangerous file names.

At the risk of treating every problem with a new layer of abstraction, shouldn't
this problem be isolated and delegated in some way?  The core might have a few
hooks in place for things that can't be handled by extensions or plugins, but
nothing else.  Worst case, the "dangerous filename" code should be isolated into
platform-specific code, since filename extensions are generally not trusted in,
say, Linux.

[Paul Godavari, 2008-10-07 01:29:17]

http://codereview.chromium.org/6043/diff/69/268
File base/file_util.h (right):

http://codereview.chromium.org/6043/diff/69/268#newcode221
Line 221: // Same as CreateTemporaryFileName bu the file is created in |dir|.
'but'

http://codereview.chromium.org/6043/diff/69/266
File base/file_util_win.cc (right):

http://codereview.chromium.org/6043/diff/69/266#newcode387
Line 387: bool CreateTemporaryFileNameInDir(const std::wstring& dir,
There should also be an implementation (or stub) of this in file_util_posix.cc.

http://codereview.chromium.org/6043/diff/69/269
File base/string_util.cc (right):

http://codereview.chromium.org/6043/diff/69/269#newcode1464
Line 1464: int lstr_len = rstr_len + (((max_len - 3) % 2 != 0) ? 1 : 0);
This could just be:
int lstr_len = rstr_len + ((max_len - 3) % 2);

http://codereview.chromium.org/6043/diff/69/285
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6043/diff/69/285#newcode340
Line 340: } else if (download->state() == DownloadItem::IN_PROGRESS) {
I wonder if this check is necessary since we're iterating over the in_progress_
map (and everything in there should be "not complete")?

http://codereview.chromium.org/6043/diff/69/285#newcode549
Line 549: base::RandInt(0, 100000));
Maybe a better way to do this instead of looping is to have Windows generate a
random name for that directory, and then splice it into the file name:
L"dangerous_download_%s.download"

http://codereview.chromium.org/6043/diff/69/285#newcode692
Line 692: if (it != in_progress_.end()) {
If you change this to "it == in_progress_.end()", then you can return early and
remove one layer of indentation, which will be easier to read.

http://codereview.chromium.org/6043/diff/69/286
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6043/diff/69/286#newcode172
Line 172: void set_file_name(const std::wstring& name) { file_name_ = name; }
I think this should be in getter/setter order.

http://codereview.chromium.org/6043/diff/69/286#newcode462
Line 462: bool IsDangerous(const std::wstring& file_name);
This can be static.

http://codereview.chromium.org/6043/diff/69/274
File chrome/browser/views/download_item_view.cc (right):

http://codereview.chromium.org/6043/diff/69/274#newcode439
Line 439: x, box_y_, box_height_, drop_down_image_set->top->width());
Is this second call necessary if we've just done it a few lines above?

http://codereview.chromium.org/6043/diff/69/276
File chrome/browser/views/download_tab_view.cc (right):

http://codereview.chromium.org/6043/diff/69/276#newcode436
Line 436: file_name_->SetText(model_->GetFileName());
Do you need to call GetFileName() for other layout states?

http://codereview.chromium.org/6043/diff/69/276#newcode699
Line 699: bool DownloadItemTabView::OnMousePressed(const
ChromeViews::MouseEvent& event) {
This function should prevent the right click context menu from appearing when
the mode is dangerous.

http://codereview.chromium.org/6043/diff/69/277
File chrome/browser/views/download_tab_view.h (right):

http://codereview.chromium.org/6043/diff/69/277#newcode177
Line 177: // Clears the list of dangeours downloads and removes this
DownloadTabView
'dangerous'

http://codereview.chromium.org/6043/diff/69/292
File chrome/views/native_button.h (right):

http://codereview.chromium.org/6043/diff/69/292#newcode87
Line 87: void ForcePaint(ChromeCanvas* canvas);
Implementation?

[jcampan, 2008-10-07 21:28:33]

New snapshot uploaded.

Thanks.

http://codereview.chromium.org/6043/diff/69/268
File base/file_util.h (right):

http://codereview.chromium.org/6043/diff/69/268#newcode221
Line 221: // Same as CreateTemporaryFileName bu the file is created in |dir|.
On 2008/10/07 01:29:17, paul wrote:
> 'but'
> 

Done.

http://codereview.chromium.org/6043/diff/69/266
File base/file_util_win.cc (right):

http://codereview.chromium.org/6043/diff/69/266#newcode387
Line 387: bool CreateTemporaryFileNameInDir(const std::wstring& dir,
On 2008/10/07 01:29:17, paul wrote:
> There should also be an implementation (or stub) of this in
file_util_posix.cc.

OK, created a stub.

http://codereview.chromium.org/6043/diff/69/269
File base/string_util.cc (right):

http://codereview.chromium.org/6043/diff/69/269#newcode1464
Line 1464: int lstr_len = rstr_len + (((max_len - 3) % 2 != 0) ? 1 : 0);
On 2008/10/07 01:29:17, paul wrote:
> This could just be:
> int lstr_len = rstr_len + ((max_len - 3) % 2);
> 

Done.

http://codereview.chromium.org/6043/diff/69/285
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6043/diff/69/285#newcode340
Line 340: } else if (download->state() == DownloadItem::IN_PROGRESS) {
On 2008/10/07 01:29:17, paul wrote:
> I wonder if this check is necessary since we're iterating over the
in_progress_
> map (and everything in there should be "not complete")?

Good point. I kept it because it was there before.
I replaced with a DCHECK.

http://codereview.chromium.org/6043/diff/69/285#newcode549
Line 549: base::RandInt(0, 100000));
On 2008/10/07 01:29:17, paul wrote:
> Maybe a better way to do this instead of looping is to have Windows generate a
> random name for that directory, and then splice it into the file name:
> L"dangerous_download_%s.download"

I tried to use the Windows API to create a temporary file that takes a prefix,
but the prefix is limited to 3 chars. I could generate a temporary name and then
use it to create the file name I am going to use, but I was just worried that
the Windows API might return the same name since this is not the file I am
creating in the end.

http://codereview.chromium.org/6043/diff/69/285#newcode692
Line 692: if (it != in_progress_.end()) {
On 2008/10/07 01:29:17, paul wrote:
> If you change this to "it == in_progress_.end()", then you can return early
and
> remove one layer of indentation, which will be easier to read.

Done.

http://codereview.chromium.org/6043/diff/69/286
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6043/diff/69/286#newcode172
Line 172: void set_file_name(const std::wstring& name) { file_name_ = name; }
On 2008/10/07 01:29:17, paul wrote:
> I think this should be in getter/setter order.

Done.

http://codereview.chromium.org/6043/diff/69/286#newcode462
Line 462: bool IsDangerous(const std::wstring& file_name);
On 2008/10/07 01:29:17, paul wrote:
> This can be static.
Can it? It is calling IsExecutable() which is not static (it uses exe_types_).

http://codereview.chromium.org/6043/diff/69/274
File chrome/browser/views/download_item_view.cc (right):

http://codereview.chromium.org/6043/diff/69/274#newcode439
Line 439: x, box_y_, box_height_, drop_down_image_set->top->width());
On 2008/10/07 01:29:17, paul wrote:
> Is this second call necessary if we've just done it a few lines above?

I did not modify it as it was there before. I think it is needed for the
drop-over animation

http://codereview.chromium.org/6043/diff/69/276
File chrome/browser/views/download_tab_view.cc (right):

http://codereview.chromium.org/6043/diff/69/276#newcode436
Line 436: file_name_->SetText(model_->GetFileName());
On 2008/10/07 01:29:17, paul wrote:
> Do you need to call GetFileName() for other layout states?
I don't think so.
Once the layout state is complete or canceled we can go with file_name() as the
name is available.

http://codereview.chromium.org/6043/diff/69/276#newcode699
Line 699: bool DownloadItemTabView::OnMousePressed(const
ChromeViews::MouseEvent& event) {
On 2008/10/07 01:29:17, paul wrote:
> This function should prevent the right click context menu from appearing when
> the mode is dangerous.

Done.

http://codereview.chromium.org/6043/diff/69/277
File chrome/browser/views/download_tab_view.h (right):

http://codereview.chromium.org/6043/diff/69/277#newcode177
Line 177: // Clears the list of dangeours downloads and removes this
DownloadTabView
On 2008/10/07 01:29:17, paul wrote:
> 'dangerous'

Done.

http://codereview.chromium.org/6043/diff/69/292
File chrome/views/native_button.h (right):

http://codereview.chromium.org/6043/diff/69/292#newcode87
Line 87: void ForcePaint(ChromeCanvas* canvas);
On 2008/10/07 01:29:17, paul wrote:
> Implementation?

Actually it is not used.  I removed it.

[Paul Godavari, 2008-10-09 23:47:54]

LGTM.

http://codereview.chromium.org/6043/diff/69/286
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6043/diff/69/286#newcode462
Line 462: bool IsDangerous(const std::wstring& file_name);
You're right, I was looking at IsExecutableMimeType.

[developer0420, 2008-10-10 01:30:09]

Microsoft also lists the extension .asx (Windows Media Audio / Video) as
potentially dangerous extension, which I did not see in
chrome/browser/download/download_exe.cc.

[Mark Larson, 2008-10-10 20:48:17]

http://codereview.chromium.org/6043/diff/639/465
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6043/diff/639/465#newcode548
Line 548: SStringPrintf(&file_name, L"dangerous_download_%d.download",
Let's not use DANGER! DANGER! for these files. The name should not be alarming.

We should use something like unconfirmed_%d.download.

Someone with more user sympathy might have a better idea.