Opened 10 months ago
Last modified 7 days ago
#9065 assigned enhancement
PHP 8 Fatal error: Malformed inputs can cause fatals.
Reported by: | dd32 | Owned by: | imath |
---|---|---|---|
Milestone: | Under Consideration | Priority: | high |
Severity: | normal | Version: | |
Component: | Core | Keywords: | has-patch early needs-refresh |
Cc: |
Description
A number of buddypress pages/routes/views use stripslashes()
on the input, when coupled with malformed input this causes warnings such as this in PHP 7.4, and fatals in PHP 8.1:
PHP 7.4 E_WARNING: stripslashes() expects parameter 1 to be string, array given in plugins/buddypress/bp-blogs/bp-blogs-template.php:178
PHP 8.1 Fatal error: Uncaught Error: stripslashes(): Argument #1 ($string) must be of type string, array given in plugins/buddypress/bp-blogs/bp-blogs-template.php on line 178
Replacing the calls to stripslashes( $_REQUEST[...] )
with wp_unslash( ... )
will partially resolve this, as it'll cause arrays (the most obvious incorrect input usually) to be handled correctly, and the invalid data passed to the underlying classes to usually be handled incorrectly.
Eg:
GET https://example.org/sites/?Search&sites_search[foo]=bar
If we use the example warning/fatal above, and replace it with wp_unslash()
we then end up with a warning/fatal of this instead:
Fatal error: Uncaught Error: addcslashes(): Argument #1 ($string) must be of type string, array given in wp-includes/class-wpdb.php on line 1785
This is ultimately because bp_esc_like( $array_data )
is then called which is incorrect use of the function. The sanitisation of the input is still not handled right, but is a step forward towards the correct handling of the data.
Change History (21)
This ticket was mentioned in PR #214 on buddypress/buddypress by @dd32.
10 months ago
#1
- Keywords has-patch added
10 months ago
#2
As one can see... this is fairly ugly checking for the data types everywhere.
This has redirected me back to https://core.trac.wordpress.org/ticket/22325 / https://core.trac.wordpress.org/ticket/18322 and experimenting with it: https://github.com/WordPress/wordpress-develop/compare/trunk...dd32:wordpress-develop:try/request-abstraction
It might be worthwhile taking that and making a bp_get()
/ bp_get_request()
method that does this sanitisation that can later be deprecated for whatever ends up in Core.
10 months ago
#3
Thanks a lot for your hard work on this @dd32 I'll review the PR asap and will try to include the fix in next minor release (12.1.0).
10 months ago
#5
@imath No urgency here IMHO, this isn't worth rushing, better to get it right and clean instead.
I only did the input sanitization
on a few files in the PR, I'll add a note to the issue description for that. There's a bunch more work needed here IMHO.
10 months ago
#6
Thanks for your message @dd32. I had a quick look and saw it was pretty heavy, is it ok if we fix it in next major instead of next minor (I'm always worrying about profiles.w.org)?
10 months ago
#7
is it ok if we fix it in next major
Totally, this isn't a 'light' PR, nor is it urgent.
The only reason for this PR was those warnings from pentesterrs, which I realised are fatals in PHP8, this kind of thing is a "problem" for a lot of plugins, BuddyPress isn't doing something horribly wrong that needs urgent-today-attention.
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
9 months ago
6 months ago
#11
@imath Is there anything I can do to help this along?
For example, would smaller PRs with targeted fixes help? Such as https://github.com/buddypress/buddypress/compare/master...dd32:buddypress:fix/search-warnings
renatonascalves commented on PR #214:
6 months ago
#12
As part of https://buddypress.trac.wordpress.org/ticket/7228, I was looking to address some of those issues. It seems you already got started to solve some of them here.
[ ] WordPress.Security.NonceVerification.Recommended 519 [ ] WordPress.Security.ValidatedSanitizedInput.InputNotSanitized 474 [ ] WordPress.Security.ValidatedSanitizedInput.MissingUnslash 462 [ ] WordPress.Security.NonceVerification.Missing 248 [ ] WordPress.Security.ValidatedSanitizedInput.InputNotValidated 128
We have quite a lot to fix here. And we need to find a way to pinpoint a commit in case a problem happens.
I'd suggest we introduce those changes per component/folder. Small pull requests where the fixes are related to a specific component only.
cc: @imath
6 months ago
#13
Hi @dd32 & @renatonascalves thanks for your comments I agree we need to progress carefully. Small SVN commits are probably the best strategy. @dd32 if you can prepare small PRs it's wonderful 😍. But we can also take advantage of this one to pick the commits & rebase it after each commit.
renatonascalves commented on PR #214:
5 months ago
#14
@dd32 Let me know if you want me to merge/commit the current/available fixes in this pr.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
5 months ago
#16
@
5 months ago
- Milestone changed from 14.0.0 to Up Next
We will continue this ticket in the next version.
Converting
stripslashes( $_REQUEST )
and friends intowp_unslash( $_REQUEST )
, and then adding appropriate sanitization of any data.This is a work in progress, submitted as a PR for unit test autoruns.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/9065