[FIX] security: OS Command Injection vulnerability (x2) #1219

[0xtejas]

This code is resistant to command injection because it uses the subprocess.run() function with a list of arguments instead of a single-string command.

When you pass a list of arguments to a subprocess.run(), Python will not invoke a shell to execute the command. This means that shell metacharacters such as ;, &&, || etc., which could be used to inject additional commands, are treated as literal characters and not interpreted by the shell.

In this case, the URL variable is passed as a separate argument to the command, so even if it contains shell metacharacters, they will not be interpreted as such. This makes the command resistant to command injection attacks.

I have re-tested using the POC and it did not spawn the shell. However, I'd suggest performing a little more extensive test. Let me know if you find anything.

[fopina]

@yogeshojha security vulnerabilities in security products should have more priority than other products 😄

[yogeshojha]

Hi @0xtejas Thank you for reporting this.
However instead of using new subprocess command, we have run_command function that solves this issue. I will use this MR and make changes.

Thank you again for reporting.

[yogeshojha]

@0xtejas @sa7mon thank you for reporting this and also sending a PR.

If any of you have time, can you please go through the changes and test it again?

Hopefully, there shouldn't be any issue because run_command is one of the functions that we have used almost everywhere to run the command, and I remember testing this in the path. Under the hood, it uses subprocess.Popen

Let me know if you find something, if not I will merge this tomorrow.

[fopina]

Hi @yogeshojha to comment on the run_command function: while using str.split and shell=False resolves the injection issue, it breaks functionality, eg: scanner -n "quoted arg" bla1 bla2. This likely fails in that code.

There's shlex.split that you could use there to better split the strings, but usually that's only used when you really need to take a full command line as input.

If you only take single arguments as user input, it's cleaner, more secure and more performant to use all the execv family functions by calling subprocess with a manually-controlled/hardcoded list of arguments (and shell=False).

I plan to change my fork to do that:

• change all calls to pass list of args instead of command line str
• store commands in DB with json.dumps of command line instead

Would that change make sense to push upstream?

[yogeshojha]

Hi @fopina sounds good to me.

For this issue, I will go ahead and continue to use run_command, since this function is being used almost everywhere, when you make changes, I guess testing it thoroughly will take some time.

So until then, I will be going ahead with this approach and will be waiting for your changes.

Thanks