Review processes started with a composed arguments string

Rule ND3124 Security

Matches methods that start a process with Process.Start(fileName, arguments), new ProcessStartInfo(fileName, arguments) or by assigning ProcessStartInfo.Arguments, while also building strings (String.Concat(), Format(), Join(), interpolation, StringBuilder) and never using the ArgumentList collection. A single arguments string is re-parsed by the target program (or by the shell when UseShellExecute is true): quotes, spaces, &, | or ; in an interpolated value change the command executed.

This is CWE-78 OS Command Injection (review: the rule cannot tell whether the composed fragments are user-controlled).

Flagged Code
public void Convert(string inputFile) {
   // flagged: re-parsed by ffmpeg, a quote in inputFile injects extra arguments
   var arguments = $"-i \"{inputFile}\" -o output.mp4";
   Process.Start("ffmpeg", arguments);
}
Fixed Code
public void Convert(string inputFile) {
   var startInfo = new ProcessStartInfo("ffmpeg") { UseShellExecute = false };
   startInfo.ArgumentList.Add("-i");
   startInfo.ArgumentList.Add(inputFile);  // quoted per argument, nothing is re-parsed
   startInfo.ArgumentList.Add("-o");
   startInfo.ArgumentList.Add("output.mp4");
   Process.Start(startInfo);
}

How to fix

Pass each argument through ProcessStartInfo.ArgumentList.Add(...) (escaping is then done per argument), keep UseShellExecute = false, and validate or whitelist any value that comes from a request, a file name or configuration. Suppress the issue when every composed fragment is a constant or an internally generated path.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review processes started with a composed arguments string</Name>
// <Id>ND3124:ReviewProcessesStartedWithComposedArgumentsString</Id>
warnif count > 0

// Third-party members are part of the code model only when the application uses them.
// Members taking the arguments as a single string that the target program (or the shell) re-parses:
let processStringMembers = ThirdParty.Types.WithFullNameIn("System.Diagnostics.Process", "System.Diagnostics.ProcessStartInfo")
   .ChildMethods().Where(m =>
      (m.SimpleName == "Start" && m.IsStatic && m.Name.StartsWith("Start(String,String")) ||   // Process.Start(fileName, arguments)
      (m.IsConstructor && m.Name.StartsWith(".ctor(String,String")) ||                        // new ProcessStartInfo(fileName, arguments)
       m.SimpleName == "set_Arguments")                                                        // startInfo.Arguments = ...
   .ToHashSetEx()
where processStringMembers.Any()

// ArgumentList quotes each argument: the safe API.
let argumentListGetter = ThirdParty.Types.WithFullName("System.Diagnostics.ProcessStartInfo")
   .ChildMethods().Where(m => m.SimpleName == "get_ArgumentList").ToHashSetEx()

// String composition in the same method: concatenation, formatting, interpolation, builders.
let stringBuildingMembers = ThirdParty.Methods.Where(m =>
      (m.ParentType.FullName == "System.String" && m.SimpleNameEqualsAny("Concat", "Format", "Join", "Replace", "Insert")) ||
       m.ParentType.FullNameEqualsAny("System.Text.StringBuilder", "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler"))
   .ToHashSetEx()

from m in Application.Methods.UsingAny(processStringMembers)
where !m.MethodsCalled.Intersect(argumentListGetter).Any()
let processMembersUsed = m.MethodsCalled.Intersect(processStringMembers).ToArray()
let composesStrings = m.MethodsCalled.Intersect(stringBuildingMembers).Any()
where composesStrings
select new {
  m,
  processMembersUsed,
  Debt = 15.ToMinutes().ToDebt(),
  Severity = Severity.Medium
}

//<Expl>
// The *{0}* starts a process through *{1}* with an arguments string
// composed in the method. If any fragment derives from user input, a
// crafted value can inject extra arguments or commands.
//</Expl>

//<Desc>
// Matches methods that start a process with *Process.Start(fileName, arguments)*,
// *new ProcessStartInfo(fileName, arguments)* or by assigning
// *ProcessStartInfo.Arguments*, while also building strings
// (*String.Concat()*, *Format()*, *Join()*, interpolation, *StringBuilder*)
// and never using the *ArgumentList* collection. A single arguments
// string is re-parsed by the target program (or by the shell when
// *UseShellExecute* is true): quotes, spaces, *&*, *|* or *;* in an
// interpolated value change the command executed.
//
// This is CWE-78 *OS Command Injection* (review: the rule cannot tell
// whether the composed fragments are user-controlled).
//
// **Flagged Code**
// ```csharp
// public void Convert(string inputFile) {
//    // flagged: re-parsed by ffmpeg, a quote in inputFile injects extra arguments
//    var arguments = $"-i \"{inputFile}\" -o output.mp4";
//    Process.Start("ffmpeg", arguments);
// }
// ```
//
// **Fixed Code**
// ```csharp
// public void Convert(string inputFile) {
//    var startInfo = new ProcessStartInfo("ffmpeg") { UseShellExecute = false };
//    startInfo.ArgumentList.Add("-i");
//    startInfo.ArgumentList.Add(inputFile);  // quoted per argument, nothing is re-parsed
//    startInfo.ArgumentList.Add("-o");
//    startInfo.ArgumentList.Add("output.mp4");
//    Process.Start(startInfo);
// }
// ```
//</Desc>

//<HowToFix>
// Pass each argument through *ProcessStartInfo.ArgumentList.Add(...)*
// (escaping is then done per argument), keep *UseShellExecute = false*,
// and validate or whitelist any value that comes from a request, a file
// name or configuration. Suppress the issue when every composed fragment
// is a constant or an internally generated path.
//</HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items