Re: Security Issues

At 03:09 27/07/2001, Rasmus Lerdorf wrote:

At 01:35 27/07/2001, Rasmus Lerdorf wrote:

I think you missed my point. People use empty() and isset() on a variable to check to see if that variable was set by the user. As such that variable is unclean and whether it came in via register_globals or not is quite irrelevant. If it is unclean it is unclean. It doesn't matter at all which mechanism (GET/POST/COOKIE) populated the data from a security perspective.

I got your point, but I disagree on it. I think that lots of users use isset() for implementing the same logic you demonstrated in your example, only in a clean, E_NOTICE-free way. I know I used to do that, Heikki says he does, and my assumption is that it's a very common practice. I think that assumption is wrong. And also your previous assumption that the E_ALL error_reporting level change would break just as many scripts as the register_globals change.

I didn't say just as many, but same order of magnitude. Most of the scripts I came across in the past 4 years would not run properly with E_NOTICE's turned on. Even if it's just 20% of the scripts, it's still the same order of magnitude. No doubt, the number would be less, since turning register_globals off would break virtually all apps out there today, but it'd be a major blow still. I argue that the security benefit from it is much smaller than doing the real thing, which is turning register_globals off.

The way to figure this out would be to actually go and check instead of making such broad assumptions. Take the first 25 projects listed at php.net/projects.php, install them and run them with E_ALL and then with register_globals=off and see how they do. It's almost 3am, and I am rather tired, but I grabbed the first one listed and installed it. Then clicked through it and did various things. So here is my entry for it if someone wants to actually take on this task of auditing the first 25 (or more) projects:

I think that's a good idea, but given the fact that the advisory mentioned exploits for some very common apps means that either - These apps are not E_NOTICE 'safe', otherwise, these issues would have been resolved or - These issues would not have been solved by turning E_NOTICE's on

Sure the issue is critical. I have never said otherwise. Security is critical. My disagreement is in how to help users write more secure scripts. I do not agree that turning off register_globals will result in significantly more secure PHP scripts out there.

I think that's the source of our disagreement - I think it's going to push the base security level of PHP applications a few notches all by itself.

It will simply cause scripts to break in non-obvious ways and the knee-jerk fix will be to swear at those annoying PHP folks and then turn register_globals on, or they will do something like: foreach($HTTP_POST_VARS as $key=>$val) $$key = $val; foreach($HTTP_GET_VARS as $key=>$val) $$key = $val; foreach($HTTP_COOKIE_VARS as $key=>$val) $$key = $val; And yes, I have run across code like this.

If somebody wants to shoot himself in the head, he's quite welcome to do it. But when you hand a gun over to somebody, you don't point it at his head, but safely hand it to his hands. What he does afterwards is his own business (assuming he doesn't come after you :)

Assuming E_ALL, register_globals only fixes the case where users forget to initialize a variable and at the same time they use isset() or empty() to check to see if they have set that variable. I contend that the instances of this are actually very rare and do not warrant the massive breakage a change to the default register_globals change will cause.

See my argument above. The most common PHP apps were exploitable, by exploits which by definition would have either not been resolved by turning on E_NOTICE's, or they're E_NOTICE-unsafe. In addition, having seen quite a bit of PHP code that does use this kind of methods (which are basically the correct development methods in PHP when you have E_NOTICE's set to on, which good coders do), I think that it's crucial that we give an answer to this audience as well. Again, even if it's just 20% of the cases, or even 2% of the cases, if it leads to security issues, that's an ENORMOUS amount.

And we haven't even explored alternative approaches to perhaps help users catch this particular situation. I could see a some sort of audit mode or lint that produced a little report of external variables (anything checked/used with or without isset/empty before they are assigned), internal variables, constants and perhaps unused variables.

I don't think that's related. register_globals encourages insecure coding, and should be phased out as quickly as possible, regardless of whatever other security analysis methods we may have or add in the future.

Or as has been brought up a few times, something similar to Perl's taint mode. Tough to do in our architecture, sure, but simply changing things without exploring alternatives or researching the real state of the problem doesn't make any sense to me.

I think this problem was well researched, it came up in several different instances in the past (which gradually caused us to introduce the globals arrays, then protect them, then turn them always on and recommend using them for good coding practices). This advisory was a perfect summary of why register_globals is evil, regardless and independently of anything else. The various ideas you raised here are cool, but they do not address the simple fact that as long as register_globals stays on, your apps are likely to be insecure and exploitable. Answering this by providing analysis tools is kind of like fixing a situation with a car that has no brakes by putting very powerful airbags in it, instead of just adding brakes. Kristian - as the 'father' of this debate and the one who convinced me this is the right thing to do - do you have anything to say about this? Zeev