patternphpMinor
PHP echo GET to change a CSS theme file
Viewed 0 times
filecssphpgetthemeechochange
Problem
I have the follow code to change between different CSS theme files, I would like some reviews on it because I am sure it can be improved to be safer.
The functions are:
-
If GET string empty set CSS default file
-
If GET non existing CSS file set CSS default file
-
If GET existing CSS file them we use the echo
';}
elseif (file_exists("./css/" . $choice . ".css"))
{echo '';}
else {echo '';}
exit();
?>
The functions are:
-
If GET string empty set CSS default file
-
If GET non existing CSS file set CSS default file
-
If GET existing CSS file them we use the echo
GET.css fileSolution
Security
It's a little odd that you clean the file for the
So I can inject
Up to version 5.xx of PHP, it was possible to inject a null byte to bypass
So I didn't find any vulnerabilities, but I still don't like your approach all that much. At the very least I would echo the replaced file name instead of the original. I would actually prefer the approach suggested by @76484 (hardcode and use a switch or use an array and then check if the filename exists in it), because using whitelists is always safer than using blacklists.
tl;dr: I couldn't find a vulnerability, but would still use a different approach.
Misc
So your code would be cleaner like this (but still using a blacklist instead of a whitelist for security):
It's a little odd that you clean the file for the
file_exists check, but not for the echoing. So I can inject
test">alert(1);, and if the file testscriptalert1script exists the alert is executed. This isn't an attack (well, at least as long as users can't upload their own custom theme), but still seems odd.Up to version 5.xx of PHP, it was possible to inject a null byte to bypass
file_exists, but that should be caught by your filter, so it shouldn't enable the attack described above, or any other attacks (such as injecting validFile\0" href="evil" foo=" and hoping there are browsers which choose the second href attribute if two are present).So I didn't find any vulnerabilities, but I still don't like your approach all that much. At the very least I would echo the replaced file name instead of the original. I would actually prefer the approach suggested by @76484 (hardcode and use a switch or use an array and then check if the filename exists in it), because using whitelists is always safer than using blacklists.
tl;dr: I couldn't find a vulnerability, but would still use a different approach.
Misc
- your indentations, newlines and spacing seem rather random, which makes your code a bit hard to read.
- if you exit in the head, your website will be quite boring.
- it should be
rel="stylesheet".
choiceisn't a good variable name. Something likecssFileNamewould be much clearer.
- you repeat the inclusion of the default CSS file. Either extract that to a function, or just remove the empty check (assuming that
.cssdoesn't exist).
- but then you still have the inclusion code for CSS in general three times.
So your code would be cleaner like this (but still using a blacklist instead of a whitelist for security):
';Code Snippets
<?php
$cssFileName = $_GET['theme'];
$cssFileName = preg_replace('/[^A-Za-z0-9\-]/', '', $cssFileName);
if (empty($cssFileName) || !file_exists("./css/" . $cssFileName . ".css")) {
$cssFileName = "default";
}
echo '<link rel"stylesheet" type="text/css" href="css/' . $cssFileName . '.css" />';Context
StackExchange Code Review Q#83589, answer score: 4
Revisions (0)
No revisions yet.