nituvious Posted October 24, 2010 Share Posted October 24, 2010 Hi guys, I wrote this speck of code to prevent directory transversal. However, I'm not that great with security issues, so I would like some of the gurus to offer pointers/tips/hints as to whether my code is safe or not and how to improve it. $pageID = $_GET["pageid"]; $pageNewIDLower = strtolower($pageID); $pageNewID = ereg_replace("[^A-Za-z0-9]","",$pageNewIDLower); if (strstr($pageNewID,"../") || strstr($pageNewID,"%") != true) { // do stuff } else { include("pages/home.htm"); } If this looks wrong, let me know. I didn't take it directly from my php code as I'm on a cell phone at the moment. Quote Link to comment Share on other sites More sharing options...
btherl Posted October 24, 2010 Share Posted October 24, 2010 Once you've removed all characters which are not alphanumeric, it's not possible for there to be any dot, slash or percent symbols left in $pageNewID. So the strstr() tests are redundant. I'm not sure if that regexp is doing what you want it to though. For example, it'll do these conversions ../../../etc/passwd => etcpasswd files/file_5.txt => filesfile5txt It'll remove file extensions as well as any legitimate directory names. I think you need to allow dot and slash in the regexp, and keep the strstr tests. Is this running on unix or windows? Quote Link to comment Share on other sites More sharing options...
nituvious Posted October 25, 2010 Author Share Posted October 25, 2010 Windows, but I would like to have a linux version as well. The ereg_replace() is doing what I intended. In my code I check through a folder if a page exists with separate extensions like .php, .txt, .htm. There's not supposed to be any underscore within the page names, but I could change that. I was concerned that it would still be possible for someone to transverse directories because I use include(); function. Quote Link to comment Share on other sites More sharing options...
btherl Posted October 26, 2010 Share Posted October 26, 2010 Ok I get it now. No, there is no chance, because you have removed all characters except letters and numbers already. That means that "../" has been removed by the regexp already. The regxp is all you need to be secure. You can add underscore to the regexp safely if needed. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.