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.

Late Escaping in WordPress

Late escaping is often seen as unnecessary. I remember when I used to think this:

“Why should I late escape something that I know is safe?”

Let’s examine the various parts of that statement.  The “I” really means not just me — the developer currently writing the code — but me at this current moment in time. While debugging old code, how often have you asked yourself who the “genius” who thought up some “clever” solution was only to realize you were the one who’d written it 6 months ago?

Late escaping future-proofs your code by making it easy to spot escaping. Otherwise, to check the security of it, you’d probably need to re-read all of the code surrounding it, check which functions are called, where the inputs come from, etc. It’s easier and less time-consuming to rely on late escaping instead.

The second part of this statement I want to examine is the “is safe” part. Safe in this context really means, “is not currently known to be insecure” with currently being the key word. The statement “the code is safe” is probably accurate at the point in time when you originally commit it. The problem is, code has a tendency to change. The function you’re calling that returns the “currently safe” code might change in the future. Or, the inputs to that function will change and be from a source you didn’t initially anticipate. That change could introduce user-provided data in a way you didn’t expect and end up being insecure. It’s safest to rely on late-escaping because it’s more resilient to unanticipated changes in the future.

I sometimes hear from developers who are worried that late escaping will hurt the performance of their site. Escaping — even with functions traditionally thought of as slow such as wp_kses_post() — is a drop in the bucket compared to one additional MySQL query on a site. If you take a look at this great post by Zack Tollman on the performance of wp_kses, you can see that older versions of PHP were a bit slow on long content. However, running wp_kses on longer content in PHP 7 and above* shows performance improvements similar to those of HHVM.

So you can rest easy — adding late escaping won’t slow down your site, and it offers many benefits:

  1. It’s easier to scan the code using PHPCS
  2. It’s easier to read during peer code review
  3. It’s more resilient to changes in other parts of the codebase
  4. Removes ambiguity and adds clarity for future code maintainers (including yourself!)
  5. Negligible performance impact

 

*For reference, WordPress VIP runs PHP 7 at a minimum