WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10360 closed defect (bug) (fixed)

$_REQUEST's slashes may differ from $_GET/$_POST

Reported by: dd32 Owned by: markjaquith
Milestone: 2.8.3 Priority: normal
Severity: normal Version: 2.8
Component: Security Keywords: has-patch commit
Focuses: Cc:

Description

$_REQUEST is re-created from $_GET/$_POST on line 51 of wp-settings.php

However, $_GET/$_POST/$_COOKIE are slashed ~ line 580, The end result is that $_POST['test'] may not equal $_REQUEST['test'] depending on the server setup, and content of such globals.

My suggestion would be to move both code blocks from where they currently are, and place them in about line 260. (Or alternativly, Just add $_REQUEST to the deslash/addslash code block..)

Attachments (2)

10360.diff (894 bytes) - added by dd32 5 years ago.
10360.2.diff (994 bytes) - added by Denis-de-Bernardy 5 years ago.
alternative approach: slash $_REQUEST

Download all attachments as: .zip

Change History (39)

comment:1 Denis-de-Bernardy5 years ago

  • Component changed from Administration to Security
  • Milestone changed from 2.9 to 2.8.1
  • Owner set to ryan

this seems rather blocking imo

comment:2 Denis-de-Bernardy5 years ago

Curious to know why $_REQUEST isn't slashed, alongside $_GET, $_POST and $_COOKIE...

comment:3 dd325 years ago

Probably because when that code was written, It was taboo to touch $_REQUEST (Or it was simply forgotten..)

That being said, I hate that plugins have come to expect slashed content..

comment:4 Denis-de-Bernardy5 years ago

yea. the real question will be what will happen if/when we drop the behavior, though... Imagine all of the security holes that we'll be introducing.

comment:5 dd325 years ago

most of the WP Private API has moved/started to move to expecting non-slashed data, Well, aside from those which retain the back-compat since they're called directly.

It is a shame that it'd introduce holes, but it'll happen one day..

comment:6 hakre5 years ago

Dropping "Request Abstraction" again. API Style. Should save us a lot of headaches w/o doing much bloat.

+1 for "Legacy code can stay legacy and fails sometime"

comment:7 dd325 years ago

  • Summary changed from $_REQUEST's slashes may differ from $_GEt/$_POSt to $_REQUEST's slashes may differ from $_GET/$_POST

Except its hardly Legacy, And is used more often in WP every version. Generally only or comparison of action args. Its rarely used for data which may include slashes, infact, pretty much unheard of.

there was a recent changeset which forces $_REQUEST to be $_GET + $_POST, so it removes most of the usual PHP-arguements against using $_REQUEST.

comment:8 hakre5 years ago

The argument against using $_REQUEST is taht, well using it disables you to determine of which the source comes from. So merging $_GET + $_POST into $_REQUEST still keep this argument.

comment:9 vladimir_kolesnikov5 years ago

  • Cc vladimir@… added

In brief:

If magic_quotes_gpc is on: $_GET, $_POST, $_COOKIE, $_SERVER and $_REQUEST will be slashed;
If magic_quotes_gpc is off: $_GET, $_POST, $_COOKIE and $_SERVER will be slashed, $_REQUEST won't.

And, $_REQUEST = array_merge($_GET, $_POST) does not take into account php.ini's variables_order variable and (for PHP 5.3.0), request_order variable.

BTW, throwing $_COOKIE out of $_REQUEST breaks phpBB.

comment:10 Denis-de-Bernardy5 years ago

@vladimir: There was some discussion related to what should end up in $_REQUEST in the related ticket.

FWIW, the whole point of the ticket was to remove $_COOKIES from $_REQUEST for security reasons, without introducing a wp_gpc() function that would end up adding needless overhead. If phpBB uses $_REQUEST where $_COOKIES should be used, they took a very questionable option.

It was decided to ignore the gpc order and to stick to $_GET and $_POST, because there's no straightforward way to know whether $_SERVER and $_ENV are included and in which order. (There are many php ini settings that serve the same purpose.)

