Skip to:
Content

BuddyPress.org

Opened 5 months ago

Last modified 30 hours ago

#9065 assigned defect (bug)

PHP 8 Fatal error: Malformed inputs can cause fatals.

Reported by: dd32's profile dd32 Owned by: imath's profile imath
Milestone: Up Next Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch early
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 (17)

This ticket was mentioned in PR #214 on buddypress/buddypress by @dd32.


5 months ago
#1

  • Keywords has-patch added

Converting stripslashes( $_REQUEST ) and friends into wp_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

@dd32 commented on PR #214:


5 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.

@imath commented on PR #214:


5 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).

#4 @imath
5 months ago

  • Milestone changed from Awaiting Review to 12.1.0

@dd32 commented on PR #214:


5 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.

@imath commented on PR #214:


5 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)?

@dd32 commented on PR #214:


5 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.

#8 @imath
5 months ago

  • Milestone changed from 12.1.0 to 14.0.0

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


4 months ago

#10 @imath
4 months ago

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

@dd32 commented on PR #214:


5 weeks 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:


5 weeks 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

imath commented on PR #214:


5 weeks 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:


2 weeks 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.


2 weeks ago

#16 @espellcaste
41 hours ago

  • Milestone changed from 14.0.0 to Up Next

We will continue this ticket in the next version.

#17 @imath
30 hours ago

  • Keywords early added
Note: See TracTickets for help on using tickets.