str_reaplce is only handling . and /
From what I can tell by quickly glancing at your code it's more secure but still not ideal IMHO
Code: Select all
$file = str_replace('.', '', $_GET['p']);
$file = str_replace('/', '', $file);
Test: $p = 'mytest/test.php';
Output:
mytesttestphp which is now a broken file path and will choke your script when file_exists() is called, so yes it's secure in that regard...but it still is not ideal.
Also consider that someone may be able to escape the file path delimiters like '.' or '/' in fact, I'm not sure but a user could possibly use '\' instead of '/' and acheive the same results on Windows servers at least. As a path seperators can be either forward or backward slashes...
And then, there is the issue of specially encoding characters, I'm not sure if this is possible, but like SQL injection attacks use mysql_escape_quotes instead just escaping the quotes...it's possible a similar attack could be carried out on your string for file paths...
Perhaps the resident multi-language expert AC could shine some light on how PHP deals with international strings as I'm sketchy myself...
But if it's possible to encode a period or slash as an HTML entity or similar, that would easy bypass your security checks if PHP knows or can be tricked into handling international strings (not sure by I'm guessing setlocale() it changes how PHP handles strings???)
for instance lets pretend the internatinal code for period or slash in Farsi is as such:
&7366; and the user can then somehow set the PHP locale to Farsi
your scripts assume ASCII period and slashes, and completely ignore the multi-byte coded periods/slashes, so imaginary data input as follows:
&7364; = Slash
&83476; = Period
Test: $p = 'mytest&7364;test&83476;php';
Would reach the PHP include function, but I *think* if the user can also set the locale to Farsi, then PHP include would then be capable of including again, whatever the attacker wanted to inject
Test: $p = 'mytest&7364;&83476;&83476;&7364;&83476;&83476;&7364;test&83476;php';
Ignored by your script, completely legitimate attack - I think...this is atleast something worth investigating as a truth because it's I belive the gist of the idea behind SQL injection attacks, using international codes instead of English ASCII codes...
I don't think I considered that in my own code either...I'll have to look into this
Cheers
