diff options
23 files changed, 221 insertions, 90 deletions
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 70af813..8757178 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -2640,7 +2640,23 @@ Psst! Incognito mode <ph name="SHORTCUT_KEY">$1<ex>(Ctrl+Shift+N)</ex></ph> may </message> <message name="IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_VIRUS" desc="Download page message: Virus."> - This file was blocked by anti-virus software. + Anti-virus software detected a virus. + </message> + <message name="IDS_DOWNLOAD_INTERRUPTED_STATUS_BLOCKED" + desc="The download was blocked due to security policy."> + Blocked + </message> + <message name="IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_BLOCKED" + desc="Download page message: The download was blocked due to security policy on this machine."> + Security settings on your computer blocked this file. + </message> + <message name="IDS_DOWNLOAD_INTERRUPTED_STATUS_SECURITY_CHECK_FAILED" + desc="The security check performed on this download failed unexpectedly"> + Virus Scan Failed + </message> + <message name="IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_SECURITY_CHECK_FAILED" + desc="Download page message: The security check performed on this download failed unexpectedly"> + Anti-virus software failed unexpectedly while scanning this file. </message> <message name="IDS_DOWNLOAD_INTERRUPTED_STATUS_NETWORK_TIMEOUT" desc="Network Timeout."> diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc index feaced0..ee7cd57 100644 --- a/chrome/browser/download/download_item_model.cc +++ b/chrome/browser/download/download_item_model.cc @@ -302,6 +302,12 @@ string16 BaseDownloadItemModel::InterruptReasonStatusMessage(int reason) { case content::DOWNLOAD_INTERRUPT_REASON_FILE_VIRUS_INFECTED: string_id = IDS_DOWNLOAD_INTERRUPTED_STATUS_VIRUS; break; + case content::DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED: + string_id = IDS_DOWNLOAD_INTERRUPTED_STATUS_BLOCKED; + break; + case content::DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED: + string_id = IDS_DOWNLOAD_INTERRUPTED_STATUS_SECURITY_CHECK_FAILED; + break; case content::DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR: string_id = IDS_DOWNLOAD_INTERRUPTED_STATUS_TEMPORARY_PROBLEM; break; @@ -362,6 +368,12 @@ string16 BaseDownloadItemModel::InterruptReasonMessage(int reason) { case content::DOWNLOAD_INTERRUPT_REASON_FILE_VIRUS_INFECTED: string_id = IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_VIRUS; break; + case content::DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED: + string_id = IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_BLOCKED; + break; + case content::DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED: + string_id = IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_SECURITY_CHECK_FAILED; + break; case content::DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR: string_id = IDS_DOWNLOAD_INTERRUPTED_DESCRIPTION_TEMPORARY_PROBLEM; break; diff --git a/chrome/browser/download/download_item_model_unittest.cc b/chrome/browser/download/download_item_model_unittest.cc index e2a704e..816c1f5 100644 --- a/chrome/browser/download/download_item_model_unittest.cc +++ b/chrome/browser/download/download_item_model_unittest.cc @@ -145,6 +145,10 @@ TEST_F(DownloadItemModelTest, InterruptedStatus) { "1/2 B File Too Large" }, { content::DOWNLOAD_INTERRUPT_REASON_FILE_VIRUS_INFECTED, "1/2 B Virus Detected" }, + { content::DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED, + "1/2 B Blocked" }, + { content::DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED, + "1/2 B Virus Scan Failed" }, { content::DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR, "1/2 B System Busy" }, { content::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED, @@ -209,6 +213,10 @@ TEST_F(DownloadItemModelTest, InterruptTooltip) { "foo.bar\nFile Too Large" }, { content::DOWNLOAD_INTERRUPT_REASON_FILE_VIRUS_INFECTED, "foo.bar\nVirus Detected" }, + { content::DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED, + "foo.bar\nBlocked" }, + { content::DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED, + "foo.bar\nVirus Scan Failed" }, { content::DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR, "foo.bar\nSystem Busy" }, { content::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED, diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc index 7207e1d..9e055ce 100644 --- a/content/browser/download/base_file.cc +++ b/content/browser/download/base_file.cc @@ -213,7 +213,8 @@ void BaseFile::Finish() { // OS_WIN, OS_MACOSX and OS_LINUX have specialized implementations. #if !defined(OS_WIN) && !defined(OS_MACOSX) && !defined(OS_LINUX) -void BaseFile::AnnotateWithSourceInformation() { +DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { + return DOWNLOAD_INTERRUPT_REASON_NONE; } #endif diff --git a/content/browser/download/base_file.h b/content/browser/download/base_file.h index d8bfecc..f10f043 100644 --- a/content/browser/download/base_file.h +++ b/content/browser/download/base_file.h @@ -69,8 +69,9 @@ class CONTENT_EXPORT BaseFile { // Indicate that the download has finished. No new data will be received. void Finish(); - // Informs the OS that this file came from the internet. - void AnnotateWithSourceInformation(); + // Informs the OS that this file came from the internet. Returns a + // DownloadInterruptReason indicating the result of the operation. + DownloadInterruptReason AnnotateWithSourceInformation(); // Calculate and return the current speed in bytes per second. int64 CurrentSpeed() const; diff --git a/content/browser/download/base_file_linux.cc b/content/browser/download/base_file_linux.cc index 07215b0..ee8d1fa 100644 --- a/content/browser/download/base_file_linux.cc +++ b/content/browser/download/base_file_linux.cc @@ -9,11 +9,12 @@ namespace content { -void BaseFile::AnnotateWithSourceInformation() { +DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DCHECK(!detached_); AddOriginMetadataToFile(full_path_, source_url_, referrer_url_); + return DOWNLOAD_INTERRUPT_REASON_NONE; } } // namespace content diff --git a/content/browser/download/base_file_mac.cc b/content/browser/download/base_file_mac.cc index e3ce2d7..21e6b55 100644 --- a/content/browser/download/base_file_mac.cc +++ b/content/browser/download/base_file_mac.cc @@ -9,12 +9,13 @@ namespace content { -void BaseFile::AnnotateWithSourceInformation() { +DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DCHECK(!detached_); AddQuarantineMetadataToFile(full_path_, source_url_, referrer_url_); AddOriginMetadataToFile(full_path_, source_url_, referrer_url_); + return DOWNLOAD_INTERRUPT_REASON_NONE; } } // namespace content diff --git a/content/browser/download/base_file_win.cc b/content/browser/download/base_file_win.cc index a98597d..b1c0dcd 100644 --- a/content/browser/download/base_file_win.cc +++ b/content/browser/download/base_file_win.cc @@ -7,9 +7,11 @@ #include <windows.h> #include <shellapi.h> +#include "base/file_util.h" #include "base/threading/thread_restrictions.h" #include "base/utf_string_conversions.h" #include "content/browser/download/download_interrupt_reasons_impl.h" +#include "content/browser/download/download_stats.h" #include "content/browser/safe_util_win.h" #include "content/public/browser/browser_thread.h" @@ -142,6 +144,35 @@ DownloadInterruptReason MapShFileOperationCodes(int code) { net::MapSystemError(code), DOWNLOAD_INTERRUPT_FROM_DISK); } +// Maps a return code from ScanAndSaveDownloadedFile() to a +// DownloadInterruptReason. The return code in |result| is usually from the +// final IAttachmentExecute::Save() call. +DownloadInterruptReason MapScanAndSaveErrorCodeToInterruptReason( + HRESULT result) { + if (SUCCEEDED(result)) + return DOWNLOAD_INTERRUPT_REASON_NONE; + + switch (result) { + case INET_E_SECURITY_PROBLEM: // 0x800c000e + // This is returned if the download was blocked due to security + // restrictions. E.g. if the source URL was in the Restricted Sites zone + // and downloads are blocked on that zone, then the download would be + // deleted and this error code is returned. + return DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED; + + case E_FAIL: // 0x80004005 + // Returned if an anti-virus product reports an infection in the + // downloaded file during IAE::Save(). + return DOWNLOAD_INTERRUPT_REASON_FILE_VIRUS_INFECTED; + + default: + // Any other error that occurs during IAttachmentExecute::Save() likely + // indicates a problem with the security check, but not necessarily the + // download. See http://crbug.com/153212. + return DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED; + } +} + } // namespace // Renames a file using the SHFileOperation API to ensure that the target file @@ -178,14 +209,36 @@ DownloadInterruptReason BaseFile::MoveFileAndAdjustPermissions( return interrupt_reason; } -void BaseFile::AnnotateWithSourceInformation() { +DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DCHECK(!detached_); - // Sets the Zone to tell Windows that this file comes from the internet. - // We ignore the return value because a failure is not fatal. - win_util::SetInternetZoneIdentifier(full_path_, - UTF8ToWide(source_url_.spec())); + bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED); + DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE; + HRESULT hr = win_util::ScanAndSaveDownloadedFile(full_path_, source_url_); + + // If the download file is missing after the call, then treat this as an + // interrupted download. + // + // If the ScanAndSaveDownloadedFile() call failed, but the downloaded file is + // still around, then don't interrupt the download. Attachment Execution + // Services deletes the submitted file if the downloaded file is blocked by + // policy or if it was found to be infected. + // + // If the file is still there, then the error could be due to AES not being + // available or some other error during the AES invocation. In either case, + // we don't surface the error to the user. + if (!file_util::PathExists(full_path_)) { + DCHECK(FAILED(hr)); + result = MapScanAndSaveErrorCodeToInterruptReason(hr); + if (result == DOWNLOAD_INTERRUPT_REASON_NONE) { + RecordDownloadCount(FILE_MISSING_AFTER_SUCCESSFUL_SCAN_COUNT); + result = DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED; + } + LogInterruptReason("ScanAndSaveDownloadedFile", hr, result); + } + bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED); + return result; } } // namespace content diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc index 65c1d9e..8630841 100644 --- a/content/browser/download/download_browsertest.cc +++ b/content/browser/download/download_browsertest.cc @@ -97,7 +97,7 @@ class DownloadFileWithDelay : public DownloadFileImpl { // Wraps DownloadFileImpl::Detach and intercepts the return callback, // storing it in the factory that produced this object for later // retrieval. - virtual void Detach(base::Closure callback) OVERRIDE; + virtual void Detach(const DetachCompletionCallback& callback) OVERRIDE; private: static void RenameCallbackWrapper( @@ -108,7 +108,8 @@ class DownloadFileWithDelay : public DownloadFileImpl { static void DetachCallbackWrapper( DownloadFileWithDelayFactory* factory, - const base::Closure& original_callback); + const DetachCompletionCallback& original_callback, + DownloadInterruptReason interrupt_reason); // This variable may only be read on the FILE thread, and may only be // indirected through (e.g. methods on DownloadFileWithDelayFactory called) @@ -186,7 +187,7 @@ void DownloadFileWithDelay::Rename(const FilePath& full_path, owner_, callback)); } -void DownloadFileWithDelay::Detach(base::Closure callback) { +void DownloadFileWithDelay::Detach(const DetachCompletionCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DownloadFileImpl::Detach( base::Bind(DownloadFileWithDelay::DetachCallbackWrapper, @@ -206,9 +207,10 @@ void DownloadFileWithDelay::RenameCallbackWrapper( // static void DownloadFileWithDelay::DetachCallbackWrapper( DownloadFileWithDelayFactory* factory, - const base::Closure& original_callback) { + const DetachCompletionCallback& original_callback, + DownloadInterruptReason interrupt_reason) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - factory->AddDetachCallback(original_callback); + factory->AddDetachCallback(base::Bind(original_callback, interrupt_reason)); } DownloadFileWithDelayFactory::DownloadFileWithDelayFactory() diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h index 1bbde02..7e57d21 100644 --- a/content/browser/download/download_file.h +++ b/content/browser/download/download_file.h @@ -36,6 +36,11 @@ class CONTENT_EXPORT DownloadFile { typedef base::Callback<void(DownloadInterruptReason reason, const FilePath& path)> RenameCompletionCallback; + // Callback used with Detach(). On success, |reason| will be + // DOWNLOAD_INTERRUPT_REASON_NONE. + typedef base::Callback<void(DownloadInterruptReason reason)> + DetachCompletionCallback; + virtual ~DownloadFile() {} // Returns DOWNLOAD_INTERRUPT_REASON_NONE on success, or a network @@ -54,14 +59,11 @@ class CONTENT_EXPORT DownloadFile { // Detach the file so it is not deleted on destruction. // |callback| will be called on the UI thread after detach. - virtual void Detach(base::Closure callback) = 0; + virtual void Detach(const DetachCompletionCallback& callback) = 0; // Abort the download and automatically close the file. virtual void Cancel() = 0; - // Informs the OS that this file came from the internet. - virtual void AnnotateWithSourceInformation() = 0; - virtual FilePath FullPath() const = 0; virtual bool InProgress() const = 0; virtual int64 BytesSoFar() const = 0; diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc index 802fec5..92f5110 100644 --- a/content/browser/download/download_file_impl.cc +++ b/content/browser/download/download_file_impl.cc @@ -132,29 +132,24 @@ void DownloadFileImpl::Rename(const FilePath& full_path, base::Bind(callback, reason, new_path)); } -void DownloadFileImpl::Detach(base::Closure callback) { +void DownloadFileImpl::Detach(const DetachCompletionCallback& callback) { // Doing the annotation here leaves a small window during // which the file has the final name but hasn't been marked with the // Mark Of The Web. However, it allows anti-virus scanners on Windows // to actually see the data (http://crbug.com/127999), and the Window // is pretty small (round trip to the UI thread). - AnnotateWithSourceInformation(); - + DownloadInterruptReason interrupt_reason = + file_.AnnotateWithSourceInformation(); file_.Detach(); - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, interrupt_reason)); } void DownloadFileImpl::Cancel() { file_.Cancel(); } -void DownloadFileImpl::AnnotateWithSourceInformation() { - bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED); - file_.AnnotateWithSourceInformation(); - bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED); -} - FilePath DownloadFileImpl::FullPath() const { return file_.full_path(); } diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h index 05ff9a7..12a8074 100644 --- a/content/browser/download/download_file_impl.h +++ b/content/browser/download/download_file_impl.h @@ -53,9 +53,8 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public DownloadFile { virtual void Rename(const FilePath& full_path, bool overwrite_existing_file, const RenameCompletionCallback& callback) OVERRIDE; - virtual void Detach(base::Closure callback) OVERRIDE; + virtual void Detach(const DetachCompletionCallback& callback) OVERRIDE; virtual void Cancel() OVERRIDE; - virtual void AnnotateWithSourceInformation() OVERRIDE; virtual FilePath FullPath() const OVERRIDE; virtual bool InProgress() const OVERRIDE; virtual int64 BytesSoFar() const OVERRIDE; diff --git a/content/browser/download/download_interrupt_reasons_impl.cc b/content/browser/download/download_interrupt_reasons_impl.cc index b3ee11e..540bbca 100644 --- a/content/browser/download/download_interrupt_reasons_impl.cc +++ b/content/browser/download/download_interrupt_reasons_impl.cc @@ -44,6 +44,9 @@ DownloadInterruptReason ConvertNetErrorToInterruptReason( // The file has a virus. FILE_ERROR_TO_INTERRUPT_REASON(FILE_VIRUS_INFECTED, VIRUS_INFECTED) + // The file was blocked by local policy. + FILE_ERROR_TO_INTERRUPT_REASON(BLOCKED_BY_CLIENT, BLOCKED) + // Network errors. // The network operation timed out. diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc index 01d0e8c..f1bd114 100644 --- a/content/browser/download/download_item_impl.cc +++ b/content/browser/download/download_item_impl.cc @@ -100,7 +100,8 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface { // takes ownership of the DownloadFile and hence implicitly destroys it // at the end of the function. static void DownloadFileDetach( - scoped_ptr<DownloadFile> download_file, base::Closure callback) { + scoped_ptr<DownloadFile> download_file, + const DownloadFile::DetachCompletionCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); download_file->Detach(callback); } @@ -827,7 +828,7 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { // interrupts to race with cancels. // Whatever happens, the first one to hit the UI thread wins. - if (state_ != IN_PROGRESS_INTERNAL) + if (state_ != IN_PROGRESS_INTERNAL && state_ != COMPLETING_INTERNAL) return; last_reason_ = reason; @@ -1179,7 +1180,7 @@ void DownloadItemImpl::ReadyForDownloadCompletionDone() { if (is_save_package_download_) { // Avoid doing anything on the file thread; there's nothing we control // there. - OnDownloadFileReleased(); + OnDownloadFileReleased(DOWNLOAD_INTERRUPT_REASON_NONE); return; } @@ -1246,7 +1247,11 @@ void DownloadItemImpl::ReleaseDownloadFile() { TransitionTo(COMPLETING_INTERNAL); } -void DownloadItemImpl::OnDownloadFileReleased() { +void DownloadItemImpl::OnDownloadFileReleased(DownloadInterruptReason reason) { + if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) { + Interrupt(reason); + return; + } if (delegate_->ShouldOpenDownload(this)) Completed(); else @@ -1290,8 +1295,9 @@ void DownloadItemImpl::Completed() { void DownloadItemImpl::CancelDownloadFile() { // TODO(rdsmith/benjhayden): Remove condition as part of // SavePackage integration. - if (!is_save_package_download_) { - DCHECK(download_file_.get()); + // download_file_ can be NULL if Interrupt() is called after the download file + // has been released. + if (!is_save_package_download_ && download_file_.get()) { BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, // Will be deleted at end of task execution. diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h index c49677c..e086655 100644 --- a/content/browser/download/download_item_impl.h +++ b/content/browser/download/download_item_impl.h @@ -208,10 +208,10 @@ class CONTENT_EXPORT DownloadItemImpl // TODO(rdsmith): Put in state variable for file name determination. IN_PROGRESS_INTERNAL, - // Between commit point (dispatch of download file release) and - // completed. Embedder may be opening the file in this state. - // Note that the DownloadItem may be deleted (by shutdown) in this - // state. + // Between commit point (dispatch of download file release) and completed. + // Embedder may be opening the file in this state. Note that the + // DownloadItem may be deleted (by shutdown) or interrupted (e.g. due to a + // failure during AnnotateWithSourceInformation()) in this state. COMPLETING_INTERNAL, // After embedder has had a chance to auto-open. User may now open @@ -279,7 +279,9 @@ class CONTENT_EXPORT DownloadItemImpl void ReleaseDownloadFile(); - void OnDownloadFileReleased(); + // TODO(rdsmith,asanka): Move the AnnotateWithSourceInformation() call to the + // final rename and eliminate the interrupt reason callback. + void OnDownloadFileReleased(DownloadInterruptReason reason); // Called when the entire download operation (including renaming etc) // is completed. diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc index bedab09..2f5ba7a 100644 --- a/content/browser/download/download_item_impl_unittest.cc +++ b/content/browser/download/download_item_impl_unittest.cc @@ -78,16 +78,13 @@ ACTION_P(ScheduleRenameCallback, new_path) { // Schedules a task to invoke the input closure on // the UI thread. Should only be used as the action for -// MockDownloadFile::Detach/Cancel as follows: +// MockDownloadFile::Detach as follows: // EXPECT_CALL(download_file, Detach(_)) -// .WillOnce(ScheduleClosure())); -ACTION(ScheduleClosure) { - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, arg0); -} - -// Similarly for scheduling a completion callback. -ACTION(ScheduleCompleteCallback) { - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(arg1)); +// .WillOnce(ScheduleDetachCallback())); +ACTION(ScheduleDetachCallback) { + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + base::Bind(arg0, DOWNLOAD_INTERRUPT_REASON_NONE)); } } // namespace @@ -508,7 +505,7 @@ TEST_F(DownloadItemTest, CallbackAfterRename) { EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item)) .WillOnce(Return(true)); EXPECT_CALL(*download_file, Detach(_)) - .WillOnce(ScheduleClosure()); + .WillOnce(ScheduleDetachCallback()); item->SetIsPersisted(); item->MaybeCompleteDownload(); RunAllPendingInMessageLoops(); diff --git a/content/browser/download/download_net_log_parameters.h b/content/browser/download/download_net_log_parameters.h index 92432c1..6461fce 100644 --- a/content/browser/download/download_net_log_parameters.h +++ b/content/browser/download/download_net_log_parameters.h @@ -80,6 +80,7 @@ base::Value* FileErrorNetLogCallback(const char* operation, net::Error net_error, net::NetLog::LogLevel log_level); +// Returns NetLog parameters for a download interruption. base::Value* FileInterruptedNetLogCallback(const char* operation, int os_error, DownloadInterruptReason reason, diff --git a/content/browser/download/download_stats.h b/content/browser/download/download_stats.h index 3e5feb4..3bd4740 100644 --- a/content/browser/download/download_stats.h +++ b/content/browser/download/download_stats.h @@ -63,6 +63,10 @@ enum DownloadCountTypes { // progress. APPEND_TO_DETACHED_FILE_COUNT, + // Counts the number of instances where the downloaded file is missing after a + // successful invocation of win_util::ScanAndSaveDownloadedFile(). + FILE_MISSING_AFTER_SUCCESSFUL_SCAN_COUNT, + DOWNLOAD_COUNT_TYPES_LAST_ENTRY }; diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h index ae85b5e..f7c2477 100644 --- a/content/browser/download/mock_download_file.h +++ b/content/browser/download/mock_download_file.h @@ -32,10 +32,9 @@ class MockDownloadFile : virtual public DownloadFile { MOCK_METHOD3(Rename, void(const FilePath& full_path, bool overwrite_existing_file, const RenameCompletionCallback& callback)); - MOCK_METHOD1(Detach, void(base::Closure)); + MOCK_METHOD1(Detach, void(const DetachCompletionCallback& callback)); MOCK_METHOD0(Cancel, void()); MOCK_METHOD0(Finish, void()); - MOCK_METHOD0(AnnotateWithSourceInformation, void()); MOCK_CONST_METHOD0(FullPath, FilePath()); MOCK_CONST_METHOD0(InProgress, bool()); MOCK_CONST_METHOD0(BytesSoFar, int64()); diff --git a/content/browser/safe_util_win.cc b/content/browser/safe_util_win.cc index 7aa699f..cc32022 100644 --- a/content/browser/safe_util_win.cc +++ b/content/browser/safe_util_win.cc @@ -11,7 +11,9 @@ #include "base/logging.h" #include "base/path_service.h" #include "base/string_util.h" +#include "base/utf_string_conversions.h" #include "base/win/scoped_comptr.h" +#include "googleurl/src/gurl.h" #include "ui/base/win/shell.h" namespace { @@ -22,8 +24,12 @@ namespace { static const GUID kClientID = { 0x2676a9a2, 0xd919, 0x4fee, { 0x91, 0x87, 0x15, 0x21, 0x0, 0x39, 0x3a, 0xb2 } }; -// Directly writes the ZoneIdentifier stream, without using the -// IAttachmentExecute service. +// Sets the Zone Identifier on the file to "Internet" (3). Returns true if the +// function succeeds, false otherwise. A failure is expected on system where +// the Zone Identifier is not supported, like a machine with a FAT32 filesystem. +// This function does not invoke Windows Attachment Execution Services. +// +// |full_path| is the path to the downloaded file. bool SetInternetZoneIdentifierDirectly(const FilePath& full_path) { const DWORD kShare = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; std::wstring path = full_path.value() + L":Zone.Identifier"; @@ -36,8 +42,7 @@ bool SetInternetZoneIdentifierDirectly(const FilePath& full_path) { // Don't include trailing null in data written. static const DWORD kIdentifierSize = arraysize(kIdentifier) - 1; DWORD written = 0; - BOOL result = WriteFile(file, kIdentifier, kIdentifierSize, &written, - NULL); + BOOL result = WriteFile(file, kIdentifier, kIdentifierSize, &written, NULL); BOOL flush_result = FlushFileBuffers(file); CloseHandle(file); @@ -115,41 +120,37 @@ bool SaferOpenItemViaShell(HWND hwnd, const std::wstring& window_title, return ui::win::OpenItemViaShellNoZoneCheck(full_path); } -bool SetInternetZoneIdentifier(const FilePath& full_path, - const std::wstring& source_url) { +HRESULT ScanAndSaveDownloadedFile(const FilePath& full_path, + const GURL& source_url) { base::win::ScopedComPtr<IAttachmentExecute> attachment_services; HRESULT hr = attachment_services.CreateInstance(CLSID_AttachmentServices); if (FAILED(hr)) { - // We don't have Attachment Execution Services, it must be a pre-XP.SP2 - // Windows installation, or the thread does not have COM initialized. - if (hr == CO_E_NOTINITIALIZED) { - NOTREACHED(); - return false; - } + // The thread must have COM initialized. + DCHECK_NE(CO_E_NOTINITIALIZED, hr); - // Write the ZoneIdentifier file directly. - return SetInternetZoneIdentifierDirectly(full_path); + // We don't have Attachment Execution Services, it must be a pre-XP.SP2 + // Windows installation, or the thread does not have COM initialized. Try to + // set the zone information directly. Failure is not considered an error. + SetInternetZoneIdentifierDirectly(full_path); + return hr; } hr = attachment_services->SetClientGuid(kClientID); if (FAILED(hr)) - return false; + return hr; hr = attachment_services->SetLocalPath(full_path.value().c_str()); if (FAILED(hr)) - return false; + return hr; - // Source is necessary for files ending in ".tmp" to avoid error 0x800c000e. - hr = attachment_services->SetSource(source_url.c_str()); + hr = attachment_services->SetSource(UTF8ToWide(source_url.spec()).c_str()); if (FAILED(hr)) - return false; + return hr; - hr = attachment_services->Save(); - if (FAILED(hr)) - return false; - - return true; + // A failure in the Save() call below could result in the downloaded file + // being deleted. + return attachment_services->Save(); } } // namespace win_util diff --git a/content/browser/safe_util_win.h b/content/browser/safe_util_win.h index 8b21f7f..41acfd5 100644 --- a/content/browser/safe_util_win.h +++ b/content/browser/safe_util_win.h @@ -9,6 +9,7 @@ #include <windows.h> class FilePath; +class GURL; namespace win_util { @@ -40,16 +41,33 @@ bool SaferOpenItemViaShell(HWND hwnd, const std::wstring& window_title, const FilePath& full_path, const std::wstring& source_url); -// Sets the Zone Identifier on the file to "Internet" (3). Returns true if the -// function succeeds, false otherwise. A failure is expected on system where -// the Zone Identifier is not supported, like a machine with a FAT32 filesystem. -// It should not be considered fatal. -// -// |full_path| is the path to save the file to, and -// |source_url| is the URL where the file was downloaded from. -bool SetInternetZoneIdentifier(const FilePath& full_path, - const std::wstring& source_url); - +// Invokes IAttachmentExecute::Save to validate the downloaded file. The call +// may scan the file for viruses and if necessary, annotate it with evidence. As +// a result of the validation, the file may be deleted. See: +// http://msdn.microsoft.com/en-us/bb776299 +// +// If Attachment Execution Services is unavailable, then this function will +// attempt to manually annotate the file with security zone information. A +// failure code will be returned in this case even if the file is sucessfully +// annotated. +// +// IAE::Save() will delete the file if it was found to be blocked by local +// security policy or if it was found to be infected. The call may also delete +// the file due to other failures (http://crbug.com/153212). A failure code will +// be returned in these cases. +// +// Typical return values: +// S_OK : The file was okay. If any viruses were found, they were cleaned. +// E_FAIL : Virus infected. +// INET_E_SECURITY_PROBLEM : The file was blocked due to security policy. +// +// Any other return value indicates an unexpected error during the scan. +// +// |full_path| : is the path to the downloaded file. This should be the final +// path of the download. +// |source_url|: the source URL for the download. +HRESULT ScanAndSaveDownloadedFile(const FilePath& full_path, + const GURL& source_url); } // namespace win_util #endif // CONTENT_COMMON_SAFE_UTIL_WIN_H_ diff --git a/content/public/browser/download_interrupt_reason_values.h b/content/public/browser/download_interrupt_reason_values.h index cd124bf..088052f 100644 --- a/content/public/browser/download_interrupt_reason_values.h +++ b/content/public/browser/download_interrupt_reason_values.h @@ -36,6 +36,13 @@ INTERRUPT_REASON(FILE_VIRUS_INFECTED, 7) // "Temporary Problem". INTERRUPT_REASON(FILE_TRANSIENT_ERROR, 10) +// The file was blocked due to local policy. +// "Blocked" +INTERRUPT_REASON(FILE_BLOCKED, 11) + +// An attempt to check the safety of the download failed due to unexpected +// reasons. See http://crbug.com/153212. +INTERRUPT_REASON(FILE_SECURITY_CHECK_FAILED, 12) // Network errors. diff --git a/net/base/net_log_event_type_list.h b/net/base/net_log_event_type_list.h index 3e64574..661fb6c 100644 --- a/net/base/net_log_event_type_list.h +++ b/net/base/net_log_event_type_list.h @@ -1588,7 +1588,7 @@ EVENT_TYPE(DOWNLOAD_ITEM_RENAMED) // This event is created when a download item is interrupted. // { -// "reason": <The reason for the interruption>, +// "interrupt_reason": <The reason for the interruption>, // "bytes_so_far": <Number of bytes received>, // "hash_state": <Current hash state, as a hex-encoded binary string>, // } @@ -1665,6 +1665,8 @@ EVENT_TYPE(DOWNLOAD_FILE_DELETED) // { // "operation": <open, write, close, etc>, // "net_error": <net::Error code>, +// "os_error": <OS depedent error code> +// "interrupt_reason": <Download interrupt reason> // } EVENT_TYPE(DOWNLOAD_FILE_ERROR) |