Make WordPress Core

Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#9602 closed defect (bug) (fixed)

eval problem in classes.php and rewrite.php

Reported by: obsidiandh's profile obsidiandh Owned by: westi's profile westi
Milestone: 2.8.5 Priority: low
Severity: minor Version: 2.7.1
Component: Permalinks Keywords: has-patch tested
Focuses: Cc:

Description

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

Attachments (3)

classes.php.patch (434 bytes) - added by obsidiandh 15 years ago.
classes.php patch
rewrite.php.patch (431 bytes) - added by obsidiandh 15 years ago.
rewrite.php patch
9602.patch (2.9 KB) - added by hakre 15 years ago.
Corrected Patch, Substition established with preg_replace_callback and a helper class.

Download all attachments as: .zip

Change History (28)

@obsidiandh
15 years ago

classes.php patch

@obsidiandh
15 years ago

rewrite.php patch

#1 @Denis-de-Bernardy
15 years ago

  • Component changed from General to Security
  • Owner changed from anonymous to ryan

#2 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch added
  • Milestone changed from Unassigned to 2.8

#3 @Denis-de-Bernardy
15 years ago

  • Component changed from Security to Permalinks
  • Keywords needs-patch added; has-patch removed
  • Priority changed from normal to low
  • Severity changed from normal to minor

can't get the patch to work.

#4 follow-up: @hakre
15 years ago

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.

#5 @hakre
15 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

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

#6 in reply to: ↑ 4 @Denis-de-Bernardy
15 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

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.

#7 @DD32
15 years ago

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.

#8 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

ah, ok. things makes sense now.

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

#9 @hakre
15 years ago

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

@hakre
15 years ago

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

#10 @hakre
15 years ago

  • Keywords has-patch added; needs-patch removed

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.

#11 @Denis-de-Bernardy
15 years ago

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

#12 @hakre
15 years ago

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

#13 @Denis-de-Bernardy
15 years ago

  • Keywords tested dev-feedback added

#15 @ryan
15 years ago

  • Milestone changed from 2.8 to 2.9

#16 @Denis-de-Bernardy
15 years ago

dev feedback welcome before the patch becomes stale

#17 @westi
15 years ago

  • Owner changed from ryan to westi
  • Status changed from new to reviewing

Testing and reviewing this at the moment.

#18 @Hans Spath
15 years ago

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.

#19 @westi
15 years ago

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

#20 @westi
15 years ago

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.

#21 @westi
15 years ago

  • Keywords dev-feedback removed
  • Milestone changed from 2.9 to 2.8.5

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

#22 @westi
15 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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

#23 @Derevko
15 years ago

  • Cc giuseppe@… added

#24 @scribu
13 years ago

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

#25 @scribu
13 years ago

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

Note: See TracTickets for help on using tickets.