eval problem in classes.php and rewrite.php

[obsidiandh]

Hi,

One of the moderators on the support forums said I should raise a ticket for this.

wp-includes/classes.php and wp-includes/rewrite.php both use the eval function.

This function is disabled by suhosin (and sometimes in php.ini) for security reasons.

I've written a patch for the files which under my limited testing seems to have no ill effects.

I expect you can find a cleaner way to do this but here is the patch (see attached files).

Hopefully this will help you get wordpress installed on more locked down systems.

Regards

Rowan

[obsidiandh]

classes.php patch

[obsidiandh]

rewrite.php patch

[Denis-de-Bernardy]

can't get the patch to work.

[hakre]

could not handle the .patch format as well. just made a patch removing the in my opinion senseless eval calls against trunk but i must admit i am a little puzzled:

was: eval("\$query = \"" . addslashes($query) . "\";");
patch: $query = addslashes($query)

i do not understand why this eval was used there. as far as i can see it must be by mistake.

[hakre]

after clearification might be a good idea to adopt in the inline comment there.

[Denis-de-Bernardy]

Replying to hakre:

i do not understand why this eval was used there. as far as i can see it must be by mistake.

same here. it looks truly weird. the original patch didn't seem to make much sense either.

Maybe Ryan knows why it was there.

[DD32]

eval is needed in that situtation (with harke's patch). However, The original idea to use something else.. Does have the side effect that its slower..

It converts the rewrite rule from "?page_id=$matches[0]&$matches[2]=$matches[3]" to "?page_id=2&test=you", ie. by evaluating the rule directly.

[Denis-de-Bernardy]

ah, ok. things makes sense now.

needed to create an attachment with verbose rules to see it work with is_local_attachment().

[hakre]

my fault, did oversee that $query contains code then. will try to convert the original patch to get further.

[hakre]

Corrected Patch, Substition established with preg_replace_callback and a helper class.

[hakre]

i took a look into the previous patches but the code looked to complex to me. i decided to go the route with preg_replace_callback encapsulated into an own object that can carry the $matches substitution data into the callback.

[Denis-de-Bernardy]

Very neat. To be very honest, I'd suggest we keep the (faster) eval() call, even if it breaks a crummy host or two. Safe mode is going to die soon enough. :-)

[hakre]

-1 for eval.

eval is evil. really, that makes stuff hard to debug and potentially unsecure. and while writing the patch and thinking about the substitutions it came into my mind that there is a design issue within this complex that should be (in the future) taken care of. Therefore I would put the finger in there and drop the use of eval.

the regexp callback should be really fast and the instance of the class has reasonable impact. have you run a speed comparison?

[Denis-de-Bernardy]

see also: #6665

[Denis-de-Bernardy]

dev feedback welcome before the patch becomes stale

[westi]

Testing and reviewing this at the moment.

[Hans Spath]

I think eval() should be avoided at all cost.

I haven't checked if it's possible and it doesn't look like it, but if a site visitor somehow manages to modify or control parts of $query (not $matches), you have a serious problem (addslashes() won't protect you):

Demonstration:

<?php
$userstr = '&blabla={$x[die(join(null,array_map(chr(99).chr(104).'
. 'chr(114),array(72,105,32,87,111,114,100,112,114,101,115,115,44,'
. '32,101,118,97,108,32,105,115,32,101,118,105,108,46,10))))]}';

$query = '?page_id=$matches[0]&$matches[2]=$matches[3]' . $userstr;
$matches = array('a', 'b', 'c', 'd');

eval("\$query = \"" . addslashes($query) . "\";");
echo "Hello world!";
echo "\$query is '$query'\n";

Output:

Hi Wordpress, eval is evil.

[westi]

(In [11853]) Replace eval usage in request processing with new WP_MatchesMapRegex() class usage. See #9602 props hakre.

[westi]

I've been testing this for a while now and it looks ok to me.

Putting into trunk for now to see how everyone else gets on.

Leaving this open for now for feedback.

[westi]

Not had any issues / complaints with this so going to merge into the 2.8 branch as well.

[westi]

(In [11891]) Replace eval usage in request processing with new WP_MatchesMapRegex() class usage. Fixes #9602 for 2.8 branch props hakre.

[scribu]

Note the scope resolution operator, which will produce a parser error in PHP4:

​http://core.trac.wordpress.org/browser/trunk/wp-includes/classes.php?annotate=blame&rev=15861#L218

It looks like WP hasn't been compatible with PHP4 since 2.8.5 :)

[scribu]

Rats, it seems static calls are available in PHP4 too: ​http://www.php.net/manual/ro/keyword.paamayim-nekudotayim.php