comment:11 vladimir_kolesnikov5 years ago

Thanks for the explanation.

Unless I am missing something, according to http://us.php.net/manual/en/reserved.variables.request.php $_REQUEST is always $_GET + $_POST + $_COOKIE and the order depends upon variables_order setting. $_SERVER and $_ENV are not merged into $_REQUEST.

comment:12 follow-up: Denis-de-Bernardy5 years ago

yeah, but see also the changelog on the same page (it used to have $_FILES), and:

  • the discussions on #8814

comment:13 in reply to: ↑ 12 vladimir_kolesnikov5 years ago

Replying to Denis-de-Bernardy:

This only affects $_SERVER - that is, $_SERVER will always have $_ENV array merged in.

See http://blog.sjinks.org.ua/p.php (screenshot: https://url.odesk.com/bw6ig) - the server has variables_order set to EGPCS but $_REQUEST still contains only $_GET + $_POST + $_COOKIE.

variables_order affects which superglobals are created, the order how G/P/C are put into _REQUEST and the order in which E/G/P/C/S get imported into the global namespace if register_global is on.

http://us.php.net/manual/en/ini.core.php#ini.request-order

"This directive describes the order in which PHP registers GET, POST and Cookie variables into the _REQUEST array" - the manual does not mention any other superglobals.

Also http://us.php.net/manual/en/reserved.variables.request.php: "When running on the command line, this will not include the argv and argc entries; these are present in the $_SERVER array." - this explicitly states that $_SERVER is not a part of $_REQUEST.

Finally, if you take a look at main/php_variables.c in the PHP source (you need the function named php_auto_globals_create_request), you'll see it never uses anything else but G/P/C (screenshot: https://url.odesk.com/1eo0y).

comment:14 follow-up: Denis-de-Bernardy5 years ago

But that applies to recent php versions, no? I couldn't care less if SERVER and ENV get in there, personally, as long as cookies aren't included. ;-)

comment:15 in reply to: ↑ 14 vladimir_kolesnikov5 years ago

Replying to Denis-de-Bernardy:

But that applies to recent php versions, no?

4.3.0 behaves the same way but the code is located in main/main.c

comment:16 Denis-de-Bernardy5 years ago

oh, ok. so then, merging $_GET and $_POST seems like the perfectly valid shortcut in retrospect. Remains to slash it properly.

comment:17 hakre5 years ago

Using $_REQUEST should just considered bad practice. Whatever is merged in there. Infact you can't properly determine without checking the php configuration and with wordpress even the core code. For any developer I would advise to not use it. Anyway that is just a sidenote.

Since parts of the wordpress code (still) depend on $_REQUEST it should contain at least the same values like $_POST and $_GET (like Denis points out). If $_POST and $_GET is slashed in another way then $_REQUEST is (as this ticket suggests), there is need to have it the same way.

Merging should be placed after salshing $_POST and $_GET.

Additionally an object could be instatiated that abstracts and contains the request data as submitted. This object can then be used to give access to untainted values as well to give access to post, get, files, cookies, (a configured) request and whatever is needed or usefull.

So is there any piece of documentation available wether or not $_REQUEST should contained untainted or slashed data? Even though it does differ from $_POST / $_GET it might be the intended behaviour. For the case it should be the same I can provide a patch.

dd325 years ago

comment:18 dd325 years ago

  • Keywords has-patch added; needs-patch dev-feedback removed
  • Version changed from 2.9 to 2.8

attachment 10360.diff added

  • Moves $_REQUEST merge of GET/POST to after slashing has been taken care of.

Denis-de-Bernardy5 years ago

alternative approach: slash $_REQUEST

comment:19 Denis-de-Bernardy5 years ago

added a patch that slashes $_REQUEST instead, in case we want to keep $_REQUEST with the same data all along.

comment:20 hakre5 years ago

  • Keywords dev-feedback added; has-patch removed

Just reviewed the code. According to inline documentation

// Force REQUEST to be GET + POST.  If SERVER, COOKIE, or ENV are needed, use those superglobals directly.

