11

I've been handed a vulnerability report(1) that seems to be implying that there may be a security issue in the way Wordpress handles URLs with following tildes. It seems the scanner thinks the website may be serving up some directory listings and the such.

I was surprised that my website was still serving content on those different URLs, so I did a test by installing a totally blank WP instance, switched to "Post name" permalinks, and confirmed that yep, any URL with added tilde still gets interpreted as the URL without the tilde.

Indeed, a url like this:

https://mywordpresssite.com/my-permalink

Is also accessible with the following URLS:

https://mywordpresssite.com/my-permalink~
https://mywordpresssite.com/my-permalink~/
https://mywordpresssite.com/my-permalink~~~~~~

I poked around a little bit to see where WP parses the permalinks, and I tracked it down to class-wp.php in the parse_request method, but couldn't get much further than that.

My question is if this is intended behaviour for WP, and if so, is there any way I can switch this off so tildes are not matched? Why would WP interpret URLs with tildes as a URL without them?

(1) Yep, now we've all seen a couple of major hacks and data leaks in the UK, it's that time again where the "security" guys all pretend they're doing their bit by handing us developers 200-page scan reports full of false-positives and generic issues they don't know anything about in the expectation if we read and act on said report, nothing bad will ever happen.

13
+50

Let's go simple

If I understand OP well, your problem is that urls containing a tilde are matched at all.

All other answers focus on the fact that sanitization for query strips out some characters before perform the query, however one should be capable to prevent a rewrite rule to don't match under some circumstances.

And it is doable, not very easy, but doable.

Why it matches, in first place?

The reason why two urls like example.com/postname and example.com/postname~ match the same rewrite rule is because WP rewrite rule for posts use the rewrite tag %postname% that is replaced by the regex ([^/]+) when rewrite rules are created.

Problem is that regex ([^/]+) also matches the postname postname~ and, because of sanitization, the name queried will be postname ending up in a valid result.

It means that if we are able to change the regex from ([^/]+) to ([^~/]+) tilde will not match anymore so we actively prevent urls containing tilde in post name to be matched.

Since no rule will match, the url will end up to be a 404, which should be the expected behavior, I think.

Prevent matching

add_rewrite_tag is a function that, despite its name, can be used to update an existing rewrite tag like %postname%.

So, if we use the code:

add_action('init', function() {
  add_rewrite_tag( '%postname%', '([^~/]+)', 'name=' );
});

we will reach our target and example.com/postname~ will not match the rule for example.com/postname.

So, yes, the 3 lines above is the only code you'll need.

However, before it works, you'll need to flush rewrite rules by visiting permalink settings page on backend.

Note that regex ([^~/]+) prevent a tilde to be anywhere in post name, not only as trailing character, but since post names can't actually contain tilde because of sanitization, that should be not a problem.

  • 1
    +1 like the simplicity ;-) it also looks like we could adjust this for other noise chars as well. – birgire Mar 30 '16 at 18:12
  • 1
    @birgire don't we all? ;) – gmazzap Mar 30 '16 at 18:15
  • @birgire yes, we could prevent any character stripped by sanitize_title, but since it is filterable, it's not possible to write an always valid solution. So I went specific. – gmazzap Mar 30 '16 at 18:19
  • 1
    This answer has by far the cleanest solution, and clearly explains the issue we're facing. Thanks a lot - bounty to you! – dKen Apr 3 '16 at 12:15
7

is intended behaviour for WP

Yes, as already explained, WP_Query::get_posts() uses sanitize_title_for_query() (which uses sanitize_title()) to sanitize the post name of a singular post.

In short, after the post name passed through sanitize_title_for_query(), my-permalink === my-permalink~~~ as sanitize_title_for_query() removes the trailing ~~~. You can test this by doing the following:

echo  sanitize_title_for_query( 'my-permalink~~~' )

is there any way I can switch this off so tildes are not matched

This is not something you can switch off. There is a filter in sanitize_title() called sanitize_title which you can use to alter the behavior of sanitize_title(), but that is almost always not a very good idea. SQL injection is very serious, so letting something slip through the cracks due to bad sanitation can have really bad influence on the integrity of your site. "Over sanitation" can sometimes be a pain in the butt.

I'm not sure what you are after, but I suspect that you maybe want to 404 single posts with these trailing tilde, in your words, "switch it off". The only way I can think of at this stage is to halt the main query when we have these trailing tildes. For this, we can filter the posts_where clause of the main query.

THE FILTER

Note: I only considered normal singular posts, and not static front pages or attachments, you can extend the filter to incorporate this

