First of all, it is very bad practise to use something like this in the tests:
class Popen:
def communicate():
return ["ping: bad address 'secdim'"]
Secondly, the right answer for this is:
p = subprocess.Popen(['ping', '-c', '1', url], shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
result, error = p.communicate() # This will give us 2 objects during the list comprehension
This one, of course, does not work.
And what if I don’t want to use Popen at all? Maybe I want to use check_call or something similar?
By the way, I’ve got this working locally by doing next thing:
def open(request):
url = request.GET["url"]
p = subprocess.Popen(['ping', '-c', '1', url], shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
result = p.communicate()[0]
if not len(result) or 'bad address' in result:
return HttpResponseBadRequest("Bad request")
But it is still bugged on the Secdim side during the tests.
Hi there, I can confirm that the auto-tests are working as intended when code is pushed.
From your patch (in your comment) it appears you are not performing any validation on the user supplied domain parameter prior to passing it to the ping command. This has not effectively addressed the vulnerability in this challenge and server side tests will fail.
If you would prefer to use ‘check_call’ that should be fine provided that you are returning the identical output as the current ‘Popen’.
Thanks for pointing the p.communicate() issue, that should return two objects.
Fair point for check_all and to put you at the right direction on which function to pick (as we cannot cater for all possible functions that a player may decide to pick in this challenge), example of the mocked test is in your local test to put you in the right direction.
As Matt correctly pointed out, vulnerability should be treated differently and the issue should be addressed prior to the Popen, otherwise, the damage is already done by the adversary. There also some other ways to address it and some ways to take shortcuts which we are aware.
Regarding tests, they are intentionally becoming less verbose as you progressing toward harder challenges.
I hoped this has clarified some of the teething issue for you. If you have any other questions, feel free to give us a yell.
It is fix. You will not able to perform command injection using shell=False flag. If task is not about the command injection and is about the host resolving - you must point about that in the task.
If you would use syntax, described by me, that will not be the threat anymore. If the service is going to ping any host - it is not a bug.
Regardless of the ‘shell=false’ flag, before passing user input to a command executed by the OS or container in this instance it is best practice to perform checks on the user supplied input to ensure it is a valid domain being supplied to the ping command and not allowing other potentially malicious characters in the string.
While these challenges are educational, they do not always abide by best practices (shell=True in this case). They will however follow defensive programming principles which involves validating user supplied input prior to passing it to another function or process .
Whilst your patch may work, it is not entirely correct in solving this challenge.
You are not correct abut the ‘command injection’ here at all.
Your sevice (task) by design allows users to interact with the ‘ping’ utility. To harden that you can:
Use some custom filters to disable the opportunity of the command injection, eg some symbols which will allow attacker to compromise server or execute arbitrary commands;
Use some language specific flags/methods and etc. in this case, ‘Popen’, by default, uses flag ‘shell=False’ which will not allow attacker to confuse ‘python’ script, using the syntax from my answer, and execute non-ping command. In this case hacker will be able to perform some pings to the inner network, but in this case you need to add something to the task regarding to the ‘white lists’ and ‘black lists’. This is a risk, not a vulnerability.
Sorry if some of my comments have come across as confusing or I have used incorrect terms.
Your points in regards to hardening are great. Number 1 is where you want to head with this challenge, custom filter to validate the string supplied as the domain is of valid format and check for symbols that can be used maliciously.
Number 2, would create an even better patch, running the shell in the default mode further mitigating the risk of command injection. Whilst pinging the internal network is a risk, it is out-of-scope for this challenge.
As I mentioned above, this challenge can be solved in other ways as well. The expected way is via enforcing URL schema validation against the input. That is to prevent only a single avenue of attack which is arbitrary command execution. Is this enough? Maybe. Do we need to do more? Yes. Setting shell=False is another layer of defence that as I described in the article, separates data from code. Anytime we mix data with code, we can have this insecure design pattern. Are there still other vulnerabilities in the app after this fix? Yes! but it is not the focus.
In reality, we refactor the code and completely remove any system call. External service calls can be done in a lot safer way than what we see in this easy challenge. The point here was focusing on getting a robust URL schema input validation and that’s why Popen is mocked.
We are all learning in this community. We aim to be constructive and helpful. I may also suggest, to take a look at the Play SDK and refactor this challenge with the tests that you think most suit. I can offer to review and publish it for you on SecDim.