, $_REQUEST is to be expected forced GET + POST. Both are mostly untainted that time so therefore I strongly speak to wontfix or invalid. There is no need to expect $_REQUEST to be slashed.

I do not see any motivation to write a patch then.

comment:21 hakre5 years ago

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

comment:22 follow-up: dd325 years ago

added a patch that slashes $_REQUEST instead

I didnt see the point in slashing the data twice, and before that block is hit, you cant guarantee what condition the data is in those variables. AFAIK nothing(mission critical) uses any of the request-related globals before then anyway

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

Not sure i can follow that english..

comment:23 follow-ups: Denis-de-Bernardy5 years ago

hehe, as I wrote, it's "in case we want to keep $_REQUEST with the same data all along" (which I don't think we do either).

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

comment:24 dd325 years ago

Sorry missed your first comment

, $_REQUEST is to be expected forced GET + POST. Both are mostly untainted that time so therefore I strongly speak to wontfix or invalid. There is no need to expect $_REQUEST to be slashed.

Except like what i originally said: (now paraphrased)

WordPress will force $_GET/$_POST to be slashed REGARDLESS OF SERVER SETUP, $_REQUEST doesnt gain the same treatment.

the comment in the code is just to force $_REQUEST to $_GET/$_POST (ie. To exclude $_SERVER and $_COOKIE)

The end result on a Server where Magic quotes are disabled is as such:

POST Data = "O'Niel";
$_POST = "O\'Niel";
$_REQUEST = "O'Niel";

comment:25 in reply to: ↑ 23 vladimir_kolesnikov5 years ago

Replying to Denis-de-Bernardy:

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

On the other hand, this might break the plugins that relied upon the old behavior (for example, if I know that $_REQUEST is unslashed, I can pass its variables (without applying stripslashes()) to $wpdb->insert()/$wpdb->update()).

I agree that relying upon $_REQUEST is a bad practice that should be avoided unless you know what you do; what I dislike is that hacking with $_REQUEST breaks 3rd party software.

comment:26 in reply to: ↑ 22 hakre5 years ago

Replying to dd32:

Just to remind: If it's the point to not change the way to slash request variables, then this is true for $_REQUEST as well. This Ticket is invalid then.

Not sure i can follow that english..

In diverse discussion it has been stated many, many times that there will be no change in the way WordPress slashes the data. This is because of the status quo. So this ticket's report must be bogus as well. Because changing the well accepted behavior of $_REQUEST will change the WordPress behavior of how request variables are slashed.

As it has been clearly said that the way WordPress slahes request related data has not to be touched this ticket is plain invalid.

I hope I can make the point more clear now.

comment:27 in reply to: ↑ 23 hakre5 years ago

Replying to Denis-de-Bernardy:

@hakre: ticket is not invalid, and not slashing $_REQUEST can introduce potential security issues if a plugin uses it expecting it's always slashed.

The way WordPress handles the request data per se introduces a lot of potential security isseus. So this is ticket is not the right place to pick that topic. Especially having $_REQUEST untainted is a kind of better practice as the pseudo-security with $_POST and $_GET.

comment:28 follow-up: dd325 years ago

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

We all agree that relying upon slashed data in superglobals is bad. Theres no question about it.

This is about CONSISTENCY.

$_POST['something'] should be able to be replaced by $_REQUEST['something'] and act EXACTLY THE SAME. This is not currently happening due to WordPress's Slashing of data in $_GET/$_POST but NOT in $_REQUEST (Which may be slashed if the server has it enabled, or not slashed otherwise..)

The slashing of data is NOT for this ticket, and another ticket has recently been closed around it.

comment:29 hakre5 years ago

+1 for removing slashes from _POST and _GET sothat - as dd32 makes bold - _POST and _GET can be replaced anytime with _REQUEST. Plus the point that "we all agree that relying upon slashed data in superglobals is bad." (is the wordpress maintainer part of that "we" or not?)

