Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#55853 new enhancement

Use of unsanitized data in wp_ajax_dashboard_widgets()

Reported by: hilayt24's profile hilayt24 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version:
Component: Widgets Keywords: close
Focuses: administration, coding-standards Cc:

Description

In the "wp-admin/includes/ajax-actions.php," there are much data that is unsanitized. Below is one example of it. I think it is good to sanitize all the fields properly to avoid unwanted scenarios.

$pagenow = $_GET['pagenow'];
	if ( 'dashboard-user' === $pagenow || 'dashboard-network' === $pagenow || 'dashboard' === $pagenow ) {
		set_current_screen( $pagenow );
	}

	switch ( $_GET['widget'] ) {
		case 'dashboard_primary':
			wp_dashboard_primary();
			break;
	}

Here the $_GET fields are used without any sanitization.

Change History (4)

#1 @sanzeeb3
2 years ago

In the example snippet above, $_GET fields are compared with the expected values only and aren't directly processed. I'm not sure about the other instances, though.

#2 @SergeyBiryukov
2 years ago

  • Component changed from Users to Widgets
  • Focuses administration added
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 6.1
  • Summary changed from Use of un sanitized data. to Use of unsanitized data in wp_ajax_dashboard_widgets()

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

I think comment:1 is correct, $_GET['pagenow'] and $_GET['widget'] are compared to a fixed set of values here, so it looks like sanitizing them is not strictly necessary.

That said, sanitize_key() could probably be applied here just in case.

#3 follow-up: @TimothyBlynJacobs
2 years ago

I'm actually not sure if we'd want to sanitize_key. It would mean that other values could get coerced into one of the correct values by sanitize_key. For instance dashboard$_primary would get sanitized to dashboard_primary which would make it through this conditional correctly, but the global would still be invalid and any other plugins that would be looking for the correct values wouldn't trigger.

Those kind of mismatches can also end up causing security issues in some cases. It's best to just compare this to a strict list of allowed items like we are already doing IMO.

#4 in reply to: ↑ 3 @SergeyBiryukov
2 years ago

  • Keywords close added; needs-patch good-first-bug removed
  • Milestone changed from 6.1 to Awaiting Review

Replying to TimothyBlynJacobs:

Those kind of mismatches can also end up causing security issues in some cases. It's best to just compare this to a strict list of allowed items like we are already doing IMO.

Right, I tend to agree. Thanks!

Note: See TracTickets for help on using tickets.