Veracode still reports OS command injection issue after I have applied the solution as recommendations from OWAPS and Roslyn Security Guard

Question

Current code in my project is shown below and Veracode reports there is an OS command injection

filename = Regex.Replace(filename, "[^a-zA-Z0-9_]", "_") & ".svg"

ProcessStartInfo startInfo = default(ProcessStartInfo);
Process pStart = new Process();
startInfo = new ProcessStartInfo(myExecutedFilePath, "\"" + filename + "\" --export-pdf=\"" + filename + "\""); **//OS command injection raises at this line**
pStart.StartInfo = startInfo;
pStart.Start();
pStart.WaitForExit();

So, I research the solution to solve this issue from OWASP and Roslyn Security Guard.

• OWASP post: https://www.owasp.org/index.php/OS_Command_Injection_Defense_Cheat_Sheet
• Roslyn Security Guard post: https://dotnet-security-guard.github.io/SG0001.htm

And here is my code after modifying based on that posts.

filename = Regex.Replace(filename, "[^a-zA-Z0-9_]", "_") & ".svg"

ProcessStartInfo startInfo = default(ProcessStartInfo);
Process pStart = new Process();
startInfo = new ProcessStartInfo();
startInfo.FileName = myExecutedFilePath;
startInfo.Arguments = "\"" + filename + "\" --export-pdf=\"" + filename + "\""; **//Veracode still reports the issue at this line**
pStart.StartInfo = startInfo;
pStart.Start();
pStart.WaitForExit();

BUT, Veracode still reports OS command injection.

So my concerns here are:

1. Did I apply the correct solution to solve OS command injection in this case?

2. Or, Should I propose mitigation for it?

Answer 1

I have received the answer from Veracode.

"You are right that separating the file path and arguments in the ProcessStartInfo object is a good beginning and that validating the filename to only include alphanumeric characters should also help.

The likely reason the static engine is still reporting this as a flaw is that Veracode doesn't recognize any cleansing functions for .NET for CWE 78. Because of this, any time we see user input being passed to a function that represents a command "sink" we will flag as CWE 78. We also don't evaluate the accuracy/efficacy of regex strings, so even if the regex were completely accurate we would still flag the flaw.

Two recommendations:

1. Consider scheduling a consultation if you want one of our application security consultants to help validate that your changes are correct in context.
2. Once you're 100% comfortable that your fix addresses the flaw, I recommend documenting this in a mitigation. "

Answer 2

I suppose filename is user input.

This is technically OS command injection, but you can argue it's "mitigated by design" in Veracode terms, because the filename is strongly validated (T: M1, S: Only letters and numbers are valid characters for the filename, others will be replaced).

However, this raises questions of another vulnerability, external control of filename or path (aka. path injection). While validation in your code will mitigate most of it, consider edge cases like an already existing file, or a filename consisting of a null byte (\0), or an empty filename.

For example your application could invoke this pdf export executable to offer the resulting pdf for the user to download. If called with a filename that already exists, the new pdf may not be created, but the existing one could be offered for download wihout authorization. This is just an example, similar problems may arise right now, or in the future as your code evolves.

Answer 3

This is an old question, but I wanted to give you a solution to the issue incase you or others have this issue like I did. Basically you did not mitigate the issue properly.

In general this flagged is because you did not pass the full file path with the variable being tested in a regex for filtering.. you only put a bare file name in the test. Using the full filename such as c:\myfile.svg instead of just myfile.svg in the test will fix half the problem. I say Test, because it should be a regex test and not a simple replace to enable the process to continue. There is also a related issue where process.start() can be exploited and to reduce exposure you should also put a regex on the file and args as demonstrated below

string _filename = myExecutedFilePath +"\"+ filename + :.svg"; 
string _args = filename + "\" --export-pdf=\"" + filename + "\"");
// double check the regex patterns :)
string _pattern_file = "^(?:[\w]\:|\\)(\\[a-z_\-\s0-9\.]+)+\.(svg)$";
// next regex assumes the args will be something like
// "file.svg" --export-pdf="file.svg"
string _pattern_args = "^\"(?:[\w]\:|\\)(\\[a-z_\-\s0-9\.]+)+\.(exe|msi|bat)\" --export-pdf=\"(?:[\w]\:|\\)(\\[a-z_\-\s0-9\.]+)+\.(exe|msi|bat)\"$";
Match _matchFileName = Regex.Match(filename+".svg", _pattern_file, RegexOptions.IgnoreCase);
Match _matchArgs = Regex.Match(_args, _pattern_args , RegexOptions.IgnoreCase);

if(_matchFileName.Success && _matchArgs.Success){
    ProcessStartInfo startInfo = default(ProcessStartInfo);
    Process pStart = new Process();
    startInfo = new ProcessStartInfo();
    startInfo.FileName = myExecutedFilePath;
    startInfo.Arguments = _args
    pStart.StartInfo = startInfo;
    pStart.Start();
    pStart.WaitForExit();
}else{
    // TODO: Add Exception Handling for unable to start process 
}