Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

line check iteration on non-String options value #96

Closed
stklcode opened this issue Jan 4, 2021 · 1 comment · Fixed by #97
Closed

line check iteration on non-String options value #96

stklcode opened this issue Jan 4, 2021 · 1 comment · Fixed by #97
Labels
Milestone

Comments

@stklcode
Copy link
Contributor

stklcode commented Jan 4, 2021

Originally reported in the WP support forums: https://wordpress.org/support/topic/php-notice-breaks-scan/

// Check option.
if ( $matches && $matches[1] && self::_check_file_line( get_option( $matches[1] ), $num ) ) {
array_push( $results, 'get_option' );
}

We do scan for a get_option() call, get the option and execute the line check again on the value. The return value however can have any type, not only string. Implicit conversion works quite well for most primitives, but fails for objects and arrays.

I am unsure about the intention behind this routine.
So do we need to add some sanitization here? Allow only scalar types that convert to string? Stringify the value? Personally I would simply drop the options check.

@stklcode stklcode added this to the 1.4.2 milestone Jan 4, 2021
@stklcode
Copy link
Contributor Author

stklcode commented Jan 8, 2021

I tried to resolve the problem introducing a is_scalar (is_string should also do the job, because we do not check for numbers or "true"/"false") filter:

// Check option.
if ( $matches && $matches[1]) {
	$opt_value = get_option( $matches[1] );
	if ( is_scalar( $opt_value ) && self::_check_file_line(strval( $opt_value ), $num ) ) {
		array_push( $results, 'get_option' );
	}
}

Stumbled across one really bad case. Imagine the following line of code:

$o = get_option( 'endless recursion' );

with the option value (string): get_option( 'endless recursion' )

This call does not cause any damage in the theme without an additional eval() or similar, but it breaks Antivirus causing in infinite loop on the server!

stklcode added a commit that referenced this issue Jan 8, 2021
Scalar option values do convert to string, however that no longer
applies to arrays and never did for objects. We now do check for
string values before the recursion to avoid warnings. Allowing other
scalars would not fail, but there is no check matching numbers or
boolean expressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant