WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#9602 closed defect (bug) (fixed)

eval problem in classes.php and rewrite.php

Reported by: obsidiandh Owned by: 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 6 years ago.
classes.php patch
rewrite.php.patch (431 bytes) - added by obsidiandh 6 years ago.
rewrite.php patch
9602.patch (2.9 KB) - added by hakre 6 years ago.
Corrected Patch, Substition established with preg_replace_callback and a helper class.

Download all attachments as: .zip

Change History (28)

@obsidiandh6 years ago

classes.php patch

@obsidiandh6 years ago

rewrite.php patch

comment:1 @Denis-de-Bernardy6 years ago

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

comment:2 @Denis-de-Bernardy6 years ago

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

comment:3 @Denis-de-Bernardy6 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.

comment:4 follow-up: @hakre6 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.

comment:5 @hakre6 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.

comment:6 in reply to: ↑ 4 @Denis-de-Bernardy6 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.

comment:7 @DD326 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.

comment:8 @Denis-de-Bernardy6 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().

comment:9 @hakre6 years ago

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

@hakre6 years ago

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

comment:10 @hakre6 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.

comment:11 @Denis-de-Bernardy6 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. :-)

comment:12 @hakre6 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?

comment:13 @Denis-de-Bernardy6 years ago

  • Keywords tested dev-feedback added

comment:15 @ryan6 years ago

  • Milestone changed from 2.8 to 2.9

comment:16 @Denis-de-Bernardy6 years ago

dev feedback welcome before the patch becomes stale

comment:17 @westi6 years ago

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

Testing and reviewing this at the moment.

comment:18 @Hans Spath6 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.

comment:19 @westi6 years ago

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

comment:20 @westi6 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.

comment:21 @westi6 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.

comment:22 @westi6 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.

comment:23 @Derevko6 years ago

  • Cc giuseppe@… added

comment:24 @scribu5 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 :)

comment:25 @scribu5 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.