[Downloads] Move client guid for AV scanning of downloaded files to chrome/

content/browser/download/base_file_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/base_file.h"
 
+#include <objbase.h>
 #include <windows.h>

[Lei Zhang, 2013/08/07 01:03:05]

nit: I think <windows.h> usually goes first?

[Greg Billock, 2013/08/07 17:00:56]

we got some of both. I'll change it here though.

+#include <cguid.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"
 #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"
 
 namespace content {
 namespace {
 
 const int kAllSpecialShFileOperationCodes[] = {
   // Should be kept in sync with the case statement below.
   ERROR_ACCESS_DENIED,
   0x71,
   0x72,
@@ skipping 292 matching lines @@
     return LogInterruptReason("SHFileOperation", result, interrupt_reason);
   return interrupt_reason;
 }
 
 DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(!detached_);
 
   bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED);
   DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE;
-  HRESULT hr = ScanAndSaveDownloadedFile(full_path_, source_url_);
+  GUID guid = GUID_NULL;
+  if (!client_guid_.empty() && base::IsValidGUID(client_guid_)) {

[asanka, 2013/08/07 16:42:36]

Skip this validation step and just DCHECK the return value from
CLSIDFromString() or UUIDFromString()? You'd still need to check if the
client_guid_ isn't empty since the base DownloadManagerDelegate implementation
passes an empty string.

[Greg Billock, 2013/08/07 17:42:56]

I wasn't sure what bad thing CLSIDFromString might do if the string is
malformatted. It looks to me like it might interpret it as some kind of
classname and consult the registry, which is not what I want.

+    HRESULT hr = CLSIDFromString(

[asanka, 2013/08/07 16:42:36]

Have you looked at using UuidFromString() instead?

The documentation for CLSIDFromString() says that it can return
REGDB_E_CLASSNOTREG (CLSID corresponding to the class string was not found in
the registry) and REGDB_E_READREGDB (the registry could not be opened for
reading). These might only apply if you pass a class string instead of a GUID
string. Still, UuidFromString() seems to have less baggage.

[Greg Billock, 2013/08/07 17:42:56]

I saw UuidFromString(), but it isn't used elsewhere in Chrome, which made me
suspicious, and I found Stuff On The Internet that CLSIDFromString is preferred.
I'm happy to change it, though -- you probably know more about it than I do. :-)

On 2013/08/07 16:42:36, asanka wrote:

[asanka, 2013/08/07 19:56:12]

I don't consider myself to be an expert on this, so I'm not going to strongly
advocate for either. But from my reading of the documentation, it seemed
UuidFromString was a more straightforward API.

CLSIDFromString is preferred over GUIDFromString since the latter appears to be
deprecated. The documentation for UuidFromString doesn't have such a disclaimer.

FWIW, I checked briefly and noticed that IE9 doesn't call
IIAttachmentExecute::SetClientGuid() (or in IE's case,
ieframe!CAttachmentServices::SetClientGuid).

[Greg Billock, 2013/08/07 23:10:31]

Tried UuidFromString. This is part of the RPC library, which we don't link. We'd
have to bring in another dependency for it.

A wrinkle is that CLSIDFromString wants braces around the string. Sigh. I'm
thinking maybe using ifdefs here is actually better. :-(

+        base::UTF8ToUTF16(client_guid_).c_str(), &guid);
+    if (FAILED(hr))
+      guid = GUID_NULL;
+  }
+  HRESULT hr = base::AVScanFile(full_path_, source_url_.spec(), guid);
 
   // 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 (!base::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