Add AntiVirus information to the system profile.

chrome/browser/metrics/antivirus_metrics_provider_win.cc

+// Copyright 2016 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 "chrome/browser/metrics/antivirus_metrics_provider_win.h"
+
+#include <iwscapi.h>
+#include <stddef.h>
+#include <windows.h>
+#include <wscapi.h>
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/feature_list.h"
+#include "base/file_version_info_win.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/metrics/field_trial.h"
+#include "base/path_service.h"
+#include "base/strings/string_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/task_runner_util.h"
+#include "base/threading/thread_restrictions.h"
+#include "base/version.h"
+#include "base/win/scoped_bstr.h"
+#include "base/win/scoped_com_initializer.h"
+#include "base/win/scoped_comptr.h"
+#include "base/win/windows_version.h"
+#include "chrome/common/channel_info.h"
+#include "components/metrics/proto/system_profile.pb.h"
+#include "components/variations/metrics_util.h"
+#include "components/version_info/version_info.h"
+
+namespace {
+
+const base::Feature kReportFullAvProductDetails {
+    AntiVirusMetricsProvider::kReportFullAvProductDetailsName,
+    base::FEATURE_DISABLED_BY_DEFAULT
+};
+
+bool ShouldReportFullNames() {
+  // The expectation is that this will be disabled for the majority of users,
+  // but this allows a small group to be enabled on other channels if there are
+  // a large percentage of hashes collected on these channels that are not
+  // resolved to names previously collected on Canary channel.
+  bool enabled = base::FeatureList::IsEnabled(kReportFullAvProductDetails);
+
+  if (chrome::GetChannel() == version_info::Channel::CANARY)
+    return true;
+
+  return enabled;
+}
+
+// Helper function for expanding all environment variables in |path|.
+std::wstring ExpandEnvironmentVariables(const std::wstring& path) {
+  static const DWORD kMaxBuffer = 32 * 1024;  // Max according to MSDN.
+  std::wstring path_expanded;
+  DWORD path_len = MAX_PATH;
+  do {
+    DWORD result = ExpandEnvironmentStrings(
+        path.c_str(), base::WriteInto(&path_expanded, path_len), path_len);
+    if (!result) {
+      // Failed to expand variables. Return the original string.
+      DPLOG(ERROR) << path;
+      break;
+    }
+    if (result <= path_len)
+      return path_expanded.substr(0, result - 1);
+    path_len = result;
+  } while (path_len < kMaxBuffer);
+
+  return path;
+}
+
+}  // namespace
+
+AntiVirusMetricsProvider::AntiVirusMetricsProvider(
+    scoped_refptr<base::SequencedTaskRunner> file_thread)
+    : file_thread_(file_thread), weak_ptr_factory_(this) {}
+
+AntiVirusMetricsProvider::~AntiVirusMetricsProvider() {}
+
+void AntiVirusMetricsProvider::ProvideSystemProfileMetrics(
+    metrics::SystemProfileProto* system_profile_proto) {
+  for (const auto& av_product : av_products_) {
+    auto product = system_profile_proto->add_antivirus_product();
+
+    std::string product_name = base::SysWideToUTF8(av_product.product_name);
+
+    if (ShouldReportFullNames())
+      product->set_product_name(product_name);
+    product->set_product_name_hash(metrics::HashName(product_name));
+
+    if (!av_product.product_version.empty()) {
+      std::string product_version =
+          base::SysWideToUTF8(av_product.product_version);
+      if (ShouldReportFullNames())
+        product->set_product_version(product_version);
+      product->set_product_version_hash(metrics::HashName(product_version));
+    }
+
+    product->set_product_state(av_product.product_state);
+  }
+}
+
+void AntiVirusMetricsProvider::GetAntiVirusMetrics(
+    const base::Closure& done_callback) {
+  base::PostTaskAndReplyWithResult(
+      file_thread_.get(), FROM_HERE,
+      base::Bind(&AntiVirusMetricsProvider::GetAntiVirusProductsOnFileThread),
+      base::Bind(&AntiVirusMetricsProvider::GotAntiVirusProducts,
+                 weak_ptr_factory_.GetWeakPtr(), done_callback));
+}
+
+// static
+AntiVirusMetricsProvider::AvProductList
+AntiVirusMetricsProvider::GetAntiVirusProductsOnFileThread() {
+  AvProductList av_products;
+
+  FillAntiVirusProducts(&av_products);

[Alexei Svitkine (slow), 2016/06/02 21:46:57]

Do you want to report that AV queries failed somehow?

For example, how do you differentiate "no AVs" vs. "error querying AVs"? Both
seem like they would appear as an empty AV list with your current impl.

[Will Harris, 2016/06/02 23:57:17]

added a histogram to report failures.

+
+  return av_products;
+}
+
+void AntiVirusMetricsProvider::GotAntiVirusProducts(
+    const base::Closure& done_callback,
+    const AvProductList& av_products) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  av_products_ = av_products;
+  done_callback.Run();
+}
+
+// static
+bool AntiVirusMetricsProvider::FillAntiVirusProducts(AvProductList* products) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  base::win::ScopedCOMInitializer com_initializer;
+
+  if (!com_initializer.succeeded())
+    return false;
+
+  base::win::ScopedComPtr<IWSCProductList> product_list;
+  HRESULT result =
+      CoCreateInstance(__uuidof(WSCProductList), NULL, CLSCTX_INPROC_SERVER,
+                       __uuidof(IWSCProductList), product_list.ReceiveVoid());
+  if (FAILED(result))
+    return false;
+
+  result = product_list->Initialize(WSC_SECURITY_PROVIDER_ANTIVIRUS);
+  if (FAILED(result))
+    return false;
+
+  LONG product_count;
+  result = product_list->get_Count(&product_count);
+  if (FAILED(result))
+    return false;
+
+  for (LONG i = 0; i < product_count; i++) {
+    IWscProduct* product = nullptr;
+    result = product_list->get_Item(i, &product);
+    if (FAILED(result))
+      return false;
+
+    static_assert(metrics::SystemProfileProto::AntiVirusState::
+                          SystemProfileProto_AntiVirusState_STATE_ON ==
+                      static_cast<metrics::SystemProfileProto::AntiVirusState>(
+                          WSC_SECURITY_PRODUCT_STATE_ON),
+                  "proto and API values must be the same");
+    static_assert(metrics::SystemProfileProto::AntiVirusState::
+                          SystemProfileProto_AntiVirusState_STATE_OFF ==
+                      static_cast<metrics::SystemProfileProto::AntiVirusState>(
+                          WSC_SECURITY_PRODUCT_STATE_OFF),
+                  "proto and API values must be the same");
+    static_assert(metrics::SystemProfileProto::AntiVirusState::
+                          SystemProfileProto_AntiVirusState_STATE_SNOOZED ==
+                      static_cast<metrics::SystemProfileProto::AntiVirusState>(
+                          WSC_SECURITY_PRODUCT_STATE_SNOOZED),
+                  "proto and API values must be the same");
+    static_assert(metrics::SystemProfileProto::AntiVirusState::
+                          SystemProfileProto_AntiVirusState_STATE_EXPIRED ==
+                      static_cast<metrics::SystemProfileProto::AntiVirusState>(
+                          WSC_SECURITY_PRODUCT_STATE_EXPIRED),
+                  "proto and API values must be the same");
+
+    AvProduct av_product;
+    WSC_SECURITY_PRODUCT_STATE product_state;
+    result = product->get_ProductState(&product_state);
+    if (FAILED(result))

[Alexei Svitkine (slow), 2016/06/02 21:46:57]

Should we skip individual entries where we have a failure as opposed to
returning false?

Actually, with your current impl it seems you just end early and whatever was
earlier would still be added. Doesn't seem optimal.

[Will Harris, 2016/06/02 23:57:17]

I'm making this fail, and if we get lots of errors in the new histogram I can
re-evaluate.

These functions really shouldn't be failing at all.

+      return false;
+
+    av_product.product_state =
+        static_cast<metrics::SystemProfileProto::AntiVirusState>(product_state);
+
+    if (av_product.product_state <
+            metrics::SystemProfileProto::AntiVirusState_MIN ||
+        av_product.product_state >
+            metrics::SystemProfileProto::AntiVirusState_MAX) {

[Alexei Svitkine (slow), 2016/06/02 21:46:57]

Set to OTHER here instead?

[Will Harris, 2016/06/02 23:57:17]

should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function should
never return anything other than the documented set of values.

[Alexei Svitkine (slow), 2016/06/03 15:48:33]

I still don't buy this - but maybe I'm missing something - so let me walk
through my thoughts.

Here is the scenario:

Microsoft releases Windows 10.5 (pardon my ignorance of what the versioning
process of new Windows is). They expand the enum to provide 3 more values.

The existing enums still have the same values. So the static asserts above are
still OK. (And in fact those asserts only compare against the header files of
the SDK we're compiling against - e.g. if we're still compiling against win10
sdk then the static asserts won't even catch anything).

Anyway, so we now have 3 new enum values. The API can start returning these. It
won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. Then,
it will hit this condition and so the question is how to handle it. (And I think
using a special OTHER value is a reasonable way.)

Now, maybe I'm wrong. For example, some APIs let you pass in the version of the
API you're compiled against when using it, and then the library has special
logic to only return you values you expect. Is that the case here? (If so, I
missed it.)

[Will Harris, 2016/06/03 16:01:21]

the latest code uses metrics::SystemProfileProto_AntiVirusState_IsValid to
check, which would trip if new enums were added.

[Alexei Svitkine (slow), 2016/06/03 16:14:15]

I agree it's cleaner to use that to check, however I think the concern from my
example still applies.

[Will Harris, 2016/06/03 16:28:56]

Either way achieves the same end goal. with the OTHER member in the enum, if MS
changes the API and adds new enums, then we get a load of OTHER, with the way it
is is now, we stop getting (potentially invalid, since the API has changed, what
else has changed?) data and instead get a lot of RESULT_PRODUCT_STATE_INVALID.

Either way, we will need to investigate what has changed, and probably fix it.

I prefer the RESULT_PRODUCT_STATE_INVALID way, and you prefer the OTHER way. It
seems I don't feel as strongly about it as you are, so I'm happy to change it to
the other way - although I don't agree, but you're the owner, so :) - but I
don't think the problems you are outlining in your comment are something to be
concerned about.

[Alexei Svitkine (slow), 2016/06/03 16:57:03]

OK. It does mean that we'll want to have this histogram forever and to monitor
it, but I guess that's fine-ish.

+      return false;
+    }
+
+    base::win::ScopedBstr product_name;
+    result = product->get_ProductName(product_name.Receive());
+    if (FAILED(result))
+      return false;
+    av_product.product_name = std::wstring(product_name, product_name.Length());
+    product_name.Release();
+
+    base::win::ScopedBstr remediation_path;
+    result = product->get_RemediationPath(remediation_path.Receive());
+    if (FAILED(result))
+      return false;
+    std::wstring path_str(remediation_path, remediation_path.Length());
+    remediation_path.Release();
+
+    base::FilePath full_path(ExpandEnvironmentVariables(path_str));
+
+#if !defined(_WIN64)
+    if (!base::PathExists(full_path)) {
+      // On 32-bit builds, path might contain C:\Program Files (x86) instead of
+      // C:\Program Files.
+      base::ReplaceFirstSubstringAfterOffset(&path_str, 0, L"%ProgramFiles%",
+                                             L"%ProgramW6432%");
+      full_path = base::FilePath(ExpandEnvironmentVariables(path_str));
+    }
+#endif  // !defined(_WIN64)
+    std::unique_ptr<FileVersionInfo> version_info(
+        FileVersionInfo::CreateFileVersionInfo(full_path));
+
+    if (version_info.get()) {
+      FileVersionInfoWin* version_info_win =
+          static_cast<FileVersionInfoWin*>(version_info.get());
+      av_product.product_version = version_info_win->product_version();

[Alexei Svitkine (slow), 2016/06/02 21:46:57]

Can you make GetProductionVersion() a helper function to make the body of this
loop easier to follow?

[Will Harris, 2016/06/02 23:57:17]

Done.

+    } else {
+      av_product.product_version = std::wstring();
+    }
+
+    products->push_back(av_product);
+  }
+
+  return true;
+}