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.
<head>
<?php
$theme = $_GET['theme'];
$choice = preg_replace('/[^A-Za-z0-9\-]/', '', $theme);
if (empty($choice)){
echo '<link rel"stylesheet" type="text/css" href="css/default.css" />';}
elseif (file_exists("./css/" . $choice . ".css"))
{echo '<link rel"stylesheet" type="text/css" href="css/'.$theme.'.css" />';}
else {echo '<link rel"stylesheet" type="text/css" href="css/default.css" />';}
exit();
?>
<!-- other elements in head -->
</head>
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.cssfile
-
1\$\begingroup\$ Why not use a switch statement and hard-code the possible theme css files? Surely there cannot be that many. \$\endgroup\$76484– 764842015年03月09日 01:37:40 +00:00Commented Mar 9, 2015 at 1:37
2 Answers 2
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"><script>alert(1);</script>, 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 validFile0円" 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):
<?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" />';
// @author Darik
$theme = isset($_GET['theme']) ? preg_replace('/[^A-Za-z0-9\-]/', '', $_GET['theme']) : 'default';
$choice = ['theme1' => 'lightbluecss', 'theme2' => 'darkbluecss', 'default' => 'default']; // list availeble [theme name => file name] here (make sure they exist)
if (isset($choice[$theme])){
echo '<link rel="stylesheet" type="text/css" href="css/'.$choice[$theme].'.css" />';
}
else{
echo '<link rel="stylesheet" type="text/css" href="css/default.css" />';
}
?>
<!-- other elements in head -->
I think this is more robust for what you are trying to do, because this will eliminate the need of file exist check, security issues, and also limits the request to the available themes listed in the $choice array; if not it will fall back to default.
As you can see the theme name doesn't has to be quite literally the file name so you can make the theme names prettier and it improves security.
I just hope this code is helpful and readable to you.
My personal advice is unless these theme files have a massive difference between them you should do it at the CSS selectors level rather than switching the whole CSS file. It will save you a lot.