WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 weeks ago

#13655 new defect (bug)

Login/Install/User Edit should stripslashes() $_POST data

Reported by: dd32 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Login and Registration Keywords: has-patch 3.2-early
Focuses: multisite Cc:

Description

Following on from #13654 All Login/Registration/Install/User Edit functionality should stripslash $_POST data.

At present, it seems that we do not stripslash at all.

For existing user passwords, we should migrate passwords to their non-stripslashed versions:

[5/31/10 6:34:11 AM] Mark Jaquith: We could migrate people.

[5/31/10 6:34:13 AM] Dion (dd32): Perhaps oughta just add proper stripslashing in 3.1, and add back-compat to change password from non-stripslashed to stripslashed.. similar to the md5->phpass implementation..

[5/31/10 6:35:13 AM] Mark Jaquith: Yep. If the PW doesn't match, addslashes() and compare again. If that matches, set the new PW hash. Right?

[5/31/10 6:35:19 AM] Dion (dd32): yep

Attachments (3)

stripslash-passwords.patch (3.7 KB) - added by johanee 4 years ago.
Fix password handling to correctly stripslash()
stripslash-passwords-take-1-fixed.patch (4.4 KB) - added by johanee 4 years ago.
Fixed patch using approach 1 (stripslash password input at once)
stripslash-passwords-take-2.patch (2.5 KB) - added by johanee 4 years ago.
Approach 2, defer stripslash as long as possible

Download all attachments as: .zip

Change History (9)

comment:1 johanee4 years ago

  • Cc johan.eenfeldt@… added
  • Keywords has-patch added; needs-patch removed

Attaching patch to fix this.

All paths for creating / editing users have been tested, including migration of un-stripslashed passwords.

Not directly regarding this issue:
Slashes handling in wp_insert_user, wp_create_user, + callers is rather convoluted (though less so for passwords compared to other fields).

It took more than one read-through of the code to convince myself that user name handling is valid (it all get fixed by a strict sanitize_user() in the end), and I'm still not entirely certain that the user_meta fields could not somehow get it wrong.

It could do with some reorganization, really.

johanee4 years ago

Fix password handling to correctly stripslash()

comment:2 johanee4 years ago

Posted from Wordcamp Stockholm

I've gone back and thought more about the best way to implement this. I also found a bug in current patch -- will update.

My first solution stripslash passwords on input. This differs from how strings are generally handled: keep them slash'ed until the very end.

It is possible to delay stripslashing until calls to wp_hash_password(). This corresponds with how other wp_insert_user() strings are handled.

I've made another version implementing this.

It does however raise a question: what password version should be sent to the authenticate filters: slashed (like its always been) or stripslashed (which might be more intuitive)?

In the new version of the patch I've left it as-is (slashed for the filters). In the original version it is stripslashed. For variety!

Please comment if any of these approaches is accepatable.

johanee4 years ago

Fixed patch using approach 1 (stripslash password input at once)

johanee4 years ago

Approach 2, defer stripslash as long as possible

comment:3 johanee4 years ago

... and a final note:

I think this handles user changes in wp-admin/network/site-users.php correctly, but I'd love someone else to look at it.

comment:4 nacin3 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Triage to Future Release

comment:5 jeremyfelt3 months ago

  • Component changed from Administration to Login and Registration
  • Focuses multisite added

comment:6 mdawaffe7 weeks ago

Note: This would also require a change to wp_xmlrpc_server::login() (or the escaping methodology in general of XML-RPC).

Note: See TracTickets for help on using tickets.