Downgrade required PHP version to 5.2.4

Well. This certainly cleans up some f'ugly code. Including some in check_plain() which is called 50,000 times on an average page load, and fixes a security problem in stream wrappers (before 5.2.4, a stream wrapper to flickr:// would not respect allow_url_fopen security setting in php.ini). According to http://www.php.net/ChangeLog-5.php#5.2.5 this release of PHP came out in November 2007, which is basically 3 years ago. According to what DamZ and chx said on IRC, there are no current distros shipping with PHP > 5.2.0 && < PHP 5.2.5.

I think it might be worth a try slipping this into the next beta release and see what kind of end-user reports come in.

do it! </peer-pressure>

+1 here, obviously.

Parentheses are definitely the way to go there. Most people are not going to remember bitwise operator precedence.

For Fedora and Ubuntu servers, Fedora 9 or newer or Ubuntu 8.10 or newer would work after this change. So I don't see this as too much of a problem.

I'm with webchick on this. Let's try shipping this in the next beta and see what blows up. ;-)

Committed to CVS HEAD.

Um, please roll this back?

Ubuntu 8.04 (server edition) is supported through the year 2013... otherwise known as the next two-and-a-half years:
https://wiki.ubuntu.com/Releases

Even the Ubuntu 8.04 desktop version (which just so happens to be running on my computer) is supported through April 2011. So right now, I cannot install Drupal 7 without rolling back this patch :)

Also, Ubuntu backports security fixes so the check_plain() issue is not a problem there despite running 5.2.4; I haven't checked the stream wrappers thing but I assume that's the case too:
http://www.ubuntu.com/usn/usn-628-1

Maybe we can get away with requiring 5.2.4 or making a special case for Ubuntu or whatever, but requiring 5.2.5 straight up is really not acceptable.

Better example than my laptop, of course: Drupal Gardens (currently hosting something like 99% of all Drupal 7 sites) also runs on Ubuntu 8.04...

So, that is a real life example of something this patch would break. We shouldn't make Drupal not be able to run on a supported version of the most popular Linux operating system :)

Maybe we force greater than 5.2.4 and in requirements do a more elaborate check that errors if version < 5.2.5 && OS != debian. That or we check for functionality ala jQuery.support().

That should preserve the goal of the higher required version. Sounds so hacky though... If there's any other platforms that are going to get us in this problem we should definitely go with the later or roll this back.

Sadly, I think this should be fully rolled back at this point. I'd rather see us emit a runtime warning for the status page than actually block Drupal from running.

Is there a demonstrable performance improvement for the check_plain() change?

Looking at the timestamps of #13 and #14, and the text of #14, I don't think neclimdul intended to change the title and priority, so setting back to what they were in #13. I don't know what our decision needs to be yet, but whatever it is, I think it needs to be made before release, so "critical".

The fact that Ubuntu 8.04 LTS will be supported up until 2013 is completely irrelevant to the discussion.

RedHat Enterprise Linux 5 will be supported up until March 31, 2014 (end of production 3) March 31, 2017 (end of extended support). RHEL ships with PHP... 5.1.6.

All the current versions (versions N) of all the major distributions now ship with at least 5.2.5, including in their current LTS versions (including RHEL6 that should be released very soon). I see no reason not to mandate it.

There are benchmarks on the check_plain() issue here, but I'm not sure if they contain the difference between HEAD and this patch http://drupal.org/node/523058#comment-2230974

Pretty sure the difference between having the version check or not is going to be less than 10-15% of the time spent in check_plain() now. That's nice to have if we can get it for free, but not enough to raise requirements just for that change - hence the current state of check_plain() and @todo.

If this issue is still open tomorrow I'll try to run some quick benchmarks, or there's scripts in that issue if anyone wants to do it before then.

Looks like a few things broke and that we might not have to wait for a beta. Let's continue to discuss this a bit. I don't feel like there is consensus yet.

If remote streams wrappers are the problem, modules using remote stream wrappers could enforce a different PHP version, not?

And what about the security issue -- can someone describe whether that is a minor or major one? Looks like we have the same security issue in every Drupal 6 release?

It's not the problem of the Drupal project if some managers promise other managers heaven on earth.

I have one more or less personal-use server which is still running ubuntu 8.04, this because I haven't had time to update it. It has had Drupal 7 installs on it in the past for usability testing and etc., but is unlikely to get any real ones before the OS upgrade is done, however I very much doubt I'm the only person in that situation.

