WordPress Plugin Developers Don’t Always Have the Best Understanding of the Security of Their Plugins
Recently on a fairly regular basis we have been interacting with developers of WordPress plugins that seem to believe that they have a better understanding of security than they actually do, which has lead to them either not fixing vulnerabilities or not making security improvements to their plugins that they really should be. What makes dealing with that more irritating is when we see developers making changes to fix vulnerabilities that don’t actually exist in their plugins. So real vulnerabilities are not getting fixed, while non vulnerabilities are getting “fixed”. Usually that “fix” involves the developer duplicating security protection already being handled in the plugin or by WordPress. Other than causing the plugin to use slightly more resources than needed, this isn’t usually detrimental. If there is someone to fault for that, it is those making false claims about vulnerabilities in plugins, which occurs all too often.
As an example of an unneeded fix, let’s take a look at version 5.2.13 of bbPress. The release announcement states that the version:
adds some sanitization to anonymous user data that went missing from previous versions
and
If your site allows anonymous users (users without registered accounts) to create topics & replies in your forums, you’ll want to upgrade to 2.5.13 right away.
Looking at the changes made in that version you can see that in the file /includes/common/functions.php several values are now run through sanitization functions.
Here are how the important values looked as of version 2.5.12 (the other value is derived from one of these):
615 616 617 618 619 | $r = bbp_parse_args( $args, array ( 'bbp_anonymous_name' => !empty( $_POST['bbp_anonymous_name'] ) ? $_POST['bbp_anonymous_name'] : false, 'bbp_anonymous_email' => !empty( $_POST['bbp_anonymous_email'] ) ? $_POST['bbp_anonymous_email'] : false, 'bbp_anonymous_website' => !empty( $_POST['bbp_anonymous_website'] ) ? $_POST['bbp_anonymous_website'] : false, ), 'filter_anonymous_post_data' ); |
There you have three POST inputs being brought in without being sanitized, but the next lines down then do the sanitization:
621 622 623 624 625 626 627 628 629 630 631 | // Filter variables and add errors if necessary $r['bbp_anonymous_name'] = apply_filters( 'bbp_pre_anonymous_post_author_name', $r['bbp_anonymous_name'] ); if ( empty( $r['bbp_anonymous_name'] ) ) bbp_add_error( 'bbp_anonymous_name', __( '<strong>ERROR</strong>: Invalid author name submitted!', 'bbpress' ) ); $r['bbp_anonymous_email'] = apply_filters( 'bbp_pre_anonymous_post_author_email', $r['bbp_anonymous_email'] ); if ( empty( $r['bbp_anonymous_email'] ) ) bbp_add_error( 'bbp_anonymous_email', __( '<strong>ERROR</strong>: Invalid email address submitted!', 'bbpress' ) ); // Website is optional $r['bbp_anonymous_website'] = apply_filters( 'bbp_pre_anonymous_post_author_website', $r['bbp_anonymous_website'] ); |
That code calls filtering defined in the file /includes/core/filters.php:
214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 | /** * Add filters to anonymous post author data */ // Post author name add_filter( 'bbp_pre_anonymous_post_author_name', 'trim', 10 ); add_filter( 'bbp_pre_anonymous_post_author_name', 'sanitize_text_field', 10 ); add_filter( 'bbp_pre_anonymous_post_author_name', 'wp_filter_kses', 10 ); add_filter( 'bbp_pre_anonymous_post_author_name', '_wp_specialchars', 30 ); // Save email add_filter( 'bbp_pre_anonymous_post_author_email', 'trim', 10 ); add_filter( 'bbp_pre_anonymous_post_author_email', 'sanitize_email', 10 ); add_filter( 'bbp_pre_anonymous_post_author_email', 'wp_filter_kses', 10 ); // Save URL add_filter( 'bbp_pre_anonymous_post_author_website', 'trim', 10 ); add_filter( 'bbp_pre_anonymous_post_author_website', 'wp_strip_all_tags', 10 ); add_filter( 'bbp_pre_anonymous_post_author_website', 'esc_url_raw', 10 ); add_filter( 'bbp_pre_anonymous_post_author_website', 'wp_filter_kses', 10 ); |
For the author name and email field one the filters applied is the same as the sanitization added in version 5.2.13, a different one is applied to the URL:
615 616 617 618 619 | $r = bbp_parse_args( $args, array ( 'bbp_anonymous_name' => !empty( $_POST['bbp_anonymous_name'] ) ? sanitize_text_field( $_POST['bbp_anonymous_name'] ) : false, 'bbp_anonymous_email' => !empty( $_POST['bbp_anonymous_email'] ) ? sanitize_email( $_POST['bbp_anonymous_email'] ) : false, 'bbp_anonymous_website' => !empty( $_POST['bbp_anonymous_website'] ) ? sanitize_text_field( $_POST['bbp_anonymous_website'] ) : false, ), 'filter_anonymous_post_data' ); |
One of the reasons it is important get be accurate with claims that a new version fixes a vulnerability is something else mentioned in the release announcement:
Also, remember that since bbPress 2.5.12, the minimum WordPress version allowed is 4.7. If you need to user a previous version of WordPress, you will want to continue to use 2.5.11.
If you use our service we have checked on claims that a new version fixes a vulnerability before adding it our data set, so you will know that if we are indicating that a version is vulnerable than it actually is. And if see something were not listing in the data shown on the service’s companion plugin’s page (and haven’t sent you an email if you are using an impacted version) you can get in touch with us to see why that is.
I think in general it is better when the sanitization is more obvious, but there is no reason to do it twice. I would probably never rely just on a filter to provide sanitization in this way, because it is so easy to overlook. I’d say it is a poor design, and it leads to developers not knowing whether something is secure or not. Early sanitization and late escaping make it obvious whether something is safe or not.