diff options
author | gbillock@chromium.org <gbillock@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-10 13:49:48 +0000 |
---|---|---|
committer | gbillock@chromium.org <gbillock@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-08-10 13:49:48 +0000 |
commit | 492c00974a1354e97e27939279eecd7fc9d8d9ce (patch) | |
tree | 899e3da8b00bb60209a012a0587564b50477c017 | |
parent | 6cf51b6c67396f11d6e4c2cccb75e8ed3a020e37 (diff) | |
download | chromium_src-492c00974a1354e97e27939279eecd7fc9d8d9ce.zip chromium_src-492c00974a1354e97e27939279eecd7fc9d8d9ce.tar.gz chromium_src-492c00974a1354e97e27939279eecd7fc9d8d9ce.tar.bz2 |
[Downloads] Move client guid for AV scanning of downloaded files to chrome/
This client app should not be used by all clients of content/. Moving to chrome so it can be managed there and re-used if needed.
Review URL: https://chromiumcodereview.appspot.com/21355004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@216843 0039d316-1c4b-4281-b951-d872f2087c98
19 files changed, 106 insertions, 110 deletions
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index a810537..aec10bb 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -34,6 +34,7 @@ #include "chrome/browser/platform_util.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/common/chrome_constants.h" #include "chrome/common/pref_names.h" #include "components/user_prefs/pref_registry_syncable.h" #include "content/public/browser/download_item.h" @@ -402,6 +403,11 @@ void ChromeDownloadManagerDelegate::CheckForFileExistence( callback); } +std::string +ChromeDownloadManagerDelegate::ApplicationClientIdForFileScanning() const { + return std::string(chrome::kApplicationClientIDStringForAVScanning); +} + DownloadProtectionService* ChromeDownloadManagerDelegate::GetDownloadProtectionService() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index e2c13ad..d23337d 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -93,6 +93,7 @@ class ChromeDownloadManagerDelegate virtual void CheckForFileExistence( content::DownloadItem* download, const content::CheckForFileExistenceCallback& callback) OVERRIDE; + virtual std::string ApplicationClientIdForFileScanning() const OVERRIDE; DownloadPrefs* download_prefs() { return download_prefs_.get(); } diff --git a/chrome/common/chrome_constants.cc b/chrome/common/chrome_constants.cc index 874a039..40621f8 100644 --- a/chrome/common/chrome_constants.cc +++ b/chrome/common/chrome_constants.cc @@ -253,6 +253,12 @@ const wchar_t kLaunchModeValue[] = L"launch_mode"; const char kProfileDirPrefix[] = "u-"; #endif +// This GUID is associated with any 'don't ask me again' settings that the +// user can select for different file types. +// {2676A9A2-D919-4FEE-9187-152100393AB2} +const char kApplicationClientIDStringForAVScanning[] = + "2676A9A2-D919-4FEE-9187-152100393AB2"; + } // namespace chrome #undef FPL diff --git a/chrome/common/chrome_constants.h b/chrome/common/chrome_constants.h index 6775c80..cd97f7c 100644 --- a/chrome/common/chrome_constants.h +++ b/chrome/common/chrome_constants.h @@ -139,6 +139,9 @@ extern const wchar_t kLaunchModeValue[]; extern const char kProfileDirPrefix[]; #endif +// Used to identify the application to the system AV function in Windows. +extern const char kApplicationClientIDStringForAVScanning[]; + } // namespace chrome #endif // CHROME_COMMON_CHROME_CONSTANTS_H_ diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc index 466be32..90ab485 100644 --- a/content/browser/download/base_file.cc +++ b/content/browser/download/base_file.cc @@ -211,6 +211,10 @@ void BaseFile::Finish() { Close(); } +void BaseFile::SetClientGuid(const std::string& guid) { + client_guid_ = guid; +} + // OS_WIN, OS_MACOSX and OS_LINUX have specialized implementations. #if !defined(OS_WIN) && !defined(OS_MACOSX) && !defined(OS_LINUX) DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { diff --git a/content/browser/download/base_file.h b/content/browser/download/base_file.h index 9c1261c..f1686c8 100644 --- a/content/browser/download/base_file.h +++ b/content/browser/download/base_file.h @@ -69,8 +69,15 @@ class CONTENT_EXPORT BaseFile { // Indicate that the download has finished. No new data will be received. void Finish(); + // Set the client guid which will be used to identify the app to the + // system AV scanning function. Should be called before + // AnnotateWithSourceInformation() to take effect. + void SetClientGuid(const std::string& guid); + // Informs the OS that this file came from the internet. Returns a // DownloadInterruptReason indicating the result of the operation. + // Note: SetClientGuid() should be called before this function on + // Windows to ensure the correct app client ID is available. DownloadInterruptReason AnnotateWithSourceInformation(); base::FilePath full_path() const { return full_path_; } @@ -142,6 +149,8 @@ class CONTENT_EXPORT BaseFile { // The URL where the download was initiated. GURL referrer_url_; + std::string client_guid_; + // OS file stream for writing scoped_ptr<net::FileStream> file_stream_; diff --git a/content/browser/download/base_file_win.cc b/content/browser/download/base_file_win.cc index 398e5fc..252f0495 100644 --- a/content/browser/download/base_file_win.cc +++ b/content/browser/download/base_file_win.cc @@ -5,9 +5,12 @@ #include "content/browser/download/base_file.h" #include <windows.h> +#include <cguid.h> +#include <objbase.h> #include <shellapi.h> #include "base/file_util.h" +#include "base/guid.h" #include "base/metrics/histogram.h" #include "base/strings/utf_string_conversions.h" #include "base/threading/thread_restrictions.h" @@ -326,7 +329,16 @@ DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() { bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED); DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE; - HRESULT hr = ScanAndSaveDownloadedFile(full_path_, source_url_); + std::string braces_guid = "{" + client_guid_ + "}"; + GUID guid = GUID_NULL; + if (base::IsValidGUID(client_guid_)) { + HRESULT hr = CLSIDFromString( + base::UTF8ToUTF16(braces_guid).c_str(), &guid); + if (FAILED(hr)) + guid = GUID_NULL; + } + + HRESULT hr = AVScanFile(full_path_, source_url_.spec(), guid); // If the download file is missing after the call, then treat this as an // interrupted download. diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h index a8e18ff..568e7ee 100644 --- a/content/browser/download/download_file.h +++ b/content/browser/download/download_file.h @@ -73,6 +73,11 @@ class CONTENT_EXPORT DownloadFile { // Returns the current (intermediate) state of the hash as a byte string. virtual std::string GetHashState() = 0; + // Set the application GUID to be used to identify the app to the + // system AV function when scanning downloaded files. Should be called + // before RenameAndAnnotate() to take effect. + virtual void SetClientGuid(const std::string& guid) = 0; + // For testing. Must be called on FILE thread. // TODO(rdsmith): Replace use of EnsureNoPendingDownloads() // on the DownloadManager with a test-specific DownloadFileFactory diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc index bbe317b..67bee81 100644 --- a/content/browser/download/download_file_impl.cc +++ b/content/browser/download/download_file_impl.cc @@ -200,6 +200,10 @@ std::string DownloadFileImpl::GetHashState() { return file_.GetHashState(); } +void DownloadFileImpl::SetClientGuid(const std::string& guid) { + file_.SetClientGuid(guid); +} + void DownloadFileImpl::StreamActive() { base::TimeTicks start(base::TimeTicks::Now()); base::TimeTicks now; diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h index 963ae9f..9a50870 100644 --- a/content/browser/download/download_file_impl.h +++ b/content/browser/download/download_file_impl.h @@ -63,6 +63,7 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public DownloadFile { virtual int64 CurrentSpeed() const OVERRIDE; virtual bool GetHash(std::string* hash) OVERRIDE; virtual std::string GetHashState() OVERRIDE; + virtual void SetClientGuid(const std::string& guid) OVERRIDE; protected: // For test class overrides. diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc index ebf501d..867b76f 100644 --- a/content/browser/download/download_file_unittest.cc +++ b/content/browser/download/download_file_unittest.cc @@ -138,6 +138,8 @@ class DownloadFileTest : public testing::Test { net::BoundNetLog(), scoped_ptr<PowerSaveBlocker>().Pass(), observer_factory_.GetWeakPtr())); + download_file_->SetClientGuid( + "12345678-ABCD-1234-DCBA-123456789ABC"); EXPECT_CALL(*input_stream_, Read(_, _)) .WillOnce(Return(ByteStreamReader::STREAM_EMPTY)) diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc index f9241832..ccfb4cc 100644 --- a/content/browser/download/download_manager_impl.cc +++ b/content/browser/download/download_manager_impl.cc @@ -422,6 +422,13 @@ void DownloadManagerImpl::StartDownloadWithId( delegate_->GenerateFileHash(), stream.Pass(), download->GetBoundNetLog(), download->DestinationObserverAsWeakPtr())); + + // Attach the client ID identifying the app to the AV system. + if (download_file.get() && delegate_) { + download_file->SetClientGuid( + delegate_->ApplicationClientIdForFileScanning()); + } + scoped_ptr<DownloadRequestHandleInterface> req_handle( new DownloadRequestHandle(info->request_handle)); download->Start(download_file.Pass(), req_handle.Pass()); diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc index a8e18d4..eba2833 100644 --- a/content/browser/download/download_manager_impl_unittest.cc +++ b/content/browser/download/download_manager_impl_unittest.cc @@ -195,6 +195,7 @@ class MockDownloadManagerDelegate : public DownloadManagerDelegate { MOCK_METHOD5(ChooseSavePath, void( WebContents*, const base::FilePath&, const base::FilePath::StringType&, bool, const SavePackagePathPickedCallback&)); + MOCK_CONST_METHOD0(ApplicationClientIdForFileScanning, std::string()); }; MockDownloadManagerDelegate::MockDownloadManagerDelegate() {} @@ -363,7 +364,7 @@ class MockDownloadFileFactory virtual ~MockDownloadFileFactory() {} // Overridden method from DownloadFileFactory - MOCK_METHOD8(MockCreateFile, DownloadFile*( + MOCK_METHOD8(MockCreateFile, MockDownloadFile*( const DownloadSaveInfo&, const base::FilePath&, const GURL&, const GURL&, bool, @@ -593,9 +594,15 @@ TEST_F(DownloadManagerTest, StartDownload) { EXPECT_CALL(GetMockDownloadManagerDelegate(), GetSaveDir(_, _, _, _)); EXPECT_CALL(GetMockDownloadManagerDelegate(), GenerateFileHash()) .WillOnce(Return(true)); + EXPECT_CALL(GetMockDownloadManagerDelegate(), + ApplicationClientIdForFileScanning()) + .WillRepeatedly(Return("client-id")); + MockDownloadFile* mock_file = new MockDownloadFile; + EXPECT_CALL(*mock_file, SetClientGuid("client-id")); EXPECT_CALL(*mock_download_file_factory_.get(), MockCreateFile(Ref(*info->save_info.get()), _, _, _, true, - stream.get(), _, _)); + stream.get(), _, _)) + .WillOnce(Return(mock_file)); download_manager_->StartDownload( info.Pass(), stream.Pass(), DownloadUrlParameters::OnStartedCallback()); diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h index 74dc75c..5d4b270 100644 --- a/content/browser/download/mock_download_file.h +++ b/content/browser/download/mock_download_file.h @@ -48,6 +48,7 @@ class MockDownloadFile : virtual public DownloadFile { MOCK_CONST_METHOD0(Id, int()); MOCK_METHOD0(GetDownloadManager, DownloadManager*()); MOCK_CONST_METHOD0(DebugString, std::string()); + MOCK_METHOD1(SetClientGuid, void(const std::string&)); }; } // namespace content diff --git a/content/browser/download/save_file.cc b/content/browser/download/save_file.cc index 80caf13..e8885e4 100644 --- a/content/browser/download/save_file.cc +++ b/content/browser/download/save_file.cc @@ -60,6 +60,8 @@ void SaveFile::Finish() { } void SaveFile::AnnotateWithSourceInformation() { + // TODO(gbillock): If this method is called, it should set the + // file_.SetClientGuid() method first. file_.AnnotateWithSourceInformation(); } diff --git a/content/browser/safe_util_win.cc b/content/browser/safe_util_win.cc index 2dce2ca..ac077f1 100644 --- a/content/browser/safe_util_win.cc +++ b/content/browser/safe_util_win.cc @@ -19,12 +19,6 @@ namespace content { namespace { -// This GUID is associated with any 'don't ask me again' settings that the -// user can select for different file types. -// {2676A9A2-D919-4fee-9187-152100393AB2} -static const GUID kClientID = { 0x2676a9a2, 0xd919, 0x4fee, - { 0x91, 0x87, 0x15, 0x21, 0x0, 0x39, 0x3a, 0xb2 } }; - // 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. @@ -55,72 +49,11 @@ bool SetInternetZoneIdentifierDirectly(const base::FilePath& full_path) { return true; } -} - -// This function implementation is based on the attachment execution -// services functionally deployed with IE6 or Service pack 2. This -// functionality is exposed in the IAttachmentExecute COM interface. -// more information at: -// http://msdn2.microsoft.com/en-us/library/ms647048.aspx -bool SaferOpenItemViaShell(HWND hwnd, const std::wstring& window_title, - const base::FilePath& full_path, - const std::wstring& 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; - } - return ui::win::OpenItemViaShell(full_path); - } - - attachment_services->SetClientGuid(kClientID); - - if (!window_title.empty()) - attachment_services->SetClientTitle(window_title.c_str()); - - // To help windows decide if the downloaded file is dangerous we can provide - // what the documentation calls evidence. Which we provide now: - // - // Set the file itself as evidence. - hr = attachment_services->SetLocalPath(full_path.value().c_str()); - if (FAILED(hr)) - return false; - // Set the origin URL as evidence. - hr = attachment_services->SetSource(source_url.c_str()); - if (FAILED(hr)) - return false; +} // namespace - // Now check the windows policy. - if (attachment_services->CheckPolicy() != S_OK) { - // It is possible that the above call returns an undocumented result - // equal to 0x800c000e which seems to indicate that the URL failed the - // the security check. If you proceed with the Prompt() call the - // Shell might show a dialog that says: - // "windows found that this file is potentially harmful. To help protect - // your computer, Windows has blocked access to this file." - // Upon dismissal of the dialog windows will delete the file (!!). - // So, we can 'return' in that case but maybe is best to let it happen to - // fail on the safe side. - - ATTACHMENT_ACTION action; - // We cannot control what the prompt says or does directly but it - // is a pretty decent dialog; for example, if an executable is signed it can - // decode and show the publisher and the certificate. - hr = attachment_services->Prompt(hwnd, ATTACHMENT_PROMPT_EXEC, &action); - if (FAILED(hr) || (ATTACHMENT_ACTION_CANCEL == action)) { - // The user has declined opening the item. - return false; - } - } - return ui::win::OpenItemViaShellNoZoneCheck(full_path); -} - -HRESULT ScanAndSaveDownloadedFile(const base::FilePath& full_path, - const GURL& source_url) { +HRESULT AVScanFile(const base::FilePath& full_path, + const std::string& source_url, + const GUID& client_guid) { base::win::ScopedComPtr<IAttachmentExecute> attachment_services; HRESULT hr = attachment_services.CreateInstance(CLSID_AttachmentServices); @@ -135,15 +68,20 @@ HRESULT ScanAndSaveDownloadedFile(const base::FilePath& full_path, return hr; } - hr = attachment_services->SetClientGuid(kClientID); - if (FAILED(hr)) - return hr; + if (!IsEqualGUID(client_guid, GUID_NULL)) { + hr = attachment_services->SetClientGuid(client_guid); + if (FAILED(hr)) + return hr; + } hr = attachment_services->SetLocalPath(full_path.value().c_str()); if (FAILED(hr)) return hr; - hr = attachment_services->SetSource(UTF8ToWide(source_url.spec()).c_str()); + // Note: SetSource looks like it needs to be called, even if empty. + // Docs say it is optional, but it appears not calling it at all sets + // a zone that is too restrictive. + hr = attachment_services->SetSource(UTF8ToWide(source_url).c_str()); if (FAILED(hr)) return hr; diff --git a/content/browser/safe_util_win.h b/content/browser/safe_util_win.h index 1636748..3860429 100644 --- a/content/browser/safe_util_win.h +++ b/content/browser/safe_util_win.h @@ -16,34 +16,6 @@ class FilePath; namespace content { -// Open or run a downloaded file via the Windows shell, possibly showing first -// a consent dialog if the the file is deemed dangerous. This function is an -// enhancement over the OpenItemViaShell() function of win_util.h. -// -// The user consent dialog will be shown or not according to the windows -// execution policy defined in the registry which can be overridden per user. -// The mechanics of the policy are explained in the Microsoft Knowledge base -// number 883260: http://support.microsoft.com/kb/883260 -// -// The 'hwnd' is the handle to the parent window. In case a dialog is displayed -// the parent window will be disabled since the dialog is meant to be modal. -// The 'window_title' is the text displayed on the title bar of the dialog. If -// you pass an empty string the dialog will have a generic 'windows security' -// name on the title bar. -// -// You must provide a valid 'full_path' to the file to be opened and a well -// formed url in 'source_url'. The url should identify the source of the file -// but does not have to be network-reachable. If the url is malformed a -// dialog will be shown telling the user that the file will be blocked. -// -// In the event that there is no default application registered for the file -// specified by 'full_path' it ask the user, via the Windows "Open With" -// dialog. -// Returns 'true' on successful open, 'false' otherwise. -bool SaferOpenItemViaShell(HWND hwnd, const std::wstring& window_title, - const base::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: @@ -67,10 +39,15 @@ bool SaferOpenItemViaShell(HWND hwnd, const std::wstring& window_title, // 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 base::FilePath& full_path, - const GURL& source_url); +// path of the download. Must be present. +// |source_url|: the source URL for the download. If empty, the source will +// not be set. +// |client_guid|: the GUID to be set in the IAttachmentExecute client slot. +// Used to identify the app to the system AV function. +// If GUID_NULL is passed, no client GUID is set. +HRESULT AVScanFile(const base::FilePath& full_path, + const std::string& source_url, + const GUID& client_guid); } // namespace content #endif // CONTENT_COMMON_SAFE_UTIL_WIN_H_ diff --git a/content/public/browser/download_manager_delegate.cc b/content/public/browser/download_manager_delegate.cc index c33fcd4..9351ce2 100644 --- a/content/public/browser/download_manager_delegate.cc +++ b/content/public/browser/download_manager_delegate.cc @@ -38,6 +38,11 @@ bool DownloadManagerDelegate::GenerateFileHash() { return false; } +std::string +DownloadManagerDelegate::ApplicationClientIdForFileScanning() const { + return std::string(); +} + DownloadManagerDelegate::~DownloadManagerDelegate() {} } // namespace content diff --git a/content/public/browser/download_manager_delegate.h b/content/public/browser/download_manager_delegate.h index 98bad43..89be2df 100644 --- a/content/public/browser/download_manager_delegate.h +++ b/content/public/browser/download_manager_delegate.h @@ -129,6 +129,12 @@ class CONTENT_EXPORT DownloadManagerDelegate { DownloadItem* download, const CheckForFileExistenceCallback& callback) {} + // Return a GUID string used for identifying the application to the + // system AV function for scanning downloaded files. If an empty + // or invalid GUID string is returned, no client identification + // will be given to the AV function. + virtual std::string ApplicationClientIdForFileScanning() const; + protected: virtual ~DownloadManagerDelegate(); }; |