add_filter( 'posts_where', function ( $where, \WP_Query $q )
{
    // Only apply the filter on the main query
    if ( !$q->is_main_query() )
        return $where;

    // Only apply the filter on singular posts
    if ( !$q->is_singular() )
        return $where;

    // We are on a singular page, lets get the singular post name
    $name = sanitize_title_for_query( $q->query_vars['name'] );

    // Suppose $name is empty, like on ugly permalinks, lets bail and let WorPress handle it from here
    if ( !$name )
        return $where;

    // Get the single post URL
    $single_post_url = home_url( add_query_arg( [] ) );
    $parsed_url      = parse_url( $single_post_url );

    // Explode the url and return the page name from the path
    $exploded_pieces = explode( '/',  $parsed_url['path'] );
    $exploded_pieces = array_reverse( $exploded_pieces );

    // Loop through the pieces and return the part holding the pagename
    $raw_name = '';
    foreach ( $exploded_pieces as $piece ) {
        if ( false !== strpos( $piece, $name ) ) {
            $raw_name = $piece;

            break;
        }
    }

    // If $raw_name is empty, we have a serious stuff-up, lets bail and let WordPress handle this mess
    if ( !$raw_name )
        return $where;

    /**
     * All we need to do now is to match $name against $raw_name. If these two don't match,
     * we most probably have some extra crap in the post name/URL. We need to 404, even if the
     * the sanitized version of $raw_name would match $name. 
     */
    if ( $raw_name === $name )
        return $where;

    // $raw_name !== $name, lets halt the main query and 404
    $where .= " AND 0=1 ";

    // Remove the redirect_canonical action so we do not get redirected to the correct URL due to the 404
    remove_action( 'template_redirect', 'redirect_canonical' );

    return $where;
}, 10, 2 );

FEW NOTES

The above filter will return a 404 page when we have a URL like https://mywordpresssite.com/my-permalink~~~~~~. You can however, by removing remove_action( 'template_redirect', 'redirect_canonical' ); from the filter, have the query automatically redirect to https://mywordpresssite.com/my-permalink and display the single post due to redirect_canonical() which is hooked to template_redirect which handles redirection of WordPress generated 404's

7

Yes it seems strange that we should have the same match for:

example.tld/2016/03/29/test/

and e.g.

example.tld/2016/03/29/..!!$$~~test~~!!$$../

Why this is possible, seems to be this part of the WP_Query::get_posts() method:

if ( '' != $q['name'] ) {
    $q['name'] = sanitize_title_for_query( $q['name'] );

where sanitize_title_for_query() is defined as:

function sanitize_title_for_query( $title ) {
        return sanitize_title( $title, '', 'query' );
}

It should be possible to make this stricter with the sanitize_title filter, but it might not be a good idea to override the default output, based on sanitize_title_with_dashes, that is responsible for the sanitation here. You should consider creating a ticket instead of changing it, if there are no current once already about this behavior.

Update

I wonder if we could clean up the noise from the current the path with sanitize_title_for_query() and redirect to the cleaned url if necessary?

Here's a demo that you could play with on your test site and adjust to your needs:

/**
 * DEMO: Remove noise from url and redirect to the cleaned version if needed 
 */
add_action( 'init', function( )
{
    // Only for the front-end
    if( is_admin() )
        return;

    // Get current url
    $url = home_url( add_query_arg( [] ) );

    // Let's clean the current path with sanitize_title_for_query()
    $parse = parse_url( $url );
    $parts = explode( '/',  $parse['path'] );
    $parts = array_map( 'sanitize_title_for_query', $parts );   
    $path_clean = join( '/', $parts );
    $url_clean = home_url( $path_clean );
    if( ! empty( $parse['query'] ) )
        $url_clean .= '?' . $parse['query'];

    // Only redirect if the current url is noisy
    if( $url === $url_clean )
        return;
    wp_safe_redirect( esc_url_raw( $url_clean ) );
    exit;
} );

It might even be better to use sanitize_title_with_dashes() directly to avoid the filters and replace:

$parts = array_map( 'sanitize_title_for_query', $parts );

with:

foreach( $parts as &$part )
{
    $part = sanitize_title_with_dashes( $part, '', 'query' );
}

ps: I think I learned this trick, to get the current path with an empty add_query_arg( [] ), from @gmazzap ;-) This is also noted in the Codex. Thanks again to @gmazzap for the reminder of using esc_url() when displaying the output of add_query_arg( [] ) or esc_url_raw() when e.g. redirecting it. Check the previous Codex reference for that too.

  • +1 Just to clarify, those special characters get removed, so, although the strange version of the URL is visible in the location bar, WordPress does work with the actual URL, which is why the request works in the first place. I'm not seeing any mayor security risks with that behavior. – Nicolai Mar 29 '16 at 13:38
  • 1
    yes I think we shouldn't mess with the sanitation filter to change this @ialocin – birgire Mar 29 '16 at 13:57
  • 1
    Sure, unless there is a very good reason, it is a hassle not worth it. Not to say, it is most likely not good for the developers sanity - not even getting into the technical sanitation. Just my two cents though. – Nicolai Mar 29 '16 at 14:08
  • 1
    @birgire when used like so add_query_arg need to be escaped with esc_url or esc_url_raw to prevent security issues... – gmazzap Mar 29 '16 at 18:49
  • ahh yes thanks, if I recall correctly this was a security problem discovered in many plugins recently @gmazzap – birgire Mar 29 '16 at 18:51
