Use of unsanitized data in wp_ajax_dashboard_widgets()

[hilayt24]

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.

[sanzeeb3]

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.

[SergeyBiryukov]

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.

[TimothyBlynJacobs]

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.

[SergeyBiryukov]

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!