The currrent patch does not reflect that, it just merges (slashes) _POST & _GET into _REQUEST and not the other way round (dd32 wrote about replacing _POST resp. _GET with _REQUEST and not the other way round).

So I see no has-patch nor commit readyness.

How can we get a valid statement from the maintainer on this issue?

comment:30 in reply to: ↑ 28 hakre5 years ago

Replying to dd32:

The slashing of data is NOT for this ticket, and another ticket has recently been closed around it.

Please reference that ticket. Thanks.

comment:31 dd325 years ago

+1 for removing slashes from _POST and _GET sothat - as dd32 makes bold

My comment was never intended to be taken that way.

I intended that the use of $_POST inside wordpress should be able to be replaced with $_REQUEST at any stage and operate the same. This infers that the slashes in REQUEST should be the same as POST. I am not referring to some non-WP application. Simple fact is, WordPress uses/expects POST/GET data to be slashed, Even though internally data is used unslashed in recent versions. $_REQUEST is NOT slashed, and therefor, cannot be used interchangably

Please reference that ticket. Thanks.

Search for it. I'm not your personal search engine.

comment:32 hakre5 years ago

("'Quote' on Quote") Simple fact is, WordPress uses/expects the $_REQUEST data not to be slashed. If you say A for _POST and _GET, then you must say B for_REQUEST. If you argue to keep up with the status quo, then this is an argument against changing current $_REQUEST data.

If you decide to change the current slashing of the superglobals containing request data, then I suggest to stop slashing them to improve the dataflow instead of repeating wrong decisions of the past.

Additionally I have only asked that you link to the other ticket you used as argument. I know that there are a lot of tickets that are about slashing or not slashing data so I can not exactly follow your point until you link that certain ticket you meant. That's all. I know how to deal with the trac search, so please do not feel offended by me asking. I thought you have it in your browsers history or similar.

comment:33 dd325 years ago

see #10452

If you decide to change the current slashing of the superglobals containing request data, then I suggest to stop slashing them to improve the dataflow instead of repeating wrong decisions of the past.

I would agree, Except that since it was decided in #10452 to not removing slashing (Whch is where the 'remove slashing' discussion belongs) then the request-related vars should follow a similar curve to each other.

I'm going to bring this up in this weeks Developer discussion on IRC.

comment:34 hakre5 years ago

+1 that really is a good idea. I would love to see a maintainers statement in/after that discussion. I know that this is not easy to decide but I think it should be given a clear statement that leads into the right direction for the future.

Next to what I (personally) would prefer it was for me in WP devrlopment to stick with the current status quo (wether or not it's a personal preference). I'm personally very open minded to have a more well defined data flow (that means to not slash the data as this is consent in the PHP community _mostly_ but not overall (WordPress is one exception, another one might be phplist [not that shure] but then my knowledge ends).

comment:35 markjaquith5 years ago

  • Owner changed from ryan to markjaquith
  • Status changed from new to accepted

I regret that "always slashed" was chosen for $_GET and $_POST, but that ship has sailed. If we want to change that, it's going to be a long journey, because there is a very real possibility that such a change will introduce security issues in plugins. But that is a separate topic.

I agree with dd32 that this is about consistency. Honestly, it was news to me that $_REQUEST is not slashed (not that I'm in the habit of trusting user data!) I regard $_REQUEST as $_GET + $_POST, and both $_GET and $_POST are always slashed—so why wouldn't $_REQUEST always be slashed? There is the possibility that changing this will break a few plugins for some setups, but those plugins were already broken because on other server setups $_REQUEST will be slashed.

Simply put, as it is now, $_REQUEST is unpredictable, and any use of it that would be affected by "slashable" data is currently unstable. dd32's patch fixes this, by making it consistent (with itself) and consistend with $_GET and $_POST, as well as fixes it in a way that won't introduce SQL injection vulnerabilities.

comment:36 markjaquith5 years ago

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

(In [11760]) Be consistent about slashing _REQUEST superglobal. props dd32. fixes #10360

comment:37 hakre5 years ago

Then so shall it be.

Note: See TracTickets for help on using tickets.