WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 5 years ago

Last modified 4 years ago

#10551 closed defect (bug) (fixed)

wp_die() triggers block when using ModSecurity Core Rules

Reported by: cstrosser Owned by: westi
Milestone: 4.1 Priority: low
Severity: normal Version: 2.7
Component: Security Keywords: westi-likes has-patch needs-docs
Focuses: Cc:
PR Number:

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 (4)

10551-new-functions-proposal.diff (1.9 KB) - added by solarissmoke 9 years ago.
18030.diff (42 bytes) - added by joehoyle 5 years ago.
18030.2.diff (12.9 KB) - added by joehoyle 5 years ago.
10551.diff (14.9 KB) - added by nacin 5 years ago.

Download all attachments as: .zip

Change History (36)

#1 @cstrosser
10 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

#2 @westi
10 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?

#3 @dd32
10 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.

#4 @Denis-de-Bernardy
10 years ago

  • Milestone changed from Unassigned to 3.0

#5 @nacin
10 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.

#6 @miqrogroove
10 years ago

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

#8 @aaroncampbell
10 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.

#9 @aaroncampbell
10 years ago

  • Cc aaron@… added

#10 @nacin
10 years ago

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

Let's do this early 3.1.

#11 @nacin
9 years ago

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

#12 @lloydbudd
9 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."

#13 @westi
9 years ago

  • Keywords needs-patch 3.2-early added

#14 @solarissmoke
9 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.

#15 follow-up: @nacin
9 years ago

  • Description modified (diff)

Don't think we should change the default.

#16 in reply to: ↑ 15 @solarissmoke
9 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.

#17 @westi
9 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

#18 @johnbillion
6 years ago

#18030 was marked as a duplicate.

#19 @joehoyle
5 years ago

I'm happy to take ownership of this, this is pretty important for me, as throwing 500s when it isn't really screws up with my monitoring!

For the current patch, I agree with nacin that the default should not be changed. I also don't really care for the new convenience functions. For real convenience, nacin and I discussed the 3 param to wp_die() can handle an integer which is a shortcut for passing array( 'response' => 500 );

Attached patch adds the integer shortcut to wp_die and replaces all instances of "Cheatin Uh?" to pass a 403 response. I'd rather not try tackle 100% of the wp_die calls in one patch here, rather go incremental.

@joehoyle
5 years ago

@joehoyle
5 years ago

#20 @joehoyle
5 years ago

Ping nacin, I know we discussed this in wceu - how do you feel about this patch? I know there was talk about the second param of '' being a pain, however - I was thinking over this, I think introducing a similarly named function like wp_die_forbidden or something just increases the amount of inconsistency with having different params.

@nacin
5 years ago

#21 @nacin
5 years ago

This function already has a ton of "magic", I'll give you that. Example: for wp_die( $wp_error_object ), that WP_Error object could also have a title in its error data, and it'd magically work.

But we're already giving this function a new prototype. Existing:

wp_die( string|WP_Error $message = '', $title = '', array $args = array() )

New:

wp_die( string|WP_Error $message = '', $title = '', int $response_code = 500 )

I've been convinced to be conservative on method overloading because it really damages IDE usage (example: add_query_arg()) but as long as "standard" arguments are listed (in add_query_arg()'s case, they are not, and func_get_arg() is used), then I'm more for it. Why not an additional prototype of:

wp_die( string|WP_Error $message = '', int $response_code = 500 )

If we had a default message (which we don't), I'd even be OK with even allowing this, too:

wp_die( int $response_code = 500 )

I dunno. I can be convinced either way. Check out 10551.diff as a sample.

#22 @joehoyle
5 years ago

I see what you mean. I think given that the second argument is so seldom used, I can get on board with having the second arg be used for the code.

IMO this is too far: wp_die( int $response_code = 500 ) I'd be worried about it giving unexpected behaviour, e.g.:

wp_die( $request->error_message ); // could unknowingly be an int, and do something unexpected.

+1 for nacin's patch.

#23 follow-up: @johnbillion
5 years ago

  • Keywords has-patch needs-docs added; needs-patch dev-feedback removed
  • Milestone changed from Future Release to 4.1
  • Version changed from 2.8.3 to 2.7

I think this is fine because it changes the signature of wp_die() but doesn't change it for the various die handler callbacks.

I agree with Joe, wp_die( int $response_code = 500 ) would mean introducing a default error message and we don't want to encourage people to avoid providing their own.

While we're at it, the inline docs for _default_wp_die_handler() need updating because $message can be a WP_Error.

#24 @wonderboymusic
5 years ago

to be paired with #11286

#25 @johnbillion
5 years ago

In 30355:

Allow the response code to be passed as a shorthand to the $title or $args parameter of wp_die(), for brevity.

See #10551 and #11286
Props nacin

#26 @johnbillion
5 years ago

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

In 30356:

Switch to a 403 response code in places where it is more appropriate than a 500 due to permissions errors.

Fixes #10551
Props nacin

#27 @joehoyle
5 years ago

Wohoo!

#28 in reply to: ↑ 23 @nacin
5 years ago

This was mostly joehoyle's leg work and initial patch, props (and thanks) joe!

Replying to joehoyle:

could unknowingly be an int, and do something unexpected

FWIW, I agree this could be awkward, but this is also precisely how die/exit already work, in that they are exit codes when they are ints.

Replying to johnbillion:

would mean introducing a default error message and we don't want to encourage people to avoid providing their own.

OK, that argument definitely resonates with me. Carry on!

#29 @DrewAPicture
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If $title also now accepts an integer as described in [30355], that string|int parameter type should be reflected.

#30 @DrewAPicture
5 years ago

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

In 30379:

Ensure the mixed type of string|int is reflected on the $title parameter in wp_die().

The ability to pass an error code as short-hand to the $title and $args parameters was added in r30355. Changes also include cleaned-up formatting and line-wraps for other documentation in the DocBlock.

See [30355]. Fixes #10551.

#31 @johnbillion
5 years ago

In 30507:

Update the inline docs for wp_die() to reflect parameter changes made in r30355

See #10551

#32 @rachelbaker
4 years ago

#29829 was marked as a duplicate.

Note: See TracTickets for help on using tickets.