IMO there's a big difference between this issue, and requiring PHP 5.2+ because we wanted features plus forcing end of life quicker for previous versions.

If we can reasonably expect that no-one's running < 5.2.4, I'd go for dropping the requirement to that, and then a hook_requirements() runtime message in the status report for < 5.2.5 nudging people to upgrade (since ubuntu is the only current example, they backport security fixes anyway, and the check_plain() is only exploitable on IE6 too iirc).

If Dries wants to discuss it...

A reminder from the OD:

5.2.5 has two rather important PDO fixes Fixed PDO crash when driver returns empty LOB stream. , Fixed bug #43139 (PDO ignores ATTR_DEFAULT_FETCH_MODE in some cases with fetchAll()).

I agree with catch that check_plain() alone shouldn't force us into 5.2.5 over 5.2.4. But what about these PDO issues? How do we balance those against an old(ish), but still popular Ubuntu version?

Just to clarify: Ubuntu 8.04 is the old LTS, the current LTS being 10.04 (released in April 2010).

If you factor in that RHEL6 will be released by the end of the year, every major linux distribution (including their LTS versions) ships with at least 5.2.5. That's all that matters.

Supporting the previous LTS versions has really never been on the table. By this reasoning, we should support both RHEL5 (the distribution of choice of most "professional" hosting companies, for reasons that are definitely not technical) *and* RHEL4.

For reference:

• Ubuntu 6.04 was released June 1, 2006, it is supported until June 2011... it ships with PHP 5.1.2
• Ubuntu 8.04 was released April 24, 2008, it is supported until April 2013... it ships with PHP 5.2.4
• RHEL4 was released February 14, 2005, it is supported until February 29, 2012 (end of standard support) and February 28, 2015 (end of extended support)... it ships with PHP 4.3.9
• RHEL5 was released March 14, 2007, it is supported until March 31, 2014 (end of standard support) and March 31, 2017 (end of extended support)... it ships with PHP 5.1.6

Both "current" versions (Ubuntu 10.04 and RHEL6) ship with PHP 5.3.2.

Tagging

For what it's worth, Ubuntu should be identifiable from the PHP version alone (they all look like 5.2.4-2ubuntu5.xx, I believe).

Upgrading from Ubuntu 8.04 LTS to Ubuntu 10.04 LTS is not exactly trivial, for a number of reasons including the switch to PHP 5.3. I think if you look around out there, you will find a large number of people/hosts that still use Ubuntu 8.04 and have no plans to switch. If we're going to prevent those setups from using Drupal all of a sudden, we should have a very good reason for it, that's all. That reason is totally unclear from reading this thread and the other one.

I think PHP 5.1 (and Red Hat, etc) is really a special case, isn't it?

These people can continue to use Drupal 6 as they do today. This will probably be supported until at least 2012.

Supporting the previous LTS versions has really never been on the table. By this reasoning, we should support both RHEL5 (the distribution of choice of most "professional" hosting companies, for reasons that are definitely not technical) *and* RHEL4.

No, not really. We had http://gophp5.org/, we did not have gophp5.2.5.org.

The reasons for dropping support for PHP < 5.2 are completely different to the reasons for dropping > 5.2 and < 5.2.5.

Every MySQL, PostgreSQL, sqlite and PHP requirements change has been supported by both a good reason to do so, and a survey of real world usage to some extent. We have a decent reason to change requirements here, but we don't have real-world usage data except for a couple of people's old installs + Gardens. Should be simple enough to do a poll of the major hosts via twitter or something.

Our PHP5.3 support is still very shaky, so trying to push hosts to upgrade to distro versions which default to that, and then breaking loads of D6 installs doesn't feel good to me. If we decide to go ahead with it, then we should have a better idea of how many hosts it will affect.

5.2.5 has two rather important PDO fixes Fixed PDO crash when driver returns empty LOB stream. , Fixed bug #43139 (PDO ignores ATTR_DEFAULT_FETCH_MODE in some cases with fetchAll()).

Do we know whether D7 is actually running into either of these during normal operation? If not I'd still go for the soft requirements check for 5.2.5.

