Re: Security Issues

At 06:01 28/07/2001, Phil Driscoll wrote:

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.

I have no doubt that thousands would turn it back on. I can't do anything about it, and as I said numerous times in numerous metaphors, I'm quite alright with that. I also have no doubt that the alternative ways we'll provide would result in a firm 'NO', and definitely not a 'YES'. I can't imagine a single person stopping to use PHP because the default changed (why the heck not just change it back, isn't it easier than learning a whole new alternative platform?). I also can't imagine people avoiding PHP because variables are accessed using $_FORM['foo'], instead of $foo. People are not *THAT* dumb or lazy. Discussing this issue in the OSCon, Rasmus claimed that right now he can teach PHP to a monkey in 3 hours, and he wouldn't want to be limited only to smart Gorillas in the future. I firmly believe that if a monkey can figure out $foo, $_FORM['foo'] is not going to be the showstopper.

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.

Any programming language gives you a long enough rope to hang you. I argue that PHP gives you a hanging loop and places it on your neck, leaving you only a couple of steps away from breaking it. Your example clearly states a piece of code where at the end, you have $Thing defined. In the 2nd example, it's clear beyond a reasonable doubt that it may come from user input, because it's written explicitly. However, if we turn $Thing back to $Auth, and add it in context: /* part 1 */ if (sth) {

        $Auth = 1;

} /* end part 1 */ ...a few dozen lines of code... /* part 2 */ if (isset($Auth)) {

        ... do something ...

} else {

        ... do something else ...

} /* end of part 2 */ In this case, having written part 1, it's not clear *at all* that Auth may be coming from user input. Should it be? Yeah. But as I've been repeating myself again and again in the last few days, as the advisory said, effectively it is NOT. Even Brian, which opposes the register_globals=off thing, found quirks in his code that he did not anticipate because of the state of mind PHP puts you in. In part 2, you're completely vulnerable to attacks. If part 1 had to be explicit, the danger would have been a hell of a lot clearer. So yes, you can hang yourself still, you're just a few feet farther away from the hanging loop now, and it comes with a warning sign.

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.

No it is not. $auth can no longer come from user input, so it's safer. $pass being unset is not much of a problem.

------------------ <?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.

How is that security? And what if I pass 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

Why would libdir be unset? The issue here is that under some circumstances, you can override variables. Different things happen in different paths of your logic, so testing is a nightmare if you just rely on the E_NOTICE feature.

------------------ File upload. If you don't use the new functions you are potentially stuffed with register globals on or off.

Having written the new functions, I don't recall why you'd be stuffed with register_globals=off. As far as I recall, the entire problem was a direct result of register_globals=on, and not enough track_vars functionality available at the time to write secure code. It was a while ago, so I could be wrong, though.

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.

Again, I'm not exactly sure what the security risk here is, but I just woke up :)

------------------ 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 :)

But that's running towards the hanging loop from a mile away, wrapping it quickly around your neck and pushing the lever. You can't do much about suicidals :)

------------------ 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.

Cool :)

------------------ 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

I think your examples were very good reasons why in fact register_globals should be set to off by default. I think that in the state of mind you're in, you're no really accounting for the state of mind that most PHP developers are, and thus, making a mistake by assuming that turning register_globals off is not going to have a far reaching security improving effect. Even if people just go over their code and explicitly declare which variables are coming from the user space, even that alone is going to help them audit their apps to a much higher level of security than it is today. Zeev