-2

We use Checkmarx to check our project.

The result show Environment.GetCommandLineArgs() may get an untrusted string and could allow an attacker to inject an arbitrary command.

var args = Environment.GetCommandLineArgs();

var ls = new List<string>(args.Skip(1).Select(arg => QuoteDotNetCommandLineArg(arg)));

Process.Start(new ProcessStartInfo()
{
    FileName = args[0],
    Arguments = string.Join(" ", ls.ToArray()),
    UseShellExecute = false
});

how to prevent it?

securecodeninja
  • 2,497
  • 3
  • 16
  • 22
zhangyujason
  • 1
  • 5
    Validate `args` before doing anything else? – SᴇM Apr 21 '21 at 07:27
  • On an higher level you should consider how the tool performs its evaluation. It is based on [static analisys](https://en.wikipedia.org/wiki/Static_program_analysis) or [dynamic analisys](https://en.wikipedia.org/wiki/Dynamic_program_analysis)? – Steve Apr 21 '21 at 07:35
  • I guess, I am missing something: This Program calls _itself_ with the same arguments just somehow quoted?? => _"Returns An array of strings where each element contains a command-line argument. **The first element is the executable file name**, and the following zero or more elements contain the remaining command-line arguments."_ – Fildor Apr 21 '21 at 07:43
  • _"how to prevent it?"_ - what does Checkmarx documentation say about that? – Fildor Apr 21 '21 at 07:45
  • This could allow an attacker to inject an arbitrary command, and enable a Command Injection attack. – zhangyujason Apr 21 '21 at 07:48
  • Yes, it _could_. How do the Checkmarx guys suggest to mitigate that? Or don't they go into that at all? – Fildor Apr 21 '21 at 07:55
  • I can't find any suggestion for it in report from Checkmarx. The report just marked it as a high severity risk. – zhangyujason Apr 21 '21 at 08:00

1 Answers1

0

Checkmarx is picking up on the fact that you are allowing the user to run an arbitrary application from your program. If this is the correct behaviour and not a security risk then you can go to Checkmarx and flag the warning to be ignored.

open-collar
  • 1,404
  • 1
  • 16
  • 22
  • I think it's a security risk. I don't know how to fix it. – zhangyujason Apr 21 '21 at 07:51
  • @zhangyujason What do you mean "you think"? Deep dive into your code and find out if it actually is a risk. By the code given in the question, I cannot even make sense of the implementation. – Fildor Apr 21 '21 at 07:57
  • I'm a new hire for the project. This code doesn't make sense to me either. – zhangyujason Apr 21 '21 at 08:08
  • 1
    If you're a new hire for a project, and you're set to tackle a potential security risk you cannot make heads nor tails of, you seek help from your fellow colleagues. Security risks are not something you should be taking lightly, and neither should the company you work for. – Lasse V. Karlsen Apr 21 '21 at 08:10
  • 1
    @zhangyujason I second Lasse. It's really no shame to seek help from someone more senior in this type of situation (or at all for that matter). There have been more than one occasion in my carreer, where I approached someone with code that made me scratch my head and they said "Oh, right. That's actually not used at all. Just throw it out." - might save you time, effort and grey hair to just ask. – Fildor Apr 21 '21 at 08:15
  • I never think seek help from my colleagues is a shame. The situation is this project hasn't been upgraded for serveral years. Nobody knows it. And I just try to find out if this is a risk and how to prevent it. – zhangyujason Apr 21 '21 at 08:22