Not trying to mess with the main coders here, but which serious webhost has not updated to 5.2.5+ yet. 5.2.5 is freaking 3 years old ( according to http://www.php.net/ChangeLog-5.php#5.2.5 ). Also the current 5.2.x includes a lot of security fixes from the host's side of view.

I simply don't see the reason to why we should lower the drupal experience and performance for the mayor crowd ( like 90%+ i think ) just to maintain support for the unlucky 2% which haven't choosen a good host yet.

I could ( and most likely will ) be wrong about that, but a buffed check_plain just seems so .. juicy :)

Do we think by default stream wrappers should not be set LOCAL?

Mostly this looks good, but I think the stream wrapper part needs more thought.

Just as a note, we've never supported broken versions of php in the past. If the latest releases of php 5.2 on those OS's work, we should be fine with letting it run right? Should we actually check that there's a problem on ubuntu 8.04 before jumping?

And debian uses 5.2.6 per default, so? First ubuntu is not the #1 linux server distro. Second we should compare between the "theoretical" and "practical" used php version. Being in an LTS does not mean, users actually use the purposed version.

I'm running drupal sites on more than 12 hosts over the world and not ONE of them uses a version below 5.2.10. Most of them use debian as os and only 2 use ubuntu lts. So i see the reasoning behind your point chx, but it seems away from/not valid for the productive world.

Why not just release a beta with 5.2.5 requirement and listen to for the incoming feedback, as it is previously mentioned?

Just to be clear, I was agreeing we need to at least lower the requirement to 5.2.4. My question was whether the rest needed to be rolled back or if that was fixed in 8.04 with the patches listed #12. It sounded like Dave was saying check_plain is fine as is.

The argument has been that there are a lot but I'm betting most larger shops looking to update D7 will be more likely to be looking at 10.04 LTS as well if not new hardware. That and php 5.3 support will almost certainly be less an issue in D7 as it was/is in D6. Really its not important though, supporting 8.04 LTS is not an unreasonable request.

The size of the population we're serving aside. It seemed like a nice bonus to use the simpler check_plain that David_Rothstein said wasn't an issue on 8.04 LTS. If you don't think its an issue, I don't have the time to benchmark/test to argue against it but it seemed like if there where people interested in testing, it could be worth while.

Basically, is it worth bothering? Maybe. If someone doesn't step up soon though, lets go with #31.

Had no idea what this issue was about until reading a few of the comments.

+1 to #46, but if we're gonna do it in one patch, let's title the issue accordingly and add the necessary tags. It's really two separate issues, and if people want it separated, let's do that, but considering the combined patch is small, I think it's okay to combine.

Thanks, chx. That code looks pretty good to me, and the "experimental" approach to the htmlspecialchars() thing seems reasonable. I have not reviewed the specific nature of that check but from the command line test I assume it is correct. (This could also be a separate issue, since putting the code back in check_plain would really not be the end of the world, but I agree it is an improvement.)

Important note, though: Because Ubuntu 8.04 has 5.2.4-2ubuntu5.xx, the version_compare() actually always returns 1 for me, so the htmlspecialchars() check does not actually run - in other words, we are assuming here that anyone with a version labeled 5.2.4-[whatever] is automatically secure, which might not be the case at all. (For example, if you have Ubuntu 8.04 but have not been keeping up with security updates, we should reject you.) Therefore, it might be better to do a second version_compare() against 5.2.5 and run the htmlspecialchars() check for anyone who does not meet that (or even just run it for everyone to keep things simple).

I'd also consider a minor rewording of the error message, instead of:

Your PHP installation is too old and insecure. Drupal requires at least 5.2.5 or 5.2.4 with the htmlspecialchars security patch backported.

maybe:

Your PHP installation is too old and insecure. Drupal requires at least PHP 5.2.5, or PHP 5.2.4 with the htmlspecialchars security patch backported.

