I just read this article '1% of CMS-Powered Sites Expose Their Database Passwords' by Feross Aboukhadijeh and immediately thought about Drupal. I realise the article is a little old and that yes of course people shouldn't edit these files on the server but I can guarantee it happens in the wild.
I've not been through each of the checks that Security Review makes in detail but I assume that adding some checks against settings.php~ and the like would be best done as part of a newly titled check rather than bundled with an existing check?
But I suppose the first question is should this be a check available in Security Review in the first place, and second question if the first applies, is shall I have a got at writing a patch?
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 1907618-security_review-temp-files-11.patch | 676 bytes | coltrane |
| #7 | 1907618-security_review-temp-files.patch | 5.21 KB | coltrane |
| #5 | 1907618-security_review-temp-files.patch | 4.94 KB | coltrane |
Comments
Comment #1
coltraneYes, a new check for Security Review would be a great. If you would like to work on that please assign this issue to yourself. Let me know if you have any questions or run into any issues, I idle in #drupal on IRC freenode.
Comment #2
ghazlewoodThanks coltrane, will have a stab at it later this week and see if I can get it working. Thanks for the offer of assistance, will fire up IRC if I have any questions.
Cheers!
George
Comment #3
coltraneNote, #1907704: Restrict temporary files created by text editors is for adding a mitigation to core for this.
Comment #4
ghazlewoodComment #5
coltraneThanks @ghazlewood, I appreciate you thinking of Security Review and creating this issue. If you would, I'd love a quick review of this.
So far just checks for settings.php~ Are there other files we should check for?
Note, I moved a function so this patch has some extra changes.
Comment #6
ghazlewoodLooks great to me, my only thoughts at this stage would be about checking for instances of some of the other possible file names as well as settings.php~
The regex in the core issue refers to:
.php(~|\.sw[op]|\.bak|\.orig\.save)$"
When I'm not using a tablet I'll apply the patch and give it a proper review too...
Thanks for resurrecting this @coltrane!
Comment #7
coltraneGood point @ghazlewood, I've updated this patch to check for those file extensions.
Comment #8
coltraneCommitted http://drupalcode.org/project/security_review.git/commit/9996e03
Comment #9
ghazlewoodSorry to be a bit slow on this coltrane but I just tried out the changes in your patch and created a set of test settings.php files with the following names:
settings.php~
settings.php.bak
settings.php.orig
settings.php.save
settings.php.swo
settings.php.swp
When I ran the review the only files that were picked up were:
settings.php~
settings.php.bak
settings.php.swo
settings.php.swp
I can't see why .save and .orig would be missed based on the preg_match myself, any ideas?
Comment #10
coltraneRe-opening, setting to CNW. Will review soon.
Comment #11
coltraneThe regex was missing an or ('|') modifier :/
Comment #12
ghazlewoodWorks great! Well spotted coltrane, took me a while to spot the difference between the lines in the patch!
Have tested this with some copies of settings.php and it works perfectly for all cases now.
Comment #13
coltraneCommitted http://drupalcode.org/project/security_review.git/commit/0ba46b4