Re: Security Issues

Apologies in advance to Zeev for arguing on this one, but be assured I firmly believe that your solution would be to the detriment of PHP and a better solution is possible :) On Saturday 28 July 2001 12:44, Zeev Suraski wrote: > > NO!! :) Saying people would stop using PHP (or won't get started) because > of this change is a gross exaggeration. IMHO, the one and only issue at > hand here is downwards compatibility, and not usability or ease of use, not > even a bit. Sorry Zeev - the answer is YES. I and no doubt thousands of others will turn register_globals on because it gives much more readable code, much less typing and does not IMHO add one jot to the security of my applications. > That's not going to find half, or a quarter, or whatever of the problems, > since PHP has tools to cleanly handle undefined variables - namely isset() > and empty(). They, or at least isset(), are quite popular. I always use something like: if(!isset($Thing) /*and possibly some range checking*/)) $Thing="sensible default"; In no way is if(!isset(_GET['Thing']) /*and possibly some range checking*/)) { $Thing="sensible default"; } else { $Thing=(_GET['Thing']); } any more secure (nor would it be if I wrote "sensible default" back to _GET. Anyway, to check my sanity i have reread the security advisory which I first read on the day it was published, and I am even more conviced now that register globals=off has the tiniest of effects for gpc variables wheras E_NOTICE has a massive effect. Here are the examples from the advisory: ------------------ <?php if ($pass === "hello") //= corrected to === $auth = 1; ... if ($auth == 1) echo "some important information"; ?> replace $pass with _GET['pass'] and the code is equally insecure. Turn E_NOTICE on and the novice programmer will get a warning message for the unset $pass. ------------------ <?php if (!($fd = fopen("$filename", "r")) echo("Could not open file: $filename<BR>\n"); ?> replace $filename with _GET['filename'] and this lunatic piece of code remains a lunatic piece of code. If $filename is not meant to be coming from the outside world then with E_NOTICE on there would be a warning message for the unset filename. ------------------ <?php include($libdir . "/languages.php"); ?> Ok, with register_globals=off then $libdir could not be directly overwritten from outside (unless there was some code which made that happen) however E_NOTICE would generate a warning for an unset $libdir ------------------ File upload. If you don't use the new functions you are potentially stuffed with register globals on or off. As an aside - we could have a php.ini directive which could 'break' code which did not use the new functions - if we save an uploaded file with one name, but set the $userfile_name to something else, and only rename the original to $userfile_name after a call to is_uploaded_file or move_uploaded_file. ------------------ <?php if (file_exists($theme)) // Checks the file exists on the local system (no remote files) include("$theme"); ?> This seems to be the same as the fopen($filename... example above. ------------------ In libdir/loadlanguage.php: <?php ... include("$langDir/$userLang"); ?> include files called out of context. The solution is to configure your web server, or put the include files outside the webroot so that they cannot be run out of context. In this example, typically $langdir should have been set inside the application or a configuration file, but $userLang will typically have come from the user either with this request, or previously and stored in a database in the users profile. So under the register_globals = off scheme, we would often end up with something along the lines of: <?php ... include("$langDir/_GET['userLang']"); ?> where $langDir is unset. We can all see how secure that is :) ------------------ Session files. I am happy to concede that there should be a php.imi directive which stops variables which can more or less be trusted, such as env and session from entering the global namespace, so that if you want to read them you have to look in the correct place. ------------------ In conclusion I would urge those who want to set register_globals=off to reconsider. A better solution is required. The better solution involves some, all, or more of: E_WARNING more granularity to the register_globals directive the file upload changes I mentioned as an aside Cheers -- Phil Driscoll