Non–late-escaping XSS sniff

JDGrimes commented 7 years ago

The XSS sniff that is currently a part of the standards is designed to enforce a philosophy of late escaping—everything should be escaped immediately before output. However, strict adherence to this is currently difficult, especially for themes, because of some patterns used in WordPress core. (Many WordPress functions return a string, rather than echoing it directly, and do not always escape everything that is incorporated into the output.) There are also times, like when reviewing code, where one is mostly interested in detecting low-hanging fruit, not forcing a late-escaping policy on another developer.

To remedy this, a new sniff may need to be introduced, though it is possible that the existing sniff could also be modified to have a less strict "mode". The idea would be to only flag unescaped output that incorporated known-to-be-dangerous values, like $_POST, $_GET, etc.

westonruter commented 7 years ago

What about using error for the low-hanging fruit and warning for anything in strict mode?

grappler commented 7 years ago

What about using error for the low-hanging fruit and warning for anything in strict mode?

I think it would depend on the ruleset.

So we will be having a list of three different types of functions.

Insecure functions `$unsafePrintingFunctions`

array(
    'get_theme_mod',
    'get_attachment_link',
    'get_options',
    '$_GET',
    '$_POST',
);

Mostly secure functions

Any non-pluggable function which core does not escape on output.

array(
'bloginfo',
'body_class',
'calendar_week_mod',
'comment_class',
'get_the_category_list',
'do_shortcode',
'get_archives_link',
'get_avatar',
'get_calendar',
'get_comment_author_link',
'get_comment_date',
'get_comment_time',
'get_previous_comments_link',
'get_previous_post_link',
'get_previous_posts_link',
'get_search_form',
'get_the_author_link',
'get_the_author',
'get_the_date',
'get_the_post_thumbnail',
'get_the_term_list',
'get_the_title',
'get_next_comments_link',
'get_next_post_link',
'get_next_posts_link',
'get_the_title',
'paginate_comments_links',
'post_password_required', // Needs to be double checked.
'post_type_archive_title',
'single_cat_title',
'single_month_title',
'single_post_title',
'single_tag_title',
'single_term_title',
'tag_description',
'term_description',
'the_date',
'the_modified_date',
'the_title_attribute',
'the_title',
'wp_dropdown_categories',
'wp_generate_tag_cloud',
'wp_get_archives',
'wp_get_attachment_image',
'wp_get_attachment_link',
'wp_link_pages',
'wp_list_authors',
'wp_list_bookmarks',
'wp_list_categories',
'wp_list_comments',
'wp_login_form',
'wp_loginout',
'wp_nav_menu',
'wp_register',
'wp_tag_cloud',
'wp_title',
);

Totally secure functions `$autoEscapedFunctions`

Any non-pluggable function that returns a known limited set of safe static values not directly based on the input arguments and cannot be modified (e.g with a filter) after escaping.
array(
'allowed_tags',
'checked',
'category_description',
'count',
'disabled',
'get_bookmark_field',
'get_bookmark',
'get_current_blog_id',
'get_delete_post_link', // Needs to be double checked.
'get_search_query', // Unless the parameter is set to false.
'get_the_ID',
'has_post_thumbnail',
'is_attachment',
'selected',
'walk_nav_menu_tree', // Needs to be double checked.
'wp_attachment_is_image',
);

WordPress VIP specific function

vip_powered_wpcom

Functions that echo and can't be escaped.

My recommendation would be to remove these functions.

bloginfo
body_class
cancel_comment_reply_link
comment_author_email_link
comment_author_email
comment_author_IP
comment_author_link
comment_author_rss
comment_author_url_link
comment_author_url
comment_author
comment_date
comment_excerpt
comment_form_title
comment_form
comment_id_fields
comment_ID
comment_reply_link
comment_text_rss
comment_text
comment_time
comment_type
comments_link
comments_number
comments_popup_link
comments_rss_link
edit_bookmark_link
edit_comment_link
edit_post_link
edit_tag_link
next_comments_link
next_image_link
next_post_link
next_posts_link
permalink_anchor
posts_nav_link
previous_comments_link
previous_image_link
previous_post_link
previous_posts_link
sticky_class
the_attachment_link
the_author_link
the_author_meta
the_author_posts_link
the_author_posts
the_author
the_category_rss
the_category
the_content_rss
the_content
the_date_xml
the_title_rss
wp_shortlink_wp_head

Fires an action

get_footer
get_header
get_sidebar
get_template_part
the_excerpt_rss
the_excerpt
the_feed_link
the_ID
the_meta
the_modified_author
the_modified_time
the_permalink
the_post_thumbnail
the_search_query
the_shortlink
the_tags
the_taxonomies
the_terms
the_time
wp_enqueue_script
wp_meta
wp_shortlink_header

