Bug 1577706 - move checks for -url from toolkit into browser code, and make osint sanitizer app-agnostic, r=mossop

Differential Revision: https://phabricator.services.mozilla.com/D44395

--HG--
extra : moz-landing-system : lando

browser/app/nsBrowserApp.cpp

@@ -4,6 +4,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsXULAppAPI.h"
+#include "mozilla/CmdLineAndEnvUtils.h"
 #include "mozilla/XREAppData.h"
 #include "application.ini.h"
 #include "mozilla/Bootstrap.h"
@@ -210,6 +211,10 @@ static int do_main(int argc, char* argv[], char* envp[]) {
     gBootstrap->XRE_LibFuzzerSetDriver(fuzzer::FuzzerDriver);
 #endif
 
+  // Note: keep in sync with LauncherProcessWin.
+  const char* acceptableParams[] = {"url", nullptr};
+  EnsureCommandlineSafe(argc, argv, acceptableParams);
+
   return gBootstrap->XRE_main(argc, argv, config);
 }
 

browser/app/winlauncher/LauncherProcessWin.cpp

@@ -213,7 +213,9 @@ namespace mozilla {
 
 Maybe<int> LauncherMain(int& argc, wchar_t* argv[],
                         const StaticXREAppData& aAppData) {
-  EnsureCommandlineSafe(argc, argv);
+  // Note: keep in sync with nsBrowserApp.
+  const wchar_t* acceptableParams[] = {L"url", nullptr};
+  EnsureCommandlineSafe(argc, argv, acceptableParams);
 
   SetLauncherErrorAppData(aAppData);
 

toolkit/xre/CmdLineAndEnvUtils.h

@@ -88,7 +88,7 @@ inline bool strimatch(const wchar_t* lowerstr, const wchar_t* mixedstr) {
   return internal::strimatch(&towlower, lowerstr, mixedstr);
 }
 
-enum class FlagLiteral { osint, safemode, url };
+enum class FlagLiteral { osint, safemode };
 
 template <typename CharT, FlagLiteral Literal>
 inline const CharT* GetLiteral();
@@ -106,7 +106,6 @@ inline const CharT* GetLiteral();
 
 DECLARE_FLAG_LITERAL(osint, "osint")
 DECLARE_FLAG_LITERAL(safemode, "safe-mode")
-DECLARE_FLAG_LITERAL(url, "url")
 
 enum class CheckArgFlag : uint32_t {
   None = 0,
@@ -193,9 +192,12 @@ inline ArgResult CheckArg(int& aArgc, CharT** aArgv, const CharT* aArg,
 }
 
 template <typename CharT>
-inline void EnsureCommandlineSafe(int& aArgc, CharT** aArgv) {
+inline void EnsureCommandlineSafe(int& aArgc, CharT** aArgv,
+                                  const CharT** aAcceptableArgs) {
   // We expect either no -osint, or the full commandline to be:
-  // app -osint -url foo
+  // app -osint
+  // followed by one of the arguments listed in aAcceptableArgs,
+  // followed by one parameter for that arg.
   // If this varies, we abort to avoid abuse of other commandline handlers
   // from apps that do a poor job escaping links they give to the OS.
 
@@ -203,7 +205,8 @@ inline void EnsureCommandlineSafe(int& aArgc, CharT** aArgv) {
 
   if (CheckArg(aArgc, aArgv, osintLit, static_cast<const CharT**>(nullptr),
                CheckArgFlag::None) == ARG_FOUND) {
-    // There should be 4 items left (app name + -osint + -url + param)
+    // There should be 4 items left (app name + -osint + (acceptable arg) +
+    // param)
     if (aArgc != 4) {
       exit(127);
     }
@@ -227,7 +230,7 @@ inline void EnsureCommandlineSafe(int& aArgc, CharT** aArgv) {
     // Strip it:
     RemoveArg(aArgc, aArgv + 1);
 
-    // Now only the -url bit should be left:
+    // Now only an acceptable argument and a parameter for it should be left:
     arg = aArgv[1];
     if (*arg != '-'
 #ifdef XP_WIN
@@ -240,10 +243,19 @@ inline void EnsureCommandlineSafe(int& aArgc, CharT** aArgv) {
     if (*arg == '-') {
       ++arg;
     }
-    if (!strimatch(GetLiteral<CharT, FlagLiteral::url>(), arg)) {
+    bool haveAcceptableArg = false;
+    const CharT** acceptableArg = aAcceptableArgs;
+    while (*acceptableArg) {
+      if (strimatch(*acceptableArg, arg)) {
+        haveAcceptableArg = true;
+        break;
+      }
+      acceptableArg++;
+    }
+    if (!haveAcceptableArg) {
       exit(127);
     }
-    // The param after url shouldn't be another switch:
+    // The param that is passed afterwards shouldn't be another switch:
     arg = aArgv[2];
     if (*arg == '-'
 #ifdef XP_WIN
@@ -432,9 +444,13 @@ inline bool SetArgv0ToFullBinaryPath(wchar_t* aArgv[]) {
 
 #endif  // defined(XP_WIN)
 
+// SaveToEnv and EnvHasValue are only available on Windows or when
+// MOZILLA_INTERNAL_API is defined
+#if defined(MOZILLA_INTERNAL_API) || defined(XP_WIN)
+
 // Save literal putenv string to environment variable.
 MOZ_NEVER_INLINE inline void SaveToEnv(const char* aEnvString) {
-#if defined(MOZILLA_INTERNAL_API)
+#  if defined(MOZILLA_INTERNAL_API)
   char* expr = strdup(aEnvString);
   if (expr) {
     PR_SetEnv(expr);
@@ -442,29 +458,27 @@ MOZ_NEVER_INLINE inline void SaveToEnv(const char* aEnvString) {
 
   // We intentionally leak |expr| here since it is required by PR_SetEnv.
   MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT(expr);
-#elif defined(XP_WIN)
+#  elif defined(XP_WIN)
   // This is the same as the NSPR implementation
   // (Note that we don't need to do a strdup for this case; the CRT makes a
   // copy)
   _putenv(aEnvString);
-#else
-#  error "Not implemented for this configuration"
-#endif
+#  endif
 }
 
 inline bool EnvHasValue(const char* aVarName) {
-#if defined(MOZILLA_INTERNAL_API)
+#  if defined(MOZILLA_INTERNAL_API)
   const char* val = PR_GetEnv(aVarName);
   return val && *val;
-#elif defined(XP_WIN)
+#  elif defined(XP_WIN)
   // This is the same as the NSPR implementation
   const char* val = getenv(aVarName);
   return val && *val;
-#else
-#  error "Not implemented for this configuration"
-#endif
+#  endif
 }
 
+#endif  // end windows/internal_api-only definitions
+
 #ifndef NS_NO_XPCOM
 already_AddRefed<nsIFile> GetFileFromEnv(const char* name);
 #endif

toolkit/xre/nsAppRunner.cpp

@@ -4599,9 +4599,6 @@ int XREMain::XRE_main(int argc, char* argv[], const BootstrapConfig& aConfig) {
   gArgc = argc;
   gArgv = argv;
 
-  EnsureCommandlineSafe(gArgc, gArgv);
-  // DO NOT TOUCH THE COMMANDLINE ARGS BEFORE THIS!
-
   ScopedLogging log;
 
   mozilla::LogModule::Init(gArgc, gArgv);