Twenty Eleven: replace esc_attr( printf() ) with sprintf to prevent potential xss and potential broken code.

[chellycat]

The following code is problematic and may lead to xss as well as broken code depending on when/how it is used:

esc_attr( printf() )

Take for example the following test:

function mfields_printf_test() {
	$test = '<h1 style="font-size: 50px; font-weight: bold; color: red;">TACO!</h1>';

	$good = esc_attr( sprintf( $test ) );
	var_dump( $good );

	$bad = esc_attr( printf( $test ) );
	var_dump( $bad );
}
add_action( 'get_header', 'mfields_printf_test' );

This will produce the following output:

​https://img.skitch.com/20120310-cjfm9aiqmym87f5we647k9equh.png

Notice how the string in $good is correctly escaped while the string in $bad has been "converted" to a numeric string with the value of "70". This is because printf() is intended to echo a value to the screen. It does have a return a value which represents the length of the outputted string. Since test is 70 chars long, this value is 70.

Also notice how the string "TACO!" is echoed to the screen in large, red text. This proves that esc_attr() is bypassed by printf() allowing unescaped data to be echoed to the screen.

[SergeyBiryukov]

Not sure I understand the connection between the description and the patch, since there's no esc_attr() call in that line.

Furthermore, esc_attr( printf() ) doesn't seem to be used anywhere in Twenty Eleven.

According to ticket:19712:5, translated strings like that should only be escaped in attributes, which is not the case here.

[nacin]

:-(

Please do not post potential security vulnerabilities on Trac. ​http://codex.wordpress.org/FAQ_Security. There is an email address -- security -at- wordpress.org.

Thankfully, this is an invalid bug report.

get_the_author() returns the author's display name. On save, we sanitize the display name field by running it through sanitize_text_field() and kses. The HTML inside it is safe. (This occurs in sanitize_user_field()... we then attach callbacks to pre_user_display_name in default-filters.)

get_the_author() is *not* safe inside of an attribute, which is probably why there is some confusion here. For example:

echo '<span title="' . get_the_author() . '">';

A display name of " onclick="alert(0) will not get sanitized when saving the display name — it is valid. But, it does cause problems in an attribute. So esc_attr() is necessary in that case.

Not sure I understand the connection between the description and the patch, since there's no esc_attr() call in that line.

I imagine chellycat was referring to looking for esc_attr( printf() ) as a typo/thinko. This has occurred before, sadly, in Twenty Ten (and Twenty Eleven had some similar inconsistencies). But note that in [19582/trunk/wp-content/themes/twentyeleven/content-single.php], the escaping proposed here was specifically removed.