search

Codiad / Codiad

Web Based, Cloud IDE
http://codiad.com
MIT License
2.84k stars 696 forks source link

Download.php command injection #808

Open wireghoul opened 9 years ago

wireghoul commented 9 years ago

Hi there,

It appears your download script suffers from command injection, to illustrate the issue create a directory named ";uname -a" and try to download it.

Cheers, Wireghoul

evertton commented 9 years ago

The problem is here: -deleted by daeks- Sanitize the files and directories names should be sufficient. We can sanitize using this regular expression [a-zA-Z0-9\s_-]+.

daeks commented 9 years ago

Yeah, I have already seen it, but marked as minor cause it requires some prerequirements which are not critical. There is already a function like that available in the controller, so I would like to use that one.

Anyway, please dont post security issues or examples to public github issues :) - pull in a PR or by email ;)

wireghoul commented 9 years ago

I would, but your website doesn't list any email contact at all. The issue requires an authenticated session and most importantly searching github for the words "system" and "$_GET" will serve up the vulnerable code. Combined with the number of bots that scrape github for this type of coding practices it seemed reasonable enough to post it to your issue tracker.

daeks commented 9 years ago

Posting that issue was quite good, just removed the direct link from evertton :) I had a look at the webpage itself, you are right. it is not mentioned there (only on github https://github.com/Codiad/ and on facebook)

evertton commented 9 years ago

Oh, sorry! I completely forgot that we should not show bugs before correction. :dizzy_face: Please, check my PR. :smile:

wireghoul commented 9 years ago

Unfortunately the fix is flawed, escapeshell args does not cover argument injection and specifying a command to run with the -I argument ($_GET['path']='-I /bin/evil' dir/) will let an attacker divert the tar process to execute a command of their choosing. See the following article for more details: http://www.cso.com.au/vendor_blog/14/bae-systems-detica/5342/security-issues-with-using-phps-escapeshellarg/ The whitelisting sanitation approach initially suggested was a much safer solution to this problem.

daeks commented 9 years ago

Personally I would remove the complete system part and let the zip extension do the job (as it is already included, but not as primary option)

evertton commented 9 years ago

Please check this new PR. :smile: