Bug 1235982 Add CFG to firefox.exe and mozglue, and a mochitest to ensure a crash draft
authorTom Ritter <tom@ritter.vg>
Thu, 25 Jan 2018 12:15:40 -0600
changeset 749535 7c0bc5a890f33f123d7383d11eb59c1f35c6becb
parent 749389 7b46ef2ae1412b15ed45e7d2367ca491344729f7
child 749536 afd8543a205758329d761c8dea5ce9f02b4c299a
push id97415
push userbmo:tom@mozilla.com
push dateWed, 31 Jan 2018 16:34:20 +0000
bugs1235982
milestone60.0a1
Bug 1235982 Add CFG to firefox.exe and mozglue, and a mochitest to ensure a crash MozReview-Commit-ID: EYKgDyGtw8m
browser/app/moz.build
mozglue/build/WindowsCFGStatus.cpp
mozglue/build/WindowsCFGStatus.h
mozglue/build/moz.build
toolkit/xre/test/browser.ini
toolkit/xre/test/browser_checkcfgstatus.js
--- a/browser/app/moz.build
+++ b/browser/app/moz.build
@@ -79,16 +79,21 @@ if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_
         'sandbox_s',
     ]
 
     DELAYLOAD_DLLS += [
         'winmm.dll',
         'user32.dll',
     ]
 
+    if CONFIG['CC_TYPE'] == "msvc":
+        CFLAGS += ['-guard:cf']
+        CXXFLAGS += ['-guard:cf']
+        LDFLAGS += ['-guard:cf']
+
 # Control the default heap size.
 # This is the heap returned by GetProcessHeap().
 # As we use the CRT heap, the default size is too large and wastes VM.
 #
 # The default heap size is 1MB on Win32.
 # The heap will grow if need be.
 #
 # Set it to 256k.  See bug 127069.
new file mode 100644
--- /dev/null
+++ b/mozglue/build/WindowsCFGStatus.cpp
@@ -0,0 +1,74 @@
+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <windows.h>
+#include <winternl.h>
+#include <io.h>
+#include <intrin.h>
+
+#include <setjmp.h>
+
+#include "WindowsCFGStatus.h"
+
+// Inspired by https://github.com/trailofbits/cfg-showcase/blob/master/cfg_icall.cpp
+
+jmp_buf env;
+
+#pragma optimize("", off )
+void not_entry_point() {
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
+
+  longjmp(env, 1);
+  return;
+}
+#pragma optimize("", on )
+
+typedef void (*fn_ptr)();
+
+/*
+ * Tests for Microsoft's Control Flow Guard compiler security feature.
+ * This function will crash if CFG is enabled, and return false if it is
+ * not enabled.
+ *
+ * CFG protects indirect function calls and ensures they call a valid entry
+ * point. We create a function pointer that calls an invalid entry point.
+ * That invalid entry point is a nop sled.
+ *
+ * Jumping into the nop sled skips the preamble that a function normally
+ * performs, so if we hit the return (ret) we would mess up the stack.
+ * To 'return' from the function safely we jump back to our original
+ * function - no preamble and no return.
+ *
+ * We use setjmp/longjmp because inline asm instructions aren't supported
+ * in x64 by MSVC.
+ */
+MFBT_API bool
+CFG_DisabledOrCrash()
+{
+  // setjmp returns 0 on the initial call and whatever value is given
+  // to longjmp (here it is 1) when it is jumped back to.
+  int val = setjmp(env);
+
+  if (val == 0) {
+    fn_ptr slide_to_the_left = (fn_ptr)((uintptr_t)(not_entry_point) + 0x20);
+
+    // If CFG is enabled, we're going to crash on the next line
+    slide_to_the_left();
+  }
+
+  return false;
+}
new file mode 100644
--- /dev/null
+++ b/mozglue/build/WindowsCFGStatus.h
@@ -0,0 +1,33 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef mozilla_windowscfgstatus_h
+#define mozilla_windowscfgstatus_h
+
+#if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
+
+#include <windows.h>
+#include "mozilla/Attributes.h"
+#include "mozilla/Types.h"
+
+extern "C" {
+
+/**
+ * Tests for Microsoft's Control Flow Guard compiler security feature.
+ * This function will crash if CFG is enabled.
+ *
+ * There is a dependency on the calling convention in
+ * toolkit/xre/test/browser_checkcfgstatus.js so be sure to update that
+ * if it changes.
+ *
+ * @returns false if CFG is not enabled. Crashes if CFG is enabled.
+ * It will never return true.
+ */
+MFBT_API bool CFG_DisabledOrCrash();
+
+}
+
+#endif // defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
+#endif // mozilla_windowscfgstatus_h
--- a/mozglue/build/moz.build
+++ b/mozglue/build/moz.build
@@ -26,44 +26,54 @@ if CONFIG['MOZ_ASAN']:
 
 if CONFIG['OS_TARGET'] == 'WINNT':
     DEFFILE = 'mozglue.def'
     # We'll break the DLL blocklist if we immediately load user32.dll
     DELAYLOAD_DLLS += [
         'user32.dll',
     ]
 
