Bug 1733821 - [6/9] Rewrite EnsureCommandlineSafeImpl to allow option…

…al arguments r=mhowell,Gijs

Add an `optionalParams` argument to `EnsureCommandlineSafeImpl`.

For security's sake, optional arguments may not occur after the URL
parameter, and may not themselves take arguments. For convenience's
sake, they are further limited to coming between `-osint` and the
required parameter.

We do not yet recognize any optional arguments; this will change in an
upcoming commit.

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

toolkit/xre/CmdLineAndEnvUtils.h

@@ -217,66 +217,113 @@ namespace internal {
 // constexpr bool IsStringRange =
 //     std::convertible_to<std::ranges::range_value_t<T>, const char *>;
 
-template <typename CharT, typename ReqContainerT>
-// requires IsStringRange<ReqContainerT>
+template <typename CharT, typename ListT>
+// requires IsStringRange<ListT>
+static bool MatchesAnyOf(CharT const* unknown, ListT const& known) {
+  for (const char* k : known) {
+    if (strimatch(k, unknown)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+template <typename CharT, typename ReqContainerT, typename OptContainerT>
+// requires IsStringRange<ReqContainerT> && IsStringRange<OptContainerT>
 inline bool EnsureCommandlineSafeImpl(int aArgc, CharT** aArgv,
-                                      ReqContainerT const& requiredParams) {
+                                      ReqContainerT const& requiredParams,
+                                      OptContainerT const& optionalParams) {
   // We expect either no -osint, or the full commandline to be:
-  // app -osint
-  // followed by one of the arguments listed in requiredParams,
-  // 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.
+  //
+  //   app -osint [<optional-param>...] <required-param> <required-argument>
+  //
+  // Otherwise, we abort to avoid abuse of other command-line handlers from apps
+  // that do a poor job escaping links they give to the OS.
+  //
+  // Note that the above implies that optional parameters do not themselves take
+  // arguments. This is a security feature, to prevent the possible injection of
+  // additional parameters via such arguments. (See, e.g., bug 384384.)
 
   static constexpr const char* osintLit = "osint";
 
+  // If "-osint" (or the equivalent) is not present, then this is trivially
+  // satisfied.
   if (CheckArg(aArgc, aArgv, osintLit, static_cast<const CharT**>(nullptr),
-               CheckArgFlag::None) == ARG_FOUND) {
-    // There should be 4 items left (app name + -osint + (acceptable arg) +
-    // param)
-    if (aArgc != 4) {
-      return false;
-    }
-
-    // The first should be osint.
-    const auto arg1 = ReadAsOption(aArgv[1]);
-    if (!arg1) return false;
-    if (!strimatch(osintLit, arg1.value())) {
-      return false;
-    }
+               CheckArgFlag::None) != ARG_FOUND) {
+    return true;
+  }
 
-    // Now only an acceptable argument and a parameter for it should be left:
-    const auto arg2 = ReadAsOption(aArgv[2]);
-    if (!arg2) return false;
+  // There should be at least 4 items present:
+  //    <app name> -osint <required param> <arg>.
+  if (aArgc < 4) {
+    return false;
+  }
 
-    const bool haveRequiredArg = [&] {
-      for (const char* a : requiredParams) {
-        if (strimatch(a, arg2.value())) {
-          return true;
-        }
-      }
-      return false;
-    }();
-    if (!haveRequiredArg) {
-      return false;
+  // The first parameter must be osint.
+  const auto arg1 = ReadAsOption(aArgv[1]);
+  if (!arg1) return false;
+  if (!strimatch(osintLit, arg1.value())) {
+    return false;
+  }
+  // Following this is any number of optional parameters, terminated by a
+  // required parameter.
+  int pos = 2;
+  while (true) {
+    if (pos >= aArgc) return false;
+
+    auto const arg = ReadAsOption(aArgv[pos]);
+    if (!arg) return false;
+
+    if (MatchesAnyOf(arg.value(), optionalParams)) {
+      ++pos;
+      continue;
     }
 
-    // The param that is passed afterwards shouldn't be another switch:
-    if (ReadAsOption(aArgv[3])) {
-      return false;
+    if (MatchesAnyOf(arg.value(), requiredParams)) {
+      ++pos;
+      break;
     }
+
+    return false;
+  }
+
+  // There must be one argument remaining...
+  if (pos + 1 != aArgc) return false;
+  // ... which must not be another option.
+  if (ReadAsOption(aArgv[pos])) {
+    return false;
   }
 
-  // Either no osint, so nothing to do, or we ensured nothing nefarious was
-  // passed.
+  // Nothing ill-formed was passed.
   return true;
 }
-}  // namespace internal
 
+// C (and so C++) disallows empty arrays. Rather than require callers to jump
+// through hoops to specify an empty optional-argument list, allow either its
+// omission or its specification as `nullptr`, and do the hoop-jumping here.
+//
+// No such facility is provided for requiredParams, which must have at least one
+// entry.
 template <typename CharT, typename ReqContainerT>
-inline void EnsureCommandlineSafe(int aArgc, CharT** aArgv,
-                                  ReqContainerT const& requiredParams) {
-  if (!internal::EnsureCommandlineSafeImpl(aArgc, aArgv, requiredParams)) {
+inline bool EnsureCommandlineSafeImpl(int aArgc, CharT** aArgv,
+                                      ReqContainerT const& requiredParams,
+                                      std::nullptr_t _ = nullptr) {
+  struct {
+    inline const char** begin() const { return nullptr; }
+    inline const char** end() const { return nullptr; }
+  } emptyContainer;
+  return EnsureCommandlineSafeImpl(aArgc, aArgv, requiredParams,
+                                   emptyContainer);
+}
+}  // namespace internal
+
+template <typename CharT, typename ReqContainerT,
+          typename OptContainerT = std::nullptr_t>
+inline void EnsureCommandlineSafe(
+    int aArgc, CharT** aArgv, ReqContainerT const& requiredParams,
+    OptContainerT const& optionalParams = nullptr) {
+  if (!internal::EnsureCommandlineSafeImpl(aArgc, aArgv, requiredParams,
+                                           optionalParams)) {
     exit(127);
   }
 }

toolkit/xre/test/gtest/TestCmdLineAndEnvUtils.cpp

@@ -246,22 +246,57 @@ std::pair<TestCaseState, std::vector<const char*>> const kCommandLines[] = {
     {PASS, {"-aleph", "-aleph", "http://www.example.com/"}},
 };
 
+constexpr static char const* const kOptionalArgs[] = {"mozilla", "allizom"};
+
+// Additional test cases for optional parameters.
+//
+// Test cases marked PASS should pass iff the above optional parameters are
+// permitted. (Test cases marked FAIL should fail regardless, and are only
+// grouped here for convenience and semantic coherence.)
+std::pair<TestCaseState, std::vector<const char*>> kCommandLinesOpt[] = {
+    {PASS, {"-osint", "-mozilla", "-aleph", "http://www.example.com/"}},
+    {PASS, {"-osint", "-allizom", "-aleph", "http://www.example.com/"}},
+    {PASS,
+     {"-osint", "-mozilla", "-allizom", "-aleph", "http://www.example.com/"}},
+    {PASS,
+     {"-osint", "-allizom", "-mozilla", "-aleph", "http://www.example.com/"}},
+
+    {FAIL, {"-mozilla", "-osint", "-aleph", "http://www.example.com/"}},
+    {FAIL, {"-osint", "-aleph", "-mozilla", "http://www.example.com/"}},
+    {FAIL, {"-osint", "-aleph", "http://www.example.com/", "-mozilla"}},
+};
+
+enum WithOptionalState : bool {
+  NoOptionalArgs = false,
+  WithOptionalArgs = true,
+};
+
 template <typename CharT>
-bool TestCommandLineImpl(CommandLine const& cl) {
+bool TestCommandLineImpl(CommandLine const& cl,
+                         WithOptionalState withOptional) {
   int argc = cl.argc();
   // EnsureCommandlineSafe's signature isn't const-correct here for annoying
   // reasons, but it is indeed non-mutating.
   CharT** argv = const_cast<CharT**>(cl.argv<CharT>());
-  return mozilla::internal::EnsureCommandlineSafeImpl<CharT>(argc, argv,
-                                                             kRequiredArgs);
+  if (withOptional) {
+    return mozilla::internal::EnsureCommandlineSafeImpl(
+        argc, argv, kRequiredArgs, kOptionalArgs);
+  }
+  return mozilla::internal::EnsureCommandlineSafeImpl(argc, argv,
+                                                      kRequiredArgs);
 }
 
 // Test that `args` produces `expectation`. On Windows, test against both
 // wide-character and narrow-character implementations.
-void TestCommandLine(TestCaseState expectation, CommandLine const& cl) {
-  EXPECT_EQ(TestCommandLineImpl<char>(cl), expectation) << "cl is: " << cl;
+void TestCommandLine(TestCaseState expectation, CommandLine const& cl,
+                     WithOptionalState withOptional) {
+  EXPECT_EQ(TestCommandLineImpl<char>(cl, withOptional), expectation)
+      << "cl is: " << cl << std::endl
+      << "withOptional is: " << bool(withOptional);
 #ifdef XP_WIN
-  EXPECT_EQ(TestCommandLineImpl<wchar_t>(cl), expectation) << "cl is: " << cl;
+  EXPECT_EQ(TestCommandLineImpl<wchar_t>(cl, withOptional), expectation)
+      << "cl is: " << cl << std::endl
+      << "withOptional is: " << bool(withOptional);
 #endif
 }
 }  // namespace testzilla
@@ -301,6 +336,24 @@ TEST(CmdLineAndEnvUtils, ensureSafe)
   using namespace testzilla;
   for (auto const& [result, data] : kCommandLines) {
     CommandLine const cl(data);
-    TestCommandLine(result, cl);
+    TestCommandLine(result, cl, NoOptionalArgs);
+  }
+  for (auto const& [_unused, data] : kCommandLinesOpt) {
+    MOZ_UNUSED(_unused);  // silence gcc
+    CommandLine const cl(data);
+    TestCommandLine(FAIL, cl, NoOptionalArgs);
+  }
+}
+
+TEST(CmdLineAndEnvUtils, ensureSafeWithOptional)
+{
+  using namespace testzilla;
+  for (auto const& [result, data] : kCommandLines) {
+    CommandLine const cl(data);
+    TestCommandLine(result, cl, WithOptionalArgs);
+  }
+  for (auto const& [result, data] : kCommandLinesOpt) {
+    CommandLine const cl(data);
+    TestCommandLine(result, cl, WithOptionalArgs);
   }
 }