Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starting a process with a shell, possible injection detected, security issue. #214

Closed
ikostan opened this issue Jul 3, 2020 — with Codacy Production · 2 comments
Assignees
Labels
bug Something isn't working Codacy Code quality automation code quality codewars issues Security security issue

Comments

Copy link
Member

ikostan commented Jul 3, 2020

Codacy detected an issue:

Message: Starting a process with a shell, possible injection detected, security issue.

Occurred on:

Currently on:

@ikostan ikostan self-assigned this Jul 3, 2020
@ikostan ikostan added bug Something isn't working Codacy Code quality automation code quality codewars issues Security security issue labels Jul 3, 2020
@ikostan
Copy link
Member Author

ikostan commented Jul 3, 2020

Some useful info here

Rather than passing a string to subprocess, our function passes a list of strings. The ping program gets each argument separately (even if the argument has a space in it), so the shell does not process other commands that are provided by the user after the ping command terminates. You do not have to explicitly set shell=False - it is the default.

@ikostan
Copy link
Member Author

ikostan commented Jul 3, 2020

Even more relevant topic here

It has security issues just when you run the function with arguments taken from users. For example:

import os
def do_clear(command): # Notice command is sent as argument from outside world and hence this makes it vulnerable
    os.system(command)

If the method is called with for example:

do_clear('rm -f */*')

Then it is possible that it deletes all the files of current directory. But if the 'clear' command is to be directly used, you do not have to worry about the security issue, as only 'clear' is run in all conditions. So the following function is secure enough.

def do_clear(): # Notice command is not sent as argument from outside world
    os.system('cls' if os.name == 'nt' else 'clear') # This is not risky as os.system takes clear/cls command always.

@ikostan ikostan closed this as completed Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Codacy Code quality automation code quality codewars issues Security security issue
Development

No branches or pull requests

1 participant