[Windows CFG Test] Added unittest for CFG enabling on process.

sandbox/win/src/cfi_unittest.cc

Index: sandbox/win/src/cfi_unittest.cc
diff --git a/sandbox/win/src/cfi_unittest.cc b/sandbox/win/src/cfi_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5f54fa754612d64d9d423c5083984ec564d75aef
--- /dev/null
+++ b/sandbox/win/src/cfi_unittest.cc
@@ -0,0 +1,51 @@
+// Copyright 2017 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 <windows.h>
+#include <versionhelpers.h>
+
+#include "base/command_line.h"
+#include "base/process/launch.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const wchar_t* kExeFilename = L"cfi_unittest_exe.exe";
+const wchar_t* kSysDllTest = L"1";
+
+// CFG security check failure (STATUS_STACK_BUFFER_OVERRUN).
+const DWORD kCfgExceptionId = 0xc0000409;

[Will Harris, 2017/02/06 22:33:36]

This appears to be declared (in winnt.h) can it be included somehow from SDK
instead of hard coded?

[penny, 2017/02/07 23:12:56]

Done.

+}  // namespace
+
+namespace sandbox {
+
+// Make sure Microsoft binaries, that are compiled with CFG enabled, catch
+// a hook and throw an exception.
+// - If this test fails, the expected CFG exception did NOT happen.  This
+//   indicates a build system change that has disabled Chrome process-wide CFG.
+TEST(CFGSupportTests, MsIndirectFailure) {
+  // CFG is only supported on >= Win8.1 Update 3.
+  // Not checking for update, since test infra is updated and it would add
+  // a lot of complexity.
+  if (::IsWindows8Point1OrGreater()) {

[Will Harris, 2017/02/06 22:33:36]

we would normally use base::win::GetVersion and VERSION_WIN8_1 and not this
function, is there any reason not to here?

[penny, 2017/02/07 23:12:56]

Yes.  If one only has to check version, and nothing more intricate, it's more
efficient to use these APIs (macros).  No need to include all of
base/win/windows_version.h and set up the singleton.  No need to call down to
GetVersion/NtQuerySystemInformation for this one unit test.

Microsoft have been recommending using these APIs for some time now. 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).aspx 
These APIs also take into account manifesting/compatibility mode.

However, you're right that our chrome code currently uses the base version
singleton, so I totally agree that we should be consistent in that regard.  If
this code was in a standalone module with no base, I'd argue these APIs should
be used.  But sbox_integration_tests definitely already uses the base singleton.


So for now, I've changed this to use base... and I'll revisit how
base/win/windows_version is implemented in the future.

DONE

+    base::CommandLine cmd_line = base::CommandLine::FromString(kExeFilename);
+    cmd_line.AppendArgNative(kSysDllTest);
+
+    base::Process proc =
+        base::LaunchProcess(cmd_line, base::LaunchOptionsForTest());
+    ASSERT_TRUE(proc.IsValid());
+
+    int exit_code = 0;
+    if (!proc.WaitForExitWithTimeout(base::TimeDelta::FromSeconds(5),

[Will Harris, 2017/02/06 22:33:36]

consider using TestTimeouts::tiny_timeout() or action_timeout()

[penny, 2017/02/07 23:12:56]

Done.  action_timeout().

I wasn't 100% certain this would always be long enough.  I'm waiting on process
creation, then library load and copy-on-write system dll, etc... but I'm using
this timeout in process_mitigations_test.cc, so it should be fine!  Thanks.

+                                     &exit_code)) {
+      // Timeout while waiting.  Try to cleanup.
+      proc.Terminate(1, false);
+      ADD_FAILURE();
+      return;
+    }
+
+    // CFG exception.
+    ASSERT_EQ(kCfgExceptionId, static_cast<DWORD>(exit_code));
+  }
+}
+
+}  // namespace sandbox