Bug 1256992 Part 2: Move SandboxBroker Initialization earlier and add telemetry and extra null checks. r=aklotz
MozReview-Commit-ID: Fu05wLn27UG
--- a/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
+++ b/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ -429,46 +429,62 @@ SandboxBroker::SetSecurityLevelForGMPlug
ret = ret && (sandbox::SBOX_ALL_OK == result);
return ret;
}
bool
SandboxBroker::AllowReadFile(wchar_t const *file)
{
+ if (!mPolicy) {
+ return false;
+ }
+
auto result =
mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
sandbox::TargetPolicy::FILES_ALLOW_READONLY,
file);
return (sandbox::SBOX_ALL_OK == result);
}
bool
SandboxBroker::AllowReadWriteFile(wchar_t const *file)
{
+ if (!mPolicy) {
+ return false;
+ }
+
auto result =
mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
sandbox::TargetPolicy::FILES_ALLOW_ANY,
file);
return (sandbox::SBOX_ALL_OK == result);
}
bool
SandboxBroker::AllowDirectory(wchar_t const *dir)
{
+ if (!mPolicy) {
+ return false;
+ }
+
auto result =
mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
sandbox::TargetPolicy::FILES_ALLOW_DIR_ANY,
dir);
return (sandbox::SBOX_ALL_OK == result);
}
bool
SandboxBroker::AddTargetPeer(HANDLE aPeerProcess)
{
+ if (!sBrokerService) {
+ return false;
+ }
+
sandbox::ResultCode result = sBrokerService->AddTargetPeer(aPeerProcess);
return (sandbox::SBOX_ALL_OK == result);
}
SandboxBroker::~SandboxBroker()
{
if (mPolicy) {
mPolicy->Release();
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -10460,10 +10460,17 @@
"alert_emails": ["wmccloskey@mozilla.com"],
"bug_numbers": [1260908],
"expires_in_version": "55",
"kind": "exponential",
"high": 8000000,
"n_buckets": 50,
"keyed": true,
"description": "Measures the size of message manager messages by message name"
+ },
+ "SANDBOX_BROKER_INITIALIZED": {
+ "alert_emails": ["bowen@mozilla.com"],
+ "bug_numbers": [1256992],
+ "expires_in_version": "55",
+ "kind": "boolean",
+ "description": "Result of call to SandboxBroker::Initialize"
}
}
--- a/toolkit/xre/nsAppRunner.cpp
+++ b/toolkit/xre/nsAppRunner.cpp
@@ -3361,16 +3361,34 @@ XREMain::XRE_mainInit(bool* aExitFlag)
#endif
SaveWordToEnv("MOZ_CRASHREPORTER_STRINGS_OVERRIDE", overridePath);
}
}
}
#endif
+#if defined(MOZ_SANDBOX) && defined(XP_WIN)
+ bool brokerInitialized = SandboxBroker::Initialize();
+ Telemetry::Accumulate(Telemetry::SANDBOX_BROKER_INITIALIZED,
+ brokerInitialized);
+ if (!brokerInitialized) {
+#if defined(MOZ_CONTENT_SANDBOX)
+ // If we're sandboxing content and we fail to initialize, then crashing here
+ // seems like the sensible option.
+ if (BrowserTabsRemoteAutostart()) {
+ MOZ_CRASH("Failed to initialize broker services, can't continue.");
+ }
+#endif
+ // Otherwise just warn for the moment, as most things will work.
+ NS_WARNING("Failed to initialize broker services, sandboxed processes will "
+ "fail to start.");
+ }
+#endif
+
#ifdef XP_MACOSX
if (EnvHasValue("MOZ_LAUNCHED_CHILD")) {
// This is needed, on relaunch, to force the OS to use the "Cocoa Dock
// API". Otherwise the call to ReceiveNextEvent() below will make it
// use the "Carbon Dock API". For more info see bmo bug 377166.
EnsureUseCocoaDockAPI();
// When the app relaunches, the original process exits. This causes
@@ -3724,22 +3742,16 @@ XREMain::XRE_mainStartup(bool* aExitFlag
if (!gtk_parse_args(&gArgc, &gArgv))
return 1;
#endif /* MOZ_WIDGET_GTK */
if (PR_GetEnv("MOZ_RUN_GTEST")) {
int result;
#ifdef XP_WIN
UseParentConsole();
-#if defined(MOZ_SANDBOX)
- if (!SandboxBroker::Initialize()) {
- NS_WARNING("Failed to initialize broker services, sandboxed processes "
- "will fail to start.");
- }
-#endif
#endif
// RunGTest will only be set if we're in xul-unit
if (mozilla::RunGTest) {
gIsGtest = true;
result = mozilla::RunGTest();
gIsGtest = false;
} else {
result = 1;
@@ -4315,30 +4327,16 @@ XREMain::XRE_mainRun()
#ifdef MOZ_INSTRUMENT_EVENT_LOOP
if (PR_GetEnv("MOZ_INSTRUMENT_EVENT_LOOP")) {
bool logToConsole = true;
mozilla::InitEventTracing(logToConsole);
}
#endif /* MOZ_INSTRUMENT_EVENT_LOOP */
-#if defined(MOZ_SANDBOX) && defined(XP_WIN)
- if (!SandboxBroker::Initialize()) {
-#if defined(MOZ_CONTENT_SANDBOX)
- // If we're sandboxing content and we fail to initialize, then crashing here
- // seems like the sensible option.
- if (BrowserTabsRemoteAutostart()) {
- MOZ_CRASH("Failed to initialize broker services, can't continue.");
- }
-#endif
- // Otherwise just warn for the moment, as most things will work.
- NS_WARNING("Failed to initialize broker services, sandboxed processes will "
- "fail to start.");
- }
-#endif
#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
SetUpSandboxEnvironment();
#endif
{
rv = appStartup->Run();
if (NS_FAILED(rv)) {
NS_ERROR("failed to run appstartup");