2

I receive a Veracode error when running the static scan: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection') (CWE ID 78)

The application calls a process with an argument that I receive from the frontend (the application is used internally and this is a userId) .

  ProcessBuilder pb = new ProcessBuilder(PROCESS, "-A", argument);
  Process p = pb.start();     // Veracode is mentioning this line

How could I manage to fix this Veracode issue ? Is there a 'safe' way to run a process?

pri
  • 1,521
  • 2
  • 13
  • 26
Sorin Penteleiciuc
  • 653
  • 1
  • 10
  • 26

2 Answers2

2

Presumably your userId has a well defined format (numbers, hexadecimal digits, alphanumeric, ...), perhaps it is always the same length.

You have to verify it by matching userId to the appropriate class of characters via regex, and reject anything which is not complying, otherwise, you are open to the following attack:

Enter Username: diginoise; rm -rf /

diginoise
  • 7,352
  • 2
  • 31
  • 39
  • I have added a check but Veracode still sees this as an error. Like there is preapred statement instead of statement, I wonder if there is something like "prepared process execution" where I run only one single process in a command. – Sorin Penteleiciuc Nov 10 '17 at 09:34
  • it is entirely possible that Veracode deems any out of process execution as vulnerable and will always shout about it. What exactly is the command you are trying to run? – diginoise Nov 10 '17 at 10:08
  • It is exactly the one from above. I tried scaning with constants and I did not receive the issue. – Sorin Penteleiciuc Nov 10 '17 at 10:13
  • @sorinpenteleiciuc I meant what external command are you executing. In your snippet you just use (constant?) `PROCESS`. Would you also share how you sanitise the input? Veracode could be shouting as soon as it sees a variable passed in - I am not sure how sophisticated it is. What it should do is to provide you with remedy steps to follow or detailed explanation. – diginoise Nov 10 '17 at 10:19
  • I used "grep" for testing purpose. It is not about the process I run, it is about the fact that I use a command which is parameter. – Sorin Penteleiciuc Nov 10 '17 at 10:25
  • the reason I asked about the `PROCESS` is that oftentimes what you are trying to achieve could be achievable from within Java code - otherwise you are making your application much less portable. – diginoise Nov 10 '17 at 11:11
  • I know, but here is a problem that I pass this argument from the UI. – Sorin Penteleiciuc Nov 10 '17 at 11:44
1

Sounds like it's an architectural problem in your application. I'm pretty sure you don't want to execute a supposed userid passed by the user as a request parameter as an OS command. This would be OS command injection by design.

The ideal solution would be to avoid creating a new OS process and use built-in Java functionality to achieve your goal.

If you do have to run an external process, do not include user input into what you are running. For example if you had the static string ps aux to run and would do the "grep" bit in Java, the Veracode finding would go away and it would be a lot more secure.

If you absolutely must include user input, make sure it is very strictly validated. Note that for OS command injection, letters only may sometimes be enough, and Veracode will correctly flag that as vulnerable, despite validation being in place. In this case, if you are sure that with your validations, it is not possible to run anything malicious, you can mark the finding in Veracode as "mitigated by design".

Gabor Lengyel
  • 14,129
  • 4
  • 32
  • 59