On WordPress Security and Contributing

…and how neither really worked today.

A sad story in two parts, where I’m rash, harsh and untactful. An explanation, a rant, a call for support, a call for action. You do not have to agree with me, I may be just an asshole and haven’t realized it yet ?

Part 1: The %1$%s vulnerability

This post will overview two issues that have hit me where it hurts with the recent WordPress 4.8.2 release.

A handful of issues were addressed, among which a vulnerability in $wpdb->prepare which was fully disclosed 4 weeks ago here. But let’s start from the very beginning.

A very long time ago a bad decision was made. wpdb->prepare was born which was based on sprintf functionality. The Codex states that the method provides “sprintf-like” functionality but only supports %d, %s and %f placeholders. Cool. As time went on, people figured out that numbered placeholders also work in this function and it was used at liberty, all over the place. Yoast SEO used, some Automattic internal code used, as well as a crapton of other instances of usage from GitHub’s search: https://github.com/search?q=wpdb-%3Eprepare+%251%24s&type=Code&utf8=%E2%9C%93

What they didn’t know is that numbered placeholders are not safe to use. First and foremost, numbered placeholders were not escaped. So $wpdb->prepare( 'SELECT * FROM wp_posts WHERE post_ID = %1$s', '1 OR 1 = 1' ); would actually yield a SQL injection, as the placeholder is not quoted it would result in SELECT * FROM wp_posts WHERE post_ID = 1 OR 1 = 1;. Classic.

The developers who figured this out (because most of the SQL would actually break when using numbered placeholders without quotes) the usage was as safe as sprintf was.

Then along comes a security researcher, discloses a vulnerability in $wpdb->prepare and states that if a developer makes a mistake and types %1$%s instead, then there’s potential for SQL injection. How?

%1$%s gets transformed to %1$'%s' by $wpdb->prepare, because one of the features of the method is to unquote instances of "%s" and '%s' and then quote them again. What does sprintf do? %1$' get eaten up as an invalid placeholder, while %s' is left up for grabs and get transformed into whatever we wanted but only with a single quote at the end of it, breaking the SQL.

An exploit would need: 1. A developer to make a typo and not notice is in the highlighted syntax of the code editor, 2. the attacker to have access to the input parameters.

A hypothetical piece of code would be SELECT * FROM wp_users WHERE user_ID = %1$%s OR user_ID = %2$%s. With both inputs controlled by the attacker we can get the following if argument 1 is “1 OR 1 != ” and argument 2 is “injection”: SELECT * FROM wp_users WHERE user_ID = 1 OR 1 != ' OR user_ID = injection'. Which would simply return true for all users, because 1 is not equal to “OR user_ID = injection”, is it?

So think of the chances of such code existing in the wild. Now think of the chances $wpdb->prepare is not used. Probably slim. At least but what we know of the vulnerability and how much has been disclosed.

Four weeks later WordPress 4.8.2 enters the scene unexpectedly. And the patch it contains simply prevents anything other than %s, %f and %d from existing. Hell breaks lose. Sites with Yoast SEO installed on them stop functioning in the backend due to some code that relied on numbered placeholders. Needless to say, the aftermath of this decision will be felt for many months to come. Generated SQL with numbered placeholders is invalid SQL, a database error is thrown, a 500 error is returned in the browser.

They call this “hardening”. A patch to “prevent plugins and themes from accidentally causing a vulnerability”. That’s funny. Why don’t we prevent stray quotes in the query? Or why don’t we prevent calls to $wpdb->query without a preceding call to $wpdb->prepare or unite them into one and break code that does not escape?

But I’m very sure the security team have wrecked their heads for weeks and this was the decision that had to be made. Who stood behind the final decision, what was the reasoning, how will the aftermath be dealt with, nobody knows. Us risky developers can only comply. We can only wholeheartedly trust that the countless nights of sleep were lost pondering the better ways of solving the issue.

And I’m not going to comment on how a zero-day was out in the public for 4 weeks and how nobody was made aware of the breaking change to $wpdb->preapare. I don’t really care at this point. Policy is policy, whoever got into trouble has already spoken (or will) about how nobody was notified about a used breaking change. It was undocumented, it’s ultimately the fault of who used it.

Thus, I can’t call this a regression as developers relied on undocumented functionality. It’s not a a bug either. But wouldn’t it be great if $wpdb->prepare would actually support numbered placeholders? Seeing that it’s been used and will continue to be attempted to be used for years to come, I would say it’s a good idea. Why hasn’t anyone requested this before? Well, because it worked.

Now, since it no longer works. A trac ticket requesting the feature is due, so that the community can figure out a way to introduce documented support for these popular numbered placeholders.

Part 2: The feature request

Meet ticket #41925 the first of a handful of duplicates that wanted numbered placeholders “back”.

A discussion (or at least a monologue for now) to see how we can introduce their safe support in $wpdb->prepare, why the security team had to do what they did.

I quickly threw a patch together to get the ball rolling as well as some test cases.

In less than 12 hours a wontfix/close decision was made by one of the decision makers. Biased by some sort of undisclosed test cases that my patch failed to pass. Biased by what the team tried to do with the vulnerability. Biased by the internal misfortunes and hurdles they were met with while trying to solve it. Biased by the fact that numbered placeholders were not supported to begin with. Just like that. A wontfix/closed stamp.

Where’s the discussion? Where’s the test cases that fail? Is it not a valid feature request? We’ve had more absurd ones hang open for years. Why is a valid feature request, all of a sudden stamped in way similar to some of the most silly single-line feature requests out there? Why should a though-through, code-heavy and well-versed (at least in my opinion, but hey, I’m biased) ticket be carrying the stigma of a wontfix/closed? Discouraging people from actively participating in exploring the problem space and finding a solution because one of the higher-ups said “No way, Jose?”

A disrespectful spit in the face is what I felt. After spending over 8 hours trying to understand what the issue is, how we can begin to address it? Shut down. Just like that? And an hour after the feature request is closed as wontfix Slack has this weekly or monthly (I don’t know) new contributors day, where developers and designers are encouraged to work on issues, etc. Oh, the freaking irony! If this is not hypocrisy, I don’t know what is.

Again. It’s a feature request. It’s related to a vulnerability. In order to implement a good solution we have to dissect the vulnerability, there’s no other way, whether you like it or not. We have to write hundreds of test cases, try and break the proposed solutions. But a wontfix/closed? “Why bother?”, would say a passerby.

“Oh, but you can still discuss and comment on it. And you can even reopen it.” some would say. After all the “encouragement”? After a core committer does an action that is literally equal to saying “We won’t fix this, no point in trying. Shoo away, go home, kids.” when effort and determination to get things going is clearly shown? Such disregard is pretty offensive and depressing.

After speaking to several other people from the Russian community, I’ve been told that this behavior not unheard of. The almighty “No” of the higher ups, just because. The community is sometimes met with hurdles upon hurdles, a contributing environment that sometimes verges on the disgusting.

Is this really the WordPress open-source project?

Part 3: On safety nets vs. bad code.