3

Let me explain WordPress' processing of a request, and a method to change WordPress' behaviour to accomplish your goals accordingly.

Parsing the request

When WordPress receives a request, it starts a process of dissecting the request and transforming it into a page. The core of this process begins when WordPress main query method WP::main() is called. This function parses the query, as you correctly identified, in parse_request() (in includes/class-wp.php). There, WordPress tries to match the URL against one of the rewrite rules. When the URL is matched, it creates a query string of the URL parts and encodes these parts (everything between two slashes) using urlencode(), to prevent special characters such as & from messing up the query string. These encoded characters might have caused you to think that the problem resided there, but they're actually turned into their corresponding "real" characters when parsing the query string.

Running the query associated with the request

After WordPress has parsed the URL, it sets up the main query class, WP_Query, which is done in the same main() method of the WP class. The beef of WP_Query can be found in its get_posts() method where all query arguments are parsed and sanitized and the actual SQL query is constructed (and, eventually, run).

In this method, on line 2730, the following code is executed:

$q['name'] = sanitize_title_for_query( $q['name'] );

This sanitizes the post for fetching it from the posts table. Outputting debug info inside the loop shows that this is where the problem resides: your post name, my-permalink~, is transformed to my-permalink, which is then used to fetch the post from the database.

The post title sanitization function

The function sanitize_title_for_query calls sanitize_title with the proper parameters, which proceeds to sanitize the title. Now the core of this function is applying the sanitize_title filter:

$title = apply_filters( 'sanitize_title', $title, $raw_title, $context );

This filter has, in native WordPress, a single function attached to it: sanitize_title_with_dashes. I've written an extensive overview of what this function does, which can be found here. In this function, the line that's causing your problem is

$title = preg_replace('/[^%a-z0-9 _-]/', '', $title);

This line strips all characters except for alphanumerical characters, spaces, hyphens and underscores.

Solving your problem

So, there is basically a single way to solve your problem: removing the sanitize_title_with_dashes function from the filter and replacing it with your own function. This is actually not that difficult to do, but:

  1. When WordPress changes the internal process of sanitizing titles, this will have major effects on your website.
  2. Other plugins hooking into this filter might not correctly handle the new functionality.
  3. Most importantly: WordPress uses the result of the sanitize_title function directly in the SQL query by this line:

    $where .= " AND $wpdb->posts.post_name = '" . $q['name'] . "'";
    

    Should you ever consider changing the filter, be sure that you properly escape the title before it is used in the query!

Conclusion: solving your problem is not necessary as far as security is concerned, but should you want to do it, replace the sanitize_title_with_dashes with your own functionality and pay attention to SQL escaping.

NB all file names and line numbers correspond with WordPress 4.4.2 files.

3

Some folks have already explained the problem, so I'll just post an alternative solution. Should be pretty self-explanatory.

add_action( 'template_redirect', function() {
    global $wp;

    if ( ! is_singular() || empty( $wp->query_vars['name'] ) )
        return;

    if ( $wp->query_vars['name'] != get_query_var( 'name' ) ) {
        die( wp_redirect( get_permalink(), 301 ) );
        // or 404, or 403, or whatever you want.
    }
});

You will have to do something a bit different for hierarchical post types though, since WP_Query will run pagename through wp_basename and then sanitize it, so query_vars['pagename'] and get_query_var('pagename') won't match for children becuase the latter will not contain the parent part.

I wish redirect_canonical just took care of this crap.

0

THIS IS THE FIX... FOR WORDPRESS's BUG JUST ADD THE BEGIN security mod block above the Wordpress Generated BLOCK.

# BEGIN security mod
<IfModule mod_rewrite.c>
RewriteRule ^.*[~]+.*$ - [R=404]
</IfModule>
#END security mod

# BEGIN WordPress
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteBase /wordpress/
RewriteRule ^index\.php$ - [L]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule . /wordpress/index.php [L]
</IfModule>

# END WordPress
-3

You can always try adding adding the following to your .htaccess file:

RewriteEngine On
RewriteRule \.php~$ – [forbidden,last]

The second line above should go right under the first line shown. It should prevent index.php~ from displaying in URLs.

  • This doesn't work for the pretty permalinks the question is about, right? – Nicolai Mar 29 '16 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.