0

I have a python code which parses the input parameters like the following:

parser = argparse.ArgumentParser(description='Information injection in file.')  # , usage=usage())
requiredNamed = parser.add_argument_group('...')
requiredNamed.add_argument('--name', action='store', dest='myname', help='...', required=True)
requiredNamed.add_argument('--type', action='store', dest='mytype', help='...', required=True)
parser.add_argument('--enums', nargs='+', help='List to include')
args = parser.parse_args()

for the above code, Checkmarx shows High-Severity Error message for "args=parser.parse_args()" as the following:

The application's get_process_output method calls an OS (shell) command with Popen, using an untrusted string with the command to execute. This could allow an attacker to inject an arbitrary command, and enable a Command Injection attack. The attacker may be able to inject the executed command via user input, parse_args, which is retrieved by the application in the {} method...

the above python-code is used internally (not in a public environment like a web-site), so there will be no command-injection happen. so in this case for passing the Checkmarx scan, could you let me know any clue to update my python-code? (for example, is there other python-API I can use to replace the above "args=parser.parse_args()"? Thanks!

securecodeninja
  • 2,497
  • 3
  • 16
  • 22
boxu
  • 1
  • 1
  • 1
  • You're not thinking about this properly. The problem is NOT the `parse_args` call. The problem is the call to `subprocess.Popen`, which is launching a command that comes from those arguments, without filtering the string to remove dangerous parts. We can't see from this tiny snippet what information you're passing, but the SOLUTION is to filter the command you pass to `Popen` so it can't start obnoxious commands. Remove `|` and `||` and `&` and `&&` and `;` clauses, for example. – Tim Roberts Apr 30 '21 at 19:24
  • What I mean is filter the user's string to remove those clauses. If you need them for YOUR commands, that's fine. – Tim Roberts Apr 30 '21 at 19:25
  • 1
    @TimRoberts better yet would be giving `Popen` a list rather than a string, which avoids shell interpretation. – hobbs Apr 30 '21 at 19:26
  • For example, let's say one of your arguments is a filename, and you create a command line `"grep xyz " + filename` and pass that to Popen. Easy. But if the user provides a filename of `/dev/zero | rm -rf ~/*`, your code would delete all your files. – Tim Roberts Apr 30 '21 at 19:26
  • Thanks a lot for all your quick answers! OK got it I will add filters before "subprocess.Popen" Thanks TimRoberts and hobbs for all your help! – boxu Apr 30 '21 at 20:01
  • I added the following filters before calling "Popen", it didn't passed the weekly Checkmarx scan (it shows same error-message in scan-report), could you help me take a look so I could update the filter so I could pass the Checkmarx scan? Thanks!! ... #add filter for checkmarx-scan if args.header.find('|') != -1 or args.header.find('&') != -1 or args.header.find(';') != -1 or args.header.find('<') != -1 or args.header.find('>') != -1: exit(-1) – boxu May 07 '21 at 18:31
  • subprocess.Popen should always be called with shell=False when possible. That will eliminate the possibility of any kind of command-injection via special characters. See https://docs.python.org/3/library/subprocess.html#security-considerations. However it appears Checkmarx is not taking this into account. – bcd Jun 09 '21 at 18:06
  • @boxu - Did you resolve this issue ? – tkumaran Oct 31 '22 at 11:56

0 Answers0