WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 8 weeks ago

#24030 assigned defect (bug)

Add an action for when nonce verification fails

Reported by: dd32 Owned by: Shelob9
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.1
Component: Security Keywords: good-first-bug has-patch dev-feedback needs-testing
Focuses: Cc:

Description

Currently when a plugin (or core) calls check_admin_referer() there is no way for auditing (or debugging) plugins to hook in and record an event that the nonce check failed.

Previously it was possible to use the explain_nonce_$nonce filter to do this, but that was removed in [21133].

If a plugin wants to record an event for a failing nonce, it'll need to call wp_verify_nonce() manually itself, and die afterwards, or call check_admin_referer() after verifying the nonce itself for logging purposes.

I'd suggest either resurrecting the previous filter as an action (for back compat) or adding a new nonce failure hook.

Attachments (3)

24030.diff (853 bytes) - added by Shelob9 5 months ago.
24030-1.patch (1.0 KB) - added by garza 8 weeks ago.
wp_verify_nonce_test.php (1.4 KB) - added by garza 8 weeks ago.
unit test for patch 24030-1

Download all attachments as: .zip

Change History (11)

comment:1 dd328 months ago

#21190 was marked as a duplicate.

comment:2 nacin6 months ago

  • Component changed from General to Security
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

comment:3 Shelob95 months ago

  • Keywords has-patch added; needs-patch removed

I'm presuming that the very complex filter that was removed in 21133 was removed for its complexity, so I have created a very simple action. I used it successfully to log actions that failed because the nonce wasn't correct. My specific test was copying the link for switching the theme to Twenty Eleven, and removing the last character from the nonce string before submitting it. I was able to log 'switch-theme_twentyeleven' using the action added in my patch 24030.diff

If something more complicated is needed, please let me know and I will take a stab at it.

Last edited 5 months ago by Shelob9 (previous) (diff)

Shelob95 months ago

comment:4 SergeyBiryukov5 months ago

  • Milestone changed from Future Release to 3.9

comment:5 DrewAPicture5 months ago

  • Owner set to Shelob9
  • Status changed from new to assigned

comment:6 johnbillion5 months ago

We should add the equivalent to check_ajax_referer() too.

comment:7 nacin4 months ago

  • Milestone changed from 3.9 to Future Release

There are existing hooks in check_(admin|ajax)_referer, so we should be able to just append _failed to those to get the other hook name.

Or, should this be done directly in wp_verify_nonce()?

garza8 weeks ago

garza8 weeks ago

unit test for patch 24030-1

comment:8 garza8 weeks ago

  • Keywords dev-feedback needs-testing added

Uploaded a new patch that moves the action created by Shelob9 into wp_verify_nonce() per the last comment by nacin.

Also created a unit test case which makes sure this action is getting called and passing the $uid and $action

Note: See TracTickets for help on using tickets.