WordPress Security (Or why you should code like a paranoid squirrel)

When I was at VIP we always argued for a very tough stance on security. To the point that we’ve been criticized for being over-zealous on escaping, permissions checking and nonce checks.

I understand many of the arguments made against enforcing late escaping. The one I understand and can empathize with the most is the one that goes something like: “If we just enforce rules without understanding the context, folks won’t understand why and when they really need to escape”. That’s a valid point, but I think it doesn’t work in terms of a large scale project. Be it your plugin or theme or even the WordPress project itself.

There’s often talk about two models of security. I’m sure they have better names but I call them the “Fortress” and the “Onion” model. The fortress being, that there is this one “moat” that protects the code. So internal functions for example can rely on the code being passed to them as being safe. The Onion model is kinda like if a paranoid squirrel wrote code. Every function should be suspicious of what it gets passed and doesn’t trust any other functions. I’m not sure where the squirrel fits in with my analogy to be honest, but I liked the thought of picturing a paranoid squirrel.

With that in mind, I would argue that the WordPress code base is currently not resilient enough to attacks as it often makes assumption about the data passed to it’s functions. A good example of this is the latest security release (5.1.1) that patches this: https://blog.ripstech.com/2019/wordpress-csrf-to-rce/

The offending code is this:

foreach ($atts as $name => $value) {
     $text .= $name . '="' . $value . '" ';
}

Anyone who’s done work with the VIP team will quickly see that this code violates the policy of “Always Escape Late”.

We would of had you rewrite that as:

foreach ($atts as $name => $value) {
     $text .= esc_attr( $name ) . '="' . esc_attr( $value ) . '" ';
}

So you would think this is an easy fix right? Just replace all the instances where we’re not late escaping to escape late. But I suspect that if I were to go thru the codebase and change all instances flagged by the PHPCS WordPress ruleset (and the VIP ruleset). The patches would be rejected.

The thinking, from my experience and my assumptions, would be that this could break backwards compatibility. A noble cause indeed. There is something to be said to not touching code that doesn’t “need” to be touched. It’s quite easy to introduce bugs or unintended consequences when adding escaping. It’s also possible that the escaping wouldn’t help or, in a small subset of circumstances, would make things vulnerable.

All this being said, I know from having seen how the sausage is made that it’s much more complicated than what may be interpreted from reading this blog post. I want to make sure it’s clear that what I’m suggesting in regards to WordPress security is not actually as easy a solution as this brief post may make it out to be.

There are many smart people who are working on this and they have a challenging task. I suspect my experience with enterprise clients has coloured my opinion in preferring security over backwards compatibility. Very good arguments could be made that if folks do not have confidence in the automatic updates (because of broken backwards compatibility) it would leave more users at risk than patching code that _may_ be a problem in the future.

But one thing I think is clear. If the code proactively was written to late escape, no matter where the data is from, we wouldn’t be in this situation. Hence, for all new code that you write, think of being like a paranoid squirrel. It’ll make your job way easier in the long run.

Leave a Reply

Your email address will not be published.