Decision needed

comments_popup_script is deprecated and does nothing.
delete_get_calendar_cache does not have a return value so it should never be echoed
do_shortcode_tag is a private function and in core it is not echoed

My last question would be for the totally secure function the data would need to be escaped after the filter and not before?

JDGrimes commented 7 years ago

What about using error for the low-hanging fruit and warning for anything in strict mode?

That is another possible approach. It really doesn't matter to me, it depends on the different applications that people have for this. As @grappler, it might depend on the ruleset.

So we will be having a list of three different types of functions.

Yes, probably something like that.

My last question would be for the totally secure function the data would need to be escaped after the filter and not before?

Yes.

dingo-d commented 7 years ago

I've been going through the list as well, and so far I've found 2 deprecated functions in the $autoEscapedFunctions list.

Deprecated functions

comments_popup_script
comments_rss_link

@grappler

bloginfo() is safe as it echoes

echo get_bloginfo( $show, 'display' );

The display filter makes it safe to use (as commented here).

So far on my list of $autoEscapedFunctions are the following functions

allowed_tags
bloginfo
body_class
calendar_week_mod
category_description
checked
comment_author_IP
comment_form
comments_link
count
delete_get_calendar_cache
disabled
do_shortcode

I'm still going through the list, will update as I check more.

grappler commented 7 years ago

get_bloginfo() is not valid for $autoEscapedFunctions as the data can be modified via a filter that is why I have added to the Mostly secure functions.

Some of the items on your list conflict with the items on my list. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/845#issuecomment-279834686

dingo-d commented 7 years ago

I just checked if they are safe, I didn't check for 'mostly safe', that's why I have these still in the allow :)

And get_bloginfo isn't on the $autoEscapedFunctions list, so we're good there I think.

These are also on the secure list (generally secure, I don't differentiate between mostly and totally safe, I'll try to do this once I get them all separated into safe and non safe)

do_shortcode_tag
get_bookmark_field
get_bookmark
get_current_blog_id
get_footer
get_header
get_search_form
get_search_query
get_sidebar
get_template_part
get_the_ID
has_post_thumbnail
is_attachment
paginate_comments_links
permalink_anchor
selected
tag_description
term_description
the_date_xml
the_ID
the_permalink
the_search_query
the_taxonomies
the_title_attribute

jrfnl commented 7 years ago

@dingo-d Regarding deprecated functions being on the lists: IMHO they should be (correctly detected) for the purposes of these lists. There is a separate sniff to alert about the use of deprecated functions.

dingo-d commented 7 years ago

@jrfnl So the fact that they are on the list doesn't matter? Seems like a bit of an unnecessary clutter. If we are sorting these out, and have a separate sniff for deprecated functions, I think there's no need for them to be here.

jrfnl commented 7 years ago

@dingo-d I do not agree. These are two separate issues and should be sniffed for independently of each other. Someone might have either one of these sniffs disabled or use a ruleset which doesn't include one of them (VIP), the other sniff should still function correctly in that case. So deprecated functions should stay in the list.

grappler commented 7 years ago

@JDGrimes @jrfnl I have gone through the current list of $autoEscapedFunctions and have seperated them into different sections. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/845#issuecomment-279834686

I have just used the functions that we currently have listed. There are some functions missing but I think it would be easiest to add them once we have the changes.

There are a few functions that need to be double checked (see the inline comments) and a few that need a decision what to do with them.

JDGrimes commented 7 years ago

I have gone through the current list of $autoEscapedFunctions and have seperated them into different sections. #845 (comment)

Looks good. (I haven't double checked any of them yet, though.)

I have just used the functions that we currently have listed. There are some functions missing but I think it would be easiest to add them once we have the changes.

+1.

There are a few functions that need to be double checked and a few that need a decision what to do with them.

comments_popup_script is deprecated and does nothing.

Yeah, might as well remove it.

delete_get_calendar_cache does not have a return value so it should never be echoed

How did it even get on the list? 😄 (Just a rhetorical question, let's remove it.)

do_shortcode_tag is a private function and in core it is not echoed

I don't see why that would ever need to be echoed directly. It's not what it is designed for, and as you say, it is private, so I see no reason to keep in on the list.

My last question would be for the totally secure function the data would need to be escaped after the filter and not before?

Yes. The only exception would be for functions that are specifically escaping functions. For example, esc_html() has a filter, but it is obvious to anything that is using that filter that they need to be escaping whatever they return. For these functions with filters that aren't specifically for applying escaping to a value, they should not have any filter after they escape the data. If they do, then they don't belong on the totally secure function list.

WordPress / WordPress-Coding-Standards