WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#10551 accepted defect (bug)

wp_die() triggers block when using ModSecurity Core Rules

Reported by: cstrosser Owned by: westi
Milestone: Future Release Priority: low
Severity: normal Version: 2.8.3
Component: Security Keywords: needs-patch dev-feedback westi-likes
Focuses: Cc:

Description (last modified by nacin)

wp_die() causes ModSecurity (using Core Rules), a commonly used Apache plugin, to throw a 406 error, blocking the message from displaying.

This seems to be triggered by the fact that a 500 error is thrown. I went and changed the default status code to 401 (Unauthorized) and it worked like a charm. I just wonder if there is a better fix for this issue... or simply a better status code to use.

Perhaps making a group of functions to make the error codes more focused.

Example:

wp_die_auth( __('You do not have sufficient permissions to access this page.') );

/**
 * Exits WordPress with Unauthorized status code.
 *
 * @see wp_die()
 */
function wp_die_auth($message, $title = '') {
	wp_die( $message, $title, 401 );
}

Attachments (1)

10551-new-functions-proposal.diff (1.9 KB) - added by solarissmoke 3 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 cstrosser5 years ago

  • Summary changed from wp_die() triggers block when using ModSecurity core rule set to wp_die() triggers block when using ModSecurity Core Rules

comment:2 westi5 years ago

  • Keywords reporter-feedback added; ModSecurity wp_die 406 error removed
  • Owner changed from ryan to westi
  • Status changed from new to accepted
  • Version set to 2.8.3

Looking at http://www.owasp.org/index.php/Category:OWASP_ModSecurity_Core_Rule_Set_Project#tab=About these rules are only Alpha quality.

Do you have more info on what the rule is trying to prevent?

comment:3 dd325 years ago

I have a feeling its recently released, see http://blog.modsecurity.org/

If i recall correctly, Its due to some security people disliking 500 errors for web app error handling, and wanting people to use the 40x error set. But theres not a 400 error code for all cases either.. IMO, the rule is pointless.

comment:4 Denis-de-Bernardy4 years ago

  • Milestone changed from Unassigned to 3.0

comment:5 nacin4 years ago

See also #11286.

I'm willing to take ownership of this and go through wp_die references and add response codes. I imagine that would mean creating some shortcut wp_die functions different requests, i.e. authentication, forbidden, not found, internal error, (401, 403, 404, 500) etc.

comment:6 miqrogroove4 years ago

Status 401 is not appropriate, unless someone has been adding a lot of features I didn't know about.

comment:8 aaroncampbell4 years ago

Please see also #12341 and #12529. Here's why I don't like 500 errors (pasted from #12529):

Part of the reason I want to do this, is that I've noticed that automated systems trying to break a site seem to see a 500 error as a clue that they are getting closer to their goal. They seem to pound a lot harder on the scripts that return 500 errors, so I'd like to clean these up and only return them when they're actually appropriate.

Currently wp_die is used 278 times in 86 files. However, we only specify an HTTP response code 3 times, which means 99% of these calls return a 500 status code.

comment:9 aaroncampbell4 years ago

  • Cc aaron@… added

comment:10 nacin4 years ago

  • Keywords early added; reporter-feedback removed
  • Milestone changed from 3.0 to 3.1

Let's do this early 3.1.

comment:11 nacin3 years ago

  • Keywords early removed
  • Milestone changed from Awaiting Triage to Future Release

comment:12 lloydbudd3 years ago

Here is another symptom reported by a customer:

"On a blackberry bold if I try to submit a comment that is empty the blackberry displays a message saying that a 500 error was generated instead of the page with the message that the comment was empty. The blackberry stops the page from loading which leaves the user confused."

comment:13 westi3 years ago

  • Keywords needs-patch 3.2-early added

comment:14 solarissmoke3 years ago

  • Keywords dev-feedback added

Here's a patch which proposes a bunch of new functions to call wp_die with different status codes. Meanwhile the default response is changed to 200.

I'm thinking that we need consensus on this first. Then it is just a case of sifting through all existing wp_die() calls and changing them accordingly.

comment:15 follow-up: nacin3 years ago

  • Description modified (diff)

Don't think we should change the default.

comment:16 in reply to: ↑ 15 solarissmoke3 years ago

Replying to nacin:

Don't think we should change the default.

Okay. How about just adding a wp_forbid function which issues a 403? - that seems the most appropriate for a number of current uses of wp_die - anything more can be added as/when needed in the future.

comment:17 westi3 years ago

  • Keywords westi-likes added; 3.2-early removed
  • Priority changed from normal to low

This is a low priority buy worth task that I would like to see us do but it is not a priority for 3.2 so removing 3.2-early

comment:18 johnbillion3 months ago

#18030 was marked as a duplicate.

Note: See TracTickets for help on using tickets.