Ticket #5644 (closed defect: fixed)

Opened 9 months ago

Last modified 7 months ago

wp_kses_normalize_entities regular expression does not use callback

Reported by: darkdragon Assigned to: ryan
Priority: normal Milestone: 2.5
Component: Security Version:
Severity: normal Keywords: kses
Cc:

Description

In wp_kses_normalize_entities(), the second preg_replace uses the 'e' or eval instead of using the preg_replace_callback() function that has been in PHP since 4.0.5.

Recommendation:

Change 

$string = preg_replace('/&#0*([0-9]{1,5});/e', 'wp_kses_normalize_entities2("\\1")', $string);

To: 

$string = preg_replace_callback('/&#0*([0-9]{1,5});/', 'wp_kses_normalize_entities2', $string);

Change History

01/11/08 05:53:37 changed by darkdragon

  • keywords set to kses.

01/11/08 05:57:41 changed by darkdragon

wp_kses_bad_protocol_once() is also the same.

(in reply to: ↑ description ) 01/11/08 16:21:14 changed by lloydbudd

Replying to darkdragon:

In wp_kses_normalize_entities(), the second preg_replace uses the 'e' or eval instead of using the preg_replace_callback() function that has been in PHP since 4.0.5.

What is the benefit?

01/11/08 18:55:18 changed by westi

  • owner changed from anonymous to westi.
  • status changed from new to assigned.

The main issue with e is that you are giving user supplied data to php to evaluate - therefore theoretically you could have a security issue if you are not careful.

This is why using a callback is better.

01/12/08 03:15:04 changed by darkdragon

I was wrong about wp_kses_bad_protocol_once(), since from what I've read on php.net on preg_replace_callback() it does not allow for adding parameters and the replacement in that needs to have a parameter passed to the callback function. Which is not possible.

I pointed it out since using 'e' replacement parameter has bitten phpBB quite a few times and is generally seen as being a security risk. I'm unsure if that stands here, since I'm not a security expert.

Preventing something could go a long way however, since the fix is relatively trivial and should not break anything.

I'm unsure how much support you have for the Kses library.

02/27/08 00:48:57 changed by ryan

Switching to preg_replace_callback() is good. I just did this in another place for [7056]. Anyone want to patch this up?

02/29/08 17:49:50 changed by ryan

(In [7106]) Use preg_replace_callback instead of 'e' modifier. see #5644

02/29/08 17:50:05 changed by ryan

  • owner changed from westi to ryan.
  • status changed from assigned to new.

02/29/08 18:28:32 changed by ryan

(In [7107]) Use preg_replace_callback instead of 'e' modifier. see #5644

02/29/08 18:30:31 changed by ryan

  • status changed from new to closed.
  • resolution set to fixed.

Fixed wp_kses_bad_protocol_once() with a global hack on allowed_protocols. I had it working by var_export()ing allowed_protocols into the callback string, but var_export can be messed up if it is run within a containing ob_start.

03/01/08 04:02:29 changed by darkdragon

  • milestone changed from 2.6 to 2.5.

That is cool. Thanks.

I totally forgot about this ticket.