Skip GetCommandLineString DCHECK in installer
This change allows the installer to bypass a DCHECK in CommandLine.
The DCHECK enforces that no arguments are parsed by
QuoteForCommandLineToArgvW with syntax "%*", e.g., %1, %2, etc. These
argument placeholders are called "insert sequences" and are used by
the Windows shell.
The DCHECK is intended to prevent new code from generating a command
line for the Windows shell by parsing a string like "chrome.exe %1"
instead of using the purpose-built GetCommandLineStringForShell().
However, setup.exe does generate a command line from string for itself
that contains placeholders, and has no need for the special syntax
created by GetCommandLineStringForShell(). Its use case is known to be
safe because the insert sequence %1 is substituted by Google Update in
a safe way. In order to support this call site, this change adds
GetCommandLineStringWithUnsafeInsertSequences(), which skips the
DCHECK, and invokes it in the setup code in question.
Bug: 1181955
Change-Id: I4b7e64a7a83b887b32c57207ce4ac3850d40f874
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2727878
Reviewed-by: Greg Thompson <[email protected]>
Reviewed-by: danakj <[email protected]>
Commit-Queue: Jesse McKenna <[email protected]>
Cr-Commit-Position: refs/heads/master@{#861452}
diff --git a/base/command_line.cc b/base/command_line.cc
index ab51501..1dd6f98 100644
--- a/base/command_line.cc
+++ b/base/command_line.cc
@@ -104,12 +104,13 @@
#if defined(OS_WIN)
// Quote a string as necessary for CommandLineToArgvW compatibility *on
// Windows*.
-std::wstring QuoteForCommandLineToArgvW(const std::wstring& arg) {
+std::wstring QuoteForCommandLineToArgvW(const std::wstring& arg,
+ bool allow_unsafe_insert_sequences) {
// Ensure that GetCommandLineString isn't used to generate command-line
- // strings for the Windows shell by checking for Windows placeholders like
+ // strings for the Windows shell by checking for Windows insert sequences like
// "%1". GetCommandLineStringForShell should be used instead to get a string
// with the correct placeholder format for the shell.
- DCHECK(arg.size() != 2 || arg[0] != L'%');
+ DCHECK(arg.size() != 2 || arg[0] != L'%' || allow_unsafe_insert_sequences);
// We follow the quoting rules of CommandLineToArgvW.
// http://msdn.microsoft.com/en-us/library/17w5ykft.aspx
@@ -533,10 +534,42 @@
}
}
+CommandLine::StringType CommandLine::GetArgumentsStringInternal(
+ bool allow_unsafe_insert_sequences) const {
+ StringType params;
+ // Append switches and arguments.
+ bool parse_switches = true;
+ for (size_t i = 1; i < argv_.size(); ++i) {
+ StringType arg = argv_[i];
+ StringType switch_string;
+ StringType switch_value;
+ parse_switches &= arg != kSwitchTerminator;
+ if (i > 1)
+ params.append(FILE_PATH_LITERAL(" "));
+ if (parse_switches && IsSwitch(arg, &switch_string, &switch_value)) {
+ params.append(switch_string);
+ if (!switch_value.empty()) {
+#if defined(OS_WIN)
+ switch_value = QuoteForCommandLineToArgvW(
+ switch_value, allow_unsafe_insert_sequences);
+#endif
+ params.append(kSwitchValueSeparator + switch_value);
+ }
+ } else {
+#if defined(OS_WIN)
+ arg = QuoteForCommandLineToArgvW(arg, allow_unsafe_insert_sequences);
+#endif
+ params.append(arg);
+ }
+ }
+ return params;
+}
+
CommandLine::StringType CommandLine::GetCommandLineString() const {
StringType string(argv_[0]);
#if defined(OS_WIN)
- string = QuoteForCommandLineToArgvW(string);
+ string = QuoteForCommandLineToArgvW(string,
+ /*allow_unsafe_insert_sequences=*/false);
#endif
StringType params(GetArgumentsString());
if (!params.empty()) {
@@ -561,35 +594,24 @@
StringType(kSwitchPrefixes[0]) + kSingleArgument +
FILE_PATH_LITERAL(" %1");
}
+
+CommandLine::StringType
+CommandLine::GetCommandLineStringWithUnsafeInsertSequences() const {
+ StringType string(argv_[0]);
+ string = QuoteForCommandLineToArgvW(string,
+ /*allow_unsafe_insert_sequences=*/true);
+ StringType params(
+ GetArgumentsStringInternal(/*allow_unsafe_insert_sequences=*/true));
+ if (!params.empty()) {
+ string.append(FILE_PATH_LITERAL(" "));
+ string.append(params);
+ }
+ return string;
+}
#endif // defined(OS_WIN)
CommandLine::StringType CommandLine::GetArgumentsString() const {
- StringType params;
- // Append switches and arguments.
- bool parse_switches = true;
- for (size_t i = 1; i < argv_.size(); ++i) {
- StringType arg = argv_[i];
- StringType switch_string;
- StringType switch_value;
- parse_switches &= arg != kSwitchTerminator;
- if (i > 1)
- params.append(FILE_PATH_LITERAL(" "));
- if (parse_switches && IsSwitch(arg, &switch_string, &switch_value)) {
- params.append(switch_string);
- if (!switch_value.empty()) {
-#if defined(OS_WIN)
- switch_value = QuoteForCommandLineToArgvW(switch_value);
-#endif
- params.append(kSwitchValueSeparator + switch_value);
- }
- } else {
-#if defined(OS_WIN)
- arg = QuoteForCommandLineToArgvW(arg);
-#endif
- params.append(arg);
- }
- }
- return params;
+ return GetArgumentsStringInternal(/*allow_unsafe_insert_sequences=*/false);
}
#if defined(OS_WIN)
diff --git a/base/command_line.h b/base/command_line.h
index 2267baa..2c5a5a3 100644
--- a/base/command_line.h
+++ b/base/command_line.h
@@ -121,12 +121,19 @@
// ending with the argument placeholder "--single-argument %1". The single-
// argument switch prevents unexpected parsing of arguments from other
// software that cannot be trusted to escape double quotes when substituting
- // into a placeholder (e.g., "%1" placeholders populated by the Windows
+ // into a placeholder (e.g., "%1" insert sequences populated by the Windows
// shell).
// NOTE: this must be used to generate the command-line string for the shell
// even if this command line was parsed from a string with the proper syntax,
// because the --single-argument switch is not preserved during parsing.
StringType GetCommandLineStringForShell() const;
+
+ // Returns the represented command-line string. Allows the use of unsafe
+ // Windows insert sequences like "%1". Only use this method if
+ // GetCommandLineStringForShell() is not adequate AND the processor inserting
+ // the arguments is known to do so securely (i.e., is not the Windows shell).
+ // If in doubt, do not use.
+ StringType GetCommandLineStringWithUnsafeInsertSequences() const;
#endif
// Constructs and returns the represented arguments string.
@@ -215,6 +222,12 @@
// Append switches and arguments, keeping switches before arguments.
void AppendSwitchesAndArguments(const StringVector& argv);
+ // Internal version of GetArgumentsString to support allowing unsafe insert
+ // sequences in rare cases (see
+ // GetCommandLineStringWithUnsafeInsertSequences).
+ StringType GetArgumentsStringInternal(
+ bool allow_unsafe_insert_sequences) const;
+
#if defined(OS_WIN)
// Initializes by parsing |raw_command_line_string_|, treating everything
// after |single_arg_switch_string| + <a single character> as the command
diff --git a/base/command_line_unittest.cc b/base/command_line_unittest.cc
index 3077edd..a955971 100644
--- a/base/command_line_unittest.cc
+++ b/base/command_line_unittest.cc
@@ -320,6 +320,15 @@
cl.GetCommandLineStringForShell(),
FILE_PATH_LITERAL("program --switch /switch2 -- --single-argument %1"));
}
+
+TEST(CommandLineTest, GetCommandLineStringWithUnsafeInsertSequences) {
+ CommandLine cl(FilePath(FILE_PATH_LITERAL("program")));
+ cl.AppendSwitchASCII("switch", "%1");
+ cl.AppendSwitch("%2");
+ cl.AppendArg("%3");
+ EXPECT_EQ(FILE_PATH_LITERAL("program --switch=%1 --%2 %3"),
+ cl.GetCommandLineStringWithUnsafeInsertSequences());
+}
#endif // defined(OS_WIN)
// Tests that when AppendArguments is called that the program is set correctly
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc
index 7b1cb0ef..1ba8bac 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -430,7 +430,13 @@
cmd_line.AppendSwitch(switches::kVerboseLogging);
InstallUtil::AppendModeAndChannelSwitches(&cmd_line);
- AppCommand cmd(cmd_line.GetCommandLineString());
+ // The substitution for the insert sequence "%1" here is performed safely by
+ // Google Update rather than insecurely by the Windows shell. Disable the
+ // safety check for unsafe insert sequences since the right thing is
+ // happening. Do not blindly copy this pattern in new code. Check with a
+ // member of base/win/OWNERS if in doubt.
+ AppCommand cmd(cmd_line.GetCommandLineStringWithUnsafeInsertSequences());
+
// TODO(alito): For now setting this command as web accessible is required
// by Google Update. Could revisit this should Google Update change the
// way permissions are handled for commands.
