Revert "Fix the issue with 3rd-party DLL blocking and IAttachmentExecute"

This reverts commit ed0a1be1.

Reason for revert: Causes a test failure in official builds:
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win%20trunk/builds/91463

Original change's description:
> Fix the issue with 3rd-party DLL blocking and IAttachmentExecute
> 
> Because the IAttachmentExecute interface will invoke registered
> third-party IOfficeAntiVirus provider, it's possible to put Chrome
> in a state where all downloads fail if a DLL needed by one of the
> provider is blocked.
> 
> This CL introduces 2 temporary solutions that aims to separately
> address the issue for enterprise and regular users. The long-term
> solution is to move the call to the attachment services to a
> utility process where third-party DLL injection is explicitly
> allowed.
> 
> For enterprise users, simply never enable the blocking feature on
> domain-joined machines.
> 
> For regular users, add a feature (InvokeAttachmentServices) that
> allows us to no longer invoke the IAttachmentExecute interface and
> instead use the fallback code path that manually sets the Zone
> Identifier (Mark of the Web).
> 
> The fallback code was a bit outdated because Windows 10 now adds the
> HostUrl and ReferrerUrl data into the Zone Identifier. This CL adds
> a new feature that writes this data similarly to how the
> IAttachmentExecute interface does. This change is also behind a new
> feature (AugmentedZoneIdentifier).
> 
> Finally, the chrome.mediaGalleries API
> (https://developer.chrome.com/apps/mediaGalleries) was also using
> the IAttachmentExecute API. A third feature now controls whether
> the mediaGalleries component uses the same function as the download
> manager, in order to benefits from the changes described above.
> 
> Bug: 870998
> Change-Id: I50c5d804c469b2499f1e6ec6998e146baaaace7a
> Reviewed-on: https://chromium-review.googlesource.com/1188864
> Reviewed-by: Asanka Herath <asanka@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Min Qin <qinmin@chromium.org>
> Commit-Queue: Patrick Monette <pmonette@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#586940}

TBR=chrisha@chromium.org,thestig@chromium.org,qinmin@chromium.org,pmonette@chromium.org,asanka@chromium.org

Change-Id: I9dd91dd84df4d7a7eddc8f2f6e264720e26b1e3b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 870998
Reviewed-on: https://chromium-review.googlesource.com/1195436Reviewed-by: Reid Kleckner <rnk@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587119}

chrome/browser/BUILD.gn

@@ -3168,7 +3168,6 @@ jumbo_split_static_library("browser") {
       "//components/browser_watcher:stability_client",
       "//components/chrome_cleaner/public/constants",
       "//components/chrome_cleaner/public/interfaces",
-      "//components/download/quarantine",
       "//third_party/crashpad/crashpad/client:client",
       "//third_party/iaccessible2",
       "//third_party/isimpledom",

chrome/browser/chrome_browser_main_win.cc

@@ -530,8 +530,7 @@ void ChromeBrowserMainPartsWin::PostProfileInit() {
   // What truly controls if the blocking is enabled is the presence of the
   // module blacklist cache file. This means that to disable the feature, the
   // cache must be deleted and the browser relaunched.
-  if (base::win::IsEnterpriseManaged() ||
-      !ModuleDatabase::IsThirdPartyBlockingPolicyEnabled() ||
+  if (!ModuleDatabase::IsThirdPartyBlockingPolicyEnabled() ||
       !base::FeatureList::IsEnabled(features::kThirdPartyModulesBlocking))
     ThirdPartyConflictsManager::DisableThirdPartyModuleBlocking(
         base::CreateTaskRunnerWithTraits(

chrome/browser/conflicts/module_database_win.cc

@@ -15,7 +15,6 @@
 #if defined(GOOGLE_CHROME_BUILD)
 #include "base/feature_list.h"
 #include "base/task/post_task.h"
-#include "base/win/win_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/conflicts/incompatible_applications_updater_win.h"
 #include "chrome/browser/conflicts/module_load_attempt_log_listener_win.h"
@@ -343,15 +342,6 @@ void ModuleDatabase::NotifyLoadedModules(ModuleDatabaseObserver* observer) {
 
 #if defined(GOOGLE_CHROME_BUILD)
 void ModuleDatabase::MaybeInitializeThirdPartyConflictsManager() {
-  // Temporarily disable this class on domain-joined machines because enterprise
-  // clients depend on IAttachmentExecute::Save() to be invoked for downloaded
-  // files, but that API call has a known issue (https://crbug.com/870998) with
-  // third-party modules blocking.
-  // TODO(pmonette): Move IAttachmentExecute::Save() to a utility process and
-  //                 remove this.
-  if (base::win::IsEnterpriseManaged())
-    return;
-
   if (!IsThirdPartyBlockingPolicyEnabled())
     return;
 

chrome/browser/media_galleries/fileapi/av_scanning_file_validator.cc

@@ -11,11 +11,8 @@
 #include <wrl/client.h>
 #endif
 
-#include <string>
-
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/feature_list.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
@@ -25,33 +22,16 @@
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_constants.h"
-#include "components/download/quarantine/quarantine.h"
 #include "content/public/browser/browser_thread.h"
-#include "url/gurl.h"
 
 using content::BrowserThread;
 
 namespace {
 
 #if defined(OS_WIN)
-// This feature controls whether the QuarantineFile() function should be used
-// to perform the AV scan instead of directly invoking the IAttachmentExecute
-// interface.
-constexpr base::Feature kMediaGalleriesQuarantineFile{
-    "MediaGalleriesQuarantineFile", base::FEATURE_DISABLED_BY_DEFAULT};
-
 base::File::Error ScanFile(const base::FilePath& dest_platform_path) {
   base::AssertBlockingAllowed();
 
-  if (base::FeatureList::IsEnabled(kMediaGalleriesQuarantineFile)) {
-    download::QuarantineFileResult quarantine_result = download::QuarantineFile(
-        dest_platform_path, GURL(), GURL(), std::string());
-
-    return quarantine_result == download::QuarantineFileResult::OK
-               ? base::File::FILE_OK
-               : base::File::FILE_ERROR_SECURITY;
-  }
-
   Microsoft::WRL::ComPtr<IAttachmentExecute> attachment_services;
   HRESULT hr = ::CoCreateInstance(CLSID_AttachmentServices, nullptr, CLSCTX_ALL,
                                   IID_PPV_ARGS(&attachment_services));

chrome/browser/ui/webui/conflicts/conflicts_handler.cc

@@ -22,7 +22,6 @@
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(GOOGLE_CHROME_BUILD)
-#include "base/win/win_util.h"
 #include "chrome/browser/conflicts/incompatible_applications_updater_win.h"
 #include "chrome/browser/conflicts/module_blacklist_cache_updater_win.h"
 #endif
@@ -303,10 +302,7 @@ void ConflictsHandler::HandleRequestModuleList(const base::ListValue* args) {
     third_party_features_status_ = kFeatureDisabled;
   }
 
-  if (base::win::IsEnterpriseManaged())
-    third_party_features_status_ = kEnterpriseManaged;
-
-  // The above 3 cases are the only possible reasons why the manager wouldn't
+  // The above 2 cases are the only possible reasons why the manager wouldn't
   // exist.
   DCHECK(third_party_features_status_.has_value());
 #else  // defined(GOOGLE_CHROME_BUILD)
@@ -370,9 +366,6 @@ std::string ConflictsHandler::GetThirdPartyFeaturesStatusString(
     case ThirdPartyFeaturesStatus::kNonGoogleChromeBuild:
       return "The third-party features are not available in non-Google Chrome "
              "builds.";
-    case ThirdPartyFeaturesStatus::kEnterpriseManaged:
-      return "The third-party features are temporarily disabled for clients on "
-             "domain-joined machines.";
     case ThirdPartyFeaturesStatus::kPolicyDisabled:
       return "The ThirdPartyBlockingEnabled group policy is disabled.";
     case ThirdPartyFeaturesStatus::kFeatureDisabled:

chrome/browser/ui/webui/conflicts/conflicts_handler.h

@@ -33,12 +33,6 @@ class ConflictsHandler : public content::WebUIMessageHandler,
   enum ThirdPartyFeaturesStatus {
     // The third-party features are not available in non-Google Chrome builds.
     kNonGoogleChromeBuild,
-    // The third-party features are temporarily disabled on domain-joined
-    // machines because of a known issue with third-party blocking and the
-    // IAttachmentExecute::Save() API (https://crbug.com/870998).
-    // TODO(pmonette): Move IAttachmentExecute::Save() to a utility process and
-    //                 remove this.
-    kEnterpriseManaged,
     // The ThirdPartyBlockingEnabled group policy is disabled.
     kPolicyDisabled,
     // Both the IncompatibleApplicationsWarning and the

components/download/quarantine/BUILD.gn

@@ -13,8 +13,6 @@ static_library("quarantine") {
     "quarantine.cc",
     "quarantine.h",
     "quarantine_constants_linux.h",
-    "quarantine_features_win.cc",
-    "quarantine_features_win.h",
     "quarantine_linux.cc",
     "quarantine_mac.mm",
     "quarantine_win.cc",

components/download/quarantine/quarantine_features_win.cc

-// Copyright 2018 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 "components/download/quarantine/quarantine_features_win.h"
-
-namespace download {
-
-// When manually setting the Zone Identifier, this feature controls whether the
-// HostUrl and ReferrerUrl values are set.
-const base::Feature kAugmentedZoneIdentifier{"AugmentedZoneIdentifier",
-                                             base::FEATURE_DISABLED_BY_DEFAULT};
-
-// This feature controls whether the InvokeAttachmentServices function will be
-// called. Has no effect on machines that are domain-joined, where the function
-// is always called.
-const base::Feature kInvokeAttachmentServices{"InvokeAttachmentServices",
-                                              base::FEATURE_ENABLED_BY_DEFAULT};
-
-}  // namespace download

components/download/quarantine/quarantine_features_win.h

-// Copyright 2018 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.
-
-#ifndef COMPONENTS_DOWNLOAD_QUARANTINE_QUARANTINE_FEATURES_WIN_H_
-#define COMPONENTS_DOWNLOAD_QUARANTINE_QUARANTINE_FEATURES_WIN_H_
-
-#include "base/feature_list.h"
-
-namespace download {
-
-extern const base::Feature kAugmentedZoneIdentifier;
-
-extern const base::Feature kInvokeAttachmentServices;
-
-}  // namespace download
-
-#endif  // COMPONENTS_DOWNLOAD_QUARANTINE_QUARANTINE_FEATURES_WIN_H_

components/download/quarantine/quarantine_win.cc

@@ -16,7 +16,6 @@
 
 #include <vector>
 
-#include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/logging.h"
@@ -25,12 +24,9 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
-#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/win/scoped_handle.h"
-#include "base/win/win_util.h"
-#include "components/download/quarantine/quarantine_features_win.h"
 #include "url/gurl.h"
 
 namespace download {
@@ -71,26 +67,14 @@ bool ZoneIdentifierPresentForFile(const base::FilePath& path) {
          lines[1].find("ZoneId=") == 0;
 }
 
-// Returns true for a valid |url| whose length does not exceed
-// INTERNET_MAX_URL_LENGTH.
-bool IsValidUrlForAttachmentServices(const GURL& url) {
-  return url.is_valid() && url.spec().size() <= INTERNET_MAX_URL_LENGTH;
-}
-
 // Sets the Zone Identifier on the file to "Internet" (3). Returns true if the
 // function succeeds, false otherwise. A failure is expected if alternate
 // streams are not supported, like a file on a FAT32 filesystem.  This function
 // does not invoke Windows Attachment Execution Services.
 //
-// If the AugmentedZoneIdentifier feature is enabled, the ReferrerUrl and
-// HostUrl values are set according to the behavior of the IAttachmentExecute
-// interface on Windows 10.
-//
 // |full_path| is the path to the downloaded file.
 QuarantineFileResult SetInternetZoneIdentifierDirectly(
-    const base::FilePath& full_path,
-    const GURL& source_url,
-    const GURL& referrer_url) {
+    const base::FilePath& full_path) {
   const DWORD kShare = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
   std::wstring path = full_path.value() + kZoneIdentifierStreamSuffix;
   HANDLE file = CreateFile(path.c_str(), GENERIC_WRITE, kShare, nullptr,
@@ -98,31 +82,16 @@ QuarantineFileResult SetInternetZoneIdentifierDirectly(
   if (INVALID_HANDLE_VALUE == file)
     return QuarantineFileResult::ANNOTATION_FAILED;
 
-  static const char kReferrerUrlFormat[] = "ReferrerUrl=%s\r\n";
-  static const char kHostUrlFormat[] = "HostUrl=%s\r\n";
-
-  std::string identifier = "[ZoneTransfer]\r\nZoneId=3\r\n";
-  if (base::FeatureList::IsEnabled(kAugmentedZoneIdentifier)) {
-    // Match what the InvokeAttachmentServices() function will output, including
-    // the order of the values.
-    if (IsValidUrlForAttachmentServices(referrer_url)) {
-      identifier.append(
-          base::StringPrintf(kReferrerUrlFormat, referrer_url.spec().c_str()));
-    }
-    identifier.append(base::StringPrintf(
-        kHostUrlFormat, IsValidUrlForAttachmentServices(source_url)
-                            ? source_url.spec().c_str()
-                            : "about:internet"));
-  }
-
+  static const char kIdentifier[] = "[ZoneTransfer]\r\nZoneId=3\r\n";
   // Don't include trailing null in data written.
+  static const DWORD kIdentifierSize = arraysize(kIdentifier) - 1;
   DWORD written = 0;
-  BOOL write_result = WriteFile(file, identifier.c_str(), identifier.length(),
-                                &written, nullptr);
+  BOOL write_result =
+      WriteFile(file, kIdentifier, kIdentifierSize, &written, nullptr);
   BOOL flush_result = FlushFileBuffers(file);
   CloseHandle(file);
 
-  return write_result && flush_result && written == identifier.length()
+  return write_result && flush_result && written == kIdentifierSize
              ? QuarantineFileResult::OK
              : QuarantineFileResult::ANNOTATION_FAILED;
 }
@@ -158,8 +127,8 @@ QuarantineFileResult SetInternetZoneIdentifierDirectly(
 //                Used to identify the app to the system AV function.
 // |save_result|: Receives the result of invoking IAttachmentExecute::Save().
 bool InvokeAttachmentServices(const base::FilePath& full_path,
-                              const GURL& source_url,
-                              const GURL& referrer_url,
+                              const std::string& source_url,
+                              const std::string& referrer_url,
                               const GUID& client_guid,
                               HRESULT* save_result) {
   Microsoft::WRL::ComPtr<IAttachmentExecute> attachment_services;
@@ -186,16 +155,16 @@ bool InvokeAttachmentServices(const base::FilePath& full_path,
     return false;
 
   // The source URL could be empty if it was not a valid URL, or was not HTTP/S,
-  // or the download was off-the-record. If so, use "about:internet" as a
+  // or the download was off-the-record. If so, user "about:internet" as a
   // fallback URL. The latter is known to reliably map to the Internet zone.
   //
   // In addition, URLs that are longer than INTERNET_MAX_URL_LENGTH are also
   // known to cause problems for URLMon. Hence also use "about:internet" in
   // these cases. See http://crbug.com/601538.
   hr = attachment_services->SetSource(
-      IsValidUrlForAttachmentServices(source_url)
-          ? base::UTF8ToWide(source_url.spec()).c_str()
-          : L"about:internet");
+      source_url.empty() || source_url.size() > INTERNET_MAX_URL_LENGTH
+          ? L"about:internet"
+          : base::UTF8ToWide(source_url).c_str());
   if (FAILED(hr))
     return false;
 
@@ -203,9 +172,9 @@ bool InvokeAttachmentServices(const base::FilePath& full_path,
   // INTERNET_MAX_URL_LENGTH. Also, the source_url is authoritative for
   // determining the relative danger of |full_path| so we don't consider it an
   // error if we have to skip the |referrer_url|.
-  if (IsValidUrlForAttachmentServices(referrer_url)) {
+  if (!referrer_url.empty() && referrer_url.size() < INTERNET_MAX_URL_LENGTH) {
     hr = attachment_services->SetReferrer(
-        base::UTF8ToWide(referrer_url.spec()).c_str());
+        base::UTF8ToWide(referrer_url).c_str());
     if (FAILED(hr))
       return false;
   }
@@ -269,29 +238,14 @@ QuarantineFileResult QuarantineFile(const base::FilePath& file,
     // Calling InvokeAttachmentServices on an empty file can result in the file
     // being deleted.  Also an anti-virus scan doesn't make a lot of sense to
     // perform on an empty file.
-    return SetInternetZoneIdentifierDirectly(file, source_url, referrer_url);
+    return SetInternetZoneIdentifierDirectly(file);
   }
 
   HRESULT save_result = S_OK;
-
-  // Check if the attachment services should be invoked based on the experiment
-  // state. Not invoking the attachment services means that the Zone Identifier
-  // will always be set to 3 (Internet), regardless of URL zones configurations.
-  //
-  // Note: The attachment services must always be invoked on domain-joined
-  // machines.
-  // TODO(pmonette): Move the InvokeAttachmentServices() call to a utility
-  //                 process and remove the feature.
-  bool should_invoke_attachment_services =
-      base::win::IsEnterpriseManaged() ||
-      base::FeatureList::IsEnabled(kInvokeAttachmentServices);
-
-  bool attachment_services_available =
-      should_invoke_attachment_services &&
-      InvokeAttachmentServices(file, source_url, referrer_url, guid,
-                               &save_result);
+  bool attachment_services_available = InvokeAttachmentServices(
+      file, source_url.spec(), referrer_url.spec(), guid, &save_result);
   if (!attachment_services_available)
-    return SetInternetZoneIdentifierDirectly(file, source_url, referrer_url);
+    return SetInternetZoneIdentifierDirectly(file);
 
   // If the download file is missing after the call, then treat this as an
   // interrupted download.