The comma and explicit mention of "PHP" make it easier to read. (I was also going to say "htmlspecialchars security patch" could link somewhere, but after looking around I'm not sure of a great place, so forget it.)

****

Overall, after reading some of the above comments, I'm going to amend my position:

Instead of saying "we should always make Drupal run on supported versions of Linux", how about we say we strive to always make it run on them unless there is a good reason not to...

And in this case, two PDO bugs that don't have a known effect on Drupal yet plus some code cleanup of check_plain() do not constitute a good reason :)

So I think we are making the right decision with this followup patch. Let's be friendly to other open source projects (and people who use them) when we can. LTS releases are an important thing for anyone who values stability, and also especially for less-technical people who just want to hit the "Update" button and know their server will work and run Drupal 7, not have to hunt around for out-of-distro PHP versions and install it themselves.

Actually, I guess this is definitely "needs work", for the second paragraph of #48. Otherwise, anyone who runs 5.2.4-something but without the security patch will still be able to install and will wind up with an insecure Drupal site.

Some feedback on the PHP version requirement:
As a system administrator that is forced to use Red Hat I'm constantly faced with specific php versions requested by both drupal and non-drupal devs all the time. The 5.1.6 version included in RHEL 5.x is too old for most projects and at least two dev shops have indicated that the 5.3.1 in RHEL 6.x will be "too new" and they've requested a 5.2.x version for the time being. In the end I will be installing PHP from 3rd party repo's for the foreseeable future anyway. I don't see a need to compromise on this just to "support" any specific Linux distro version, especially the ones with longer lifetimes.

#31 looks fine to me in terms of fixing the critical. I am not 100% comfortable RTBC-ing it because it sounds like the stream wrapper issue needs a little more discussion (it is an API change after all). But it seems to make sense as a security hardening thing.

Anyway, I do think #46 is better overall, just needs the small change I mentioned so we make sure we never let anyone use an insecure version of 5.2.4. Here it is with that.

+define('STREAM_WRAPPERS_LOCAL_HIDDEN', STREAM_WRAPPERS_LOCAL | STREAM_WRAPPERS_HIDDEN);
+
+
+/**

extra space.

Agreed the stream wrappers change needs some more discussion, the 5.2.4 check looks good to me in principle, much nicer to have that check there than in check_plain() itself.

Important note, though: Because Ubuntu 8.04 has 5.2.4-2ubuntu5.xx, the version_compare() actually always returns 1 for me

This sounds more like a bug with our version checking than something we need to special case.

That said, I was going to be really against the special case check until I started explaining my position. Then I realized its probably the right thing anyways since it is a special case. It would theoretically lets us move the required version constant if needed. Hopefully that wont happen again though...

BTW, that's a +1 to the version stuff in #52 and I agree with #53. Did we get 2 issues tangled up here?

Did we get 2 issues tangled up here?

Yes. See #30 for details. But they are related. This issue initially was about increasing the PHP requirement from 5.2.0 to 5.2.5 in order to harden D7 security. Since D6 did not use stream wrappers, and D7 does, it is D7's job to security harden their use, and this is only possible as of 5.2.4, since that's where stream_wrapper_register() supports a $flags parameter for identifying a stream wrapper as remote so that it respect the allow_url_include php.ini setting. This was committed in #11, but requires two follow-ups:

1) Now that we've hardened remote stream wrappers, chx wants to harden even further by making that the default, requiring developers who create stream wrappers to explicitly opt-in to being LOCAL, and therefore, more trusted. As per #34 and other comments, people want more time to think this through.

2) Lower the requirement from 5.2.5 to 5.2.4, since that's all that's really needed for stream wrapper hardening.

We could do these as separate follow-ups, but perhaps it makes sense to leave them combined for another day or so, and only separate them if #1 gets bogged down, but we want to move quickly on #2.

Rerolled to remove the extra space (per #53).

This issue isn't getting adequately reviewed, so splitting off the stream wrapper portion in #942690: Security harden stream wrappers by defaulting them as remote. This patch just contains the code to lower the required version. Relative to #59, I added a sentence to the code comment.

Note that Drupal on PostgreSQL requires 5.2.11 *already*. This is enforced in PostgreSQL's install.inc.

Just for clarification, #62 is a reply to #30, where chx was concerned that one of the two PDO bugs existing in 5.2.4 but fixed in 5.2.5, would bite people on PostgreSQL. #62 means we don't have to worry about that being a problem if we commit #60. The other PDO bug mentioned in the OD is documented in #60. So I think we're good to go here.

Commited #436582 by webchick
Abort abort\! Back to 5.2.4.

Now that 5.2.4 is the minimum, would it not also make sense to remove the function_exists('sys_get_temp_dir') check from file_directory_temp() ?

No. We tried that before and it caused problems on Mac OSX. See http://drupal.org/node/551658#comment-2919160 and below.