+    if CONFIG['CC_TYPE'] == "msvc":
+        CFLAGS += ['-guard:cf']
+        CXXFLAGS += ['-guard:cf']
+        LDFLAGS += ['-guard:cf']
+
 if CONFIG['MOZ_WIDGET_TOOLKIT']:
 
     if CONFIG['MOZ_MEMORY'] and FORCE_SHARED_LIB:
         pass
         # TODO: SHARED_LIBRARY_LIBS go here
     else:
         # Temporary, until bug 662814 lands
         NoVisibilityFlags()
         SOURCES += [
             'dummy.cpp',
         ]
 
     if CONFIG['OS_TARGET'] == 'WINNT':
         LOCAL_INCLUDES += [
             '/memory/build',
         ]
+
+        if CONFIG['CC_TYPE'] == "msvc":
+            SOURCES += ['WindowsCFGStatus.cpp']
         SOURCES += [
             'WindowsDllBlocklist.cpp',
         ]
+
         DisableStlWrapping()
         OS_LIBS += [
             'version',
         ]
 
     EXPORTS.mozilla += [
         'arm.h',
         'mips.h',
         'SSE.h',
+        'WindowsCFGStatus.h',
         'WindowsDllBlocklist.h',
         'WindowsDllServices.h',
     ]
 
     if CONFIG['CPU_ARCH'].startswith('x86'):
         SOURCES += [
             'SSE.cpp',
         ]
--- a/toolkit/xre/test/browser.ini
+++ b/toolkit/xre/test/browser.ini
@@ -1,4 +1,8 @@
 [DEFAULT]
 
 [browser_checkdllblockliststate.js]
 skip-if = os != "win" || (os == "win" && os_version == "10.0") # Bug 1401250
+
+[browser_checkcfgstatus.js]
+# CFG is only supported on Windows 10+, only run it there
+skip-if = os != "win" || os_version == "6.1" || os_version == "6.3"
\ No newline at end of file
new file mode 100644
--- /dev/null
+++ b/toolkit/xre/test/browser_checkcfgstatus.js
@@ -0,0 +1,79 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
+"use strict";
+
+/**
+ * Returns the id of the crash minidump.
+ *
+ * @param subject (nsISupports)
+ *        The subject passed through the ipc:content-shutdown
+ *        observer notification when a content process crash has
+ *        occurred.
+ * @returns {String} The crash dump id.
+ */
+function getCrashDumpId(subject) {
+  Assert.ok(subject instanceof Ci.nsIPropertyBag2,
+            "Subject needs to be a nsIPropertyBag2 to clean up properly");
+
+  return subject.getPropertyAsAString("dumpID");
+}
+
+// Calls a function that should crash when CFG is enabled
+add_task(async function test_cfg_enabled() {
+  // On debug builds, crashing tabs results in much thinking, which
+  // slows down the test and results in intermittent test timeouts,
+  // so we'll pump up the expected timeout for this test.
+  requestLongerTimeout(2);
+
+  if (!gMultiProcessBrowser) {
+    Assert.ok(false, "This test should only be run in multi-process mode.");
+    return;
+  }
+
+  await BrowserTestUtils.withNewTab({
+    gBrowser,
+    url: "http://example.com"
+  }, async function(browser) {
+    // First, sanity check...
+    Assert.ok(browser.isRemoteBrowser,
+              "This browser needs to be remote if this test is going to " +
+              "work properly.");
+    let contentProcessGone = TestUtils.topicObserved("ipc:content-shutdown");
+
+    ContentTask.spawn(browser, null, function() {
+      // Until 1342564 is fixed, we need to disable this call or it will cause False Positives
+      privateNoteIntentionalCrash();
+
+      ChromeUtils.import("resource://gre/modules/ctypes.jsm");
+      let mozglue = ctypes.open("mozglue.dll");
+      let CFG_DisabledOrCrash = mozglue.declare("CFG_DisabledOrCrash", ctypes.default_abi, ctypes.bool);
+      CFG_DisabledOrCrash();
+      // ^-- this line should have crashed us. If we get to the next line, no bueno
+
+      Assert.ok(false, "This test should cause a crash when CFG is enabled. If it " +
+           "does not, this false assertion will trigger. It means CFG is not enabled " +
+           "and we have lost compiler hardening features.");
+    });
+
+    // If we don't crash within 5 seconds, give up.
+    let timeout = new Promise(resolve => setTimeout(resolve, 5000, [null]));
+
+    // We crash or timeout
+    let [subject, /* , data */] = await Promise.race([timeout, contentProcessGone]);
+
+    if (!subject) {
+      // We timed out, or otherwise didn't crash properly
+      Assert.ok(false, "This test should cause a crash when CFG is enabled. We didn't " +
+        "observe a crash. This specific assertion should be redundant to a false assertion " +
+        "immediately prior to it. If it occurs alone, then something strange has occured " +
+        "and CFG status and this test should be investigated.");
+    } else {
+      // We crashed properly, clean up...
+      info("Content process is gone!");
+
+      // If we don't clean up the minidump, the harness will complain.
+      let dumpID = getCrashDumpId(subject);
+
+      Assert.ok(dumpID == "", "There should NOT be a dumpID, but we have one: " + dumpID);
+    }
+  });
+});