GDPR! It'll keep you safe! You should implement it!

Lorne Kates

WordPress ( TR ) isn't exactly GDPR compliant out of the box. Most softwarez aren't, tobe fair. But WordPress is an open platform, and very plug-in friendly. So surely some developer will come along and make a plug-in that will help with GDPR.

And so someone did:

Cookie Information | Free GDPR Consent Solution

Now, as the plugin says, it doesn't MAKE your site GDPR compliant. Instead it's a helper-- will help you identify GDPR issues, offer some suggestions, maybe fix some of the easy ones. It specifically knows about some other major plug-ins, so it can tie into their settings and let you change things as needed. 100K+ active installs since Nov 4th, 2017. Cool.

Except... because the EU insists that you keep all your users data safe, secure and subject to deletion / anonymization-- all for the public good of course-- you end up with developers more focused on making their product do that, rather than making their product NOT make all that data unsafe, exposed and hacked.

Wordfence Author / Nov 8, 2018

Privilege Escalation Flaw In WP GDPR Compliance Plugin Exploited In The Wild

After its removal from the WordPress plugin repository yesterday, the popular plugin WP GDPR Compliance released version 1.4.3, an update which patched multiple critical vulnerabilities. At the time of this writing, the plugin has been reinstated in the WordPress repository and has over 100,000...

tldr: doesn't validate the "change these options and do this action" data from the user, which allows anyone to change ANY WordPress options (such as "default_role = administrator"), or trigger ANY action,

you're still reading: this is because the plug-in gathers together a list of all the options it wants to change for GDPR, and all the actions (basically, Wordpress raises events for other code to hook into) that would make changes for GDPR purposes. It then sends out those name/value pairs of option name: value.

And on the postback, it just takes those option names from the $_POST collection, and dumps them all into the db. Not "I will maintain a list of the options I am allowed to change, and on postback ignore any option that isn't in that list". And it doesn't do that either for the do_action, meaning someone can trigger literally anything.

Yikes. 100K+ active installs of a plug-in specifically designed for people who aren't technical and may not be aware of security vulnerabilities.

Exploits went into the wild. Security researches identified the issues. Wordpress pulled the plugin. And the developer fixed the issue. The plug-in is now back online.

Normally that'd be the end of the WTF, right? Well-- for funsies, I decided to take a look at the changeset to see what had changed: https://plugins.trac.wordpress.org/changeset/1970366/

Sure enough there's the code that says "array_of_shit_I_may_change", and it checks the postback against that list. Good. But there's a couple more security fixes in there that-- ahh-- maybe should have been in there right from the beginning including:

adding a hidden, random nonce to the form, to prevent CSRF. Something that was common practice since the early '00s. And something that WordPress should have a "make an admin form" framework for because otherwise you get people who don't know what they're doing making admin forms that change arbitrary data
adding a check for the permission "manage_options". Meaning the plug-in WASN'T doing authorization checks before executing admin-level actions. Authorization checks!

Oh and for more fun, there are a couple OTHER fixes there in the release notes

Security fix: Removed base64_decode() function: meaning he wasn't doing HTML or input/output sanitation. Awesome!
Security fix: Correctly escape input in $wpdb->prepare() function. Not using parameterized queries. In 2018.

So, if you haven't lost confidence in this plug-in's ability to be secure yet-- given the developer didn't know to authorize the user, prevent CSRF, prevent SQL-Injection, or limit the scope of root-level software... I want you to take a look at the release history of the plug. It isn't very long, a quick read.

Cookie Information | Free GDPR Consent Solution

Okay, did you see it? Did you catch it? Did you get that little eyetwitch of "how wat" when you realized?

If you didn't get it, here it is in spoilers:

(and if I fuck up the spoilers because Markdown is shit, and our WYSIWYG doesn't have a button for it, oh well)
.
.
.
.
.

The plug-in has been in production since November 2017. An entire year from the time this was posted. And this latest update is the ONLY UPDATE TO HAVE ANY SECURITY FIXES. An entire year of development without paying attention to security. What else lurks in there?

dkf

@Lorne-Kates said in GDPR! It'll keep you safe! You should implement it!:

WordPress ( TR )

I stopped reading right there.

Rhywden

@dkf Yeah. "Weird clusterfuck software develops solution to a problem. Film at 11!"

xaade

@Lorne-Kates said in GDPR! It'll keep you safe! You should implement it!:

because the EU insists that you keep all your users data safe, secure and subject to deletion / anonymization-- all for the public good of course-- you end up with developers more focused on making their product do that, rather than making their product NOT make all that data unsafe, exposed and hacked.

Because entity insists that you do X, you end up with developers more focused on making their product do X rather than making their product NOT do the opposite of X?

xaade

@Rhywden said in GDPR! It'll keep you safe! You should implement it!:

@dkf Yeah. "Weird clusterfuck software develops solution to a problem. Film at 11!"

I don't understand why a plug-in is given access to do anything it wants.

loopback0

@xaade said in GDPR! It'll keep you safe! You should implement it!:

I don't understand why a plug-in is given access to do anything it wants.

You quoted the answer.

@Rhywden said in GDPR! It'll keep you safe! You should implement it!:

Weird clusterfuck software

anonymous234

@Lorne-Kates said in GDPR! It'll keep you safe! You should implement it!:

But WordPress is an open platform, and very plug-in friendly

Damn right. I had to learn basic WordPress development recently. Let me tell you what WordPress does:

Receive URL
Do user authentication.
Call template files to display the required "post" objects from the database.

That is all Wordpress provides. Users, posts, and arbitrary key-value attributes for each. Everything else is implemented on top of it with a metric buttload of PHP hooks.

I mean if you replace "post" with "object" it makes a nice base for a web framework, but really, that's all it is.

DogsB

@anonymous234 said in GDPR! It'll keep you safe! You should implement it!:

@Lorne-Kates said in GDPR! It'll keep you safe! You should implement it!:

But WordPress is an open platform, and very plug-in friendly

Damn right. I had to learn basic WordPress development recently. Let me tell you what WordPress does:

Receive URL

Do user authentication.

Call template files to display the required "post" objects from the database.

That is all Wordpress provides. Users, posts, and arbitrary key-value attributes for each. Everything else is implemented on top of it with a metric buttload of PHP hooks.

I mean if you replace "post" with "object" it makes a nice base for a web framework, but really, that's all it is.

Almost sounds like a back handed compliment. Burn the heretic!

anonymous234

@DogsB Well, it works well for what it does.

The problem here is if you delegate everything to plugins... well, you depend on plugins to be well made (and not interfere with each other).

Lorne Kates

@xaade said in GDPR! It'll keep you safe! You should implement it!:

@Rhywden said in GDPR! It'll keep you safe! You should implement it!:

@dkf Yeah. "Weird clusterfuck software develops solution to a problem. Film at 11!"

I don't understand why a plug-in is given access to do anything it wants.

Any WordPress plug-in code can do anything. There isn't any sandboxing or roles-and-permissions or rights.

It's a bit of PHP code that says "hey, Imma gonna raise an event now. Anyone who says they have something to do, I'll let you know when it's your turn."

Given that model, I can't see any way to implement a sandbox of any sorts, short of writing an entire inner platform. In PHP.

DCoder

@Lorne-Kates Unfortunately, this plugin is still above-average when compared to other WP plugins simply because they reacted to the problem. The entire WP ecosystem, including WP itself, is garbage through and through.

But because I can't resist watching the trainwreck, I looked at the source code (ze goggles, do {nothing() }, etc.).

Security fix: Correctly escape input in $wpdb->prepare() function. Not using parameterized queries. In 2018.

WP doesn't support parameterized queries period. Because there's still a neandarthal somewhere using an ancient hosting environment without the PHP extension required for parameterized queries, and WP won't add anything modern until that hosting environment turns into stardust. Instead they have a prepare function that works like printf or String.Format — you give it an SQL statement with placeholders and a bunch of variables, and it builds a complete query for you.

The developer of this plugin originally failed to even read the documentation of prepare(). It says, in the second fucking paragraph, All placeholders MUST be left unquoted in the query string because prepare will do the quoting for you. The developer previously quoted them: WHERE `user_email` = '%s'. This patch removed the quotes: WHERE `user_email` = %s.
If you're asking "then how the fuck did it work before, when the quotes were doubling up?", the answer is – prepare anticipates shit code like this and explicitly removes such wrapping quotes from the query before inserting the variables into it. Shit code counterbalancing shittier code, it's shit all the way down.

Security fix: Removed base64_decode() function: meaning he wasn't doing HTML or input/output sanitation. Awesome!

Nope. This is not about sanitization. This was … advanced stupidity. (Also, : base64 is not sanitization.)
The old version was mixing your email address with your session ID and serializing that into the form as a hidden POST variable, then unserializing it to read back your email to find the DB record it should be processing: getByEmailAddressAndSessionId(<unserialized data goes here>)

The first fix was saving this serialized data as a new DB column called token, treating this "token" as opaque data, and looking for the request through that. getByToken(<POSTed token goes here>). A step in the right direction, but the serialized data is still leaking to the browser.

The second fix took care of that. Now the token is generated from random data, but still in a curious way: substr(md5(openssl_random_pseudo_bytes(20)), -32). This will generate 20 random bytes, calculate the MD5 hash of those bytes as a hex string, and take the last 32 characters of that hex string. Why they felt the need to explicitly take the last 32 characters of a fixed 32 character string remains a mystery.

if you haven't lost confidence in this plug-in's ability to be secure

I don't have any confidence in WP or its plugins. Then again, I could say the same about many other popular PHP projects.

This quote has aged well:

wordpress is an unauthenticated remote shell that, as a useful side feature, also contains a blog

Source: bash.org

JBert

@DCoder said in GDPR! It'll keep you safe! You should implement it!:

The second fix took care of that. Now the token is generated from random data, but still in a curious way: substr(md5(openssl_random_pseudo_bytes(20)), -32). This will generate 20 random bytes, calculate the MD5 hash of those bytes as a hex string, and take the last 32 characters of that hex string. Why they felt the need to explicitly take the last 32 characters of a fixed 32 character string remains a mystery.

This is double stupid. You take 20 bytes of high-quality random bytes and then you pass it to a hash function which can do anything it wants with it including generating non-unique result values.

Had the author generated 24 random bytes and simply encoded them using base64 then he'd have had strings with 32 printable characters as well, and it would have been one of 2^{(8 * 24)} unique values whereas now it's at best 2^{(8 * 20)} with a risk of hash collisions giving non-unique values.

Disclaimer: I don't really know how well MD5 would do on short inputs like these, but again: why risk it if there are better approaches with more entropy?

Gąska

@JBert said in GDPR! It'll keep you safe! You should implement it!:

Had the author generated 24 random bytes and simply encoded them using base64 then he'd have had strings with 32 printable characters as well, and it would have been one of 28 * 24 unique values whereas now it's at best 28 * 20 with a risk of hash collisions giving non-unique values.

Not to mention that if you're not using MD5, you don't have to limit yourself to 32 characters anymore.

dkf

@JBert said in GDPR! It'll keep you safe! You should implement it!:

This is double stupid.

I'm surprised it wasn't forced through sha1 before md5…

Gąska

@dkf said in GDPR! It'll keep you safe! You should implement it!:

@JBert said in GDPR! It'll keep you safe! You should implement it!:

This is double stupid.

I'm surprised it wasn't forced through sha1 before md5…

Or the other way around.

JBert

@Gąska said in GDPR! It'll keep you safe! You should implement it!:

Not to mention that if you're not using MD5, you don't have to limit yourself to 32 characters anymore.

@dkf said in GDPR! It'll keep you safe! You should implement it!:

I'm surprised it wasn't forced through sha1 before md5…

Like entropy, stupidity can always be increased...

masonwheeler

@JBert said in GDPR! It'll keep you safe! You should implement it!:

"In before people theorizing about the peak level of entropy or stupidity: I don't want to know what the absolute upper limit is

[insert obvious politician joke here]

Lorne Kates

@DCoder said in GDPR! It'll keep you safe! You should implement it!:

This is not about sanitization. This was … advanced stupidity. (Also, : base64 is not sanitization.)

Yeah, I mistypo'd when I said santitization. I meant a shitty version of "encrypting".

Gąska

@Lorne-Kates that's not even encrypting. Encryption requires some secret to decode.

dkf

@Gąska said in GDPR! It'll keep you safe! You should implement it!:

that's not even encrypting. Encryption requires some secret to decode.

Or to encode. That's the mathematical magic of public key cryptography. (But base64 is neither; it's just encoding.)

Lorne Kates

@Gąska said in GDPR! It'll keep you safe! You should implement it!:

@Lorne-Kates that's not even encrypting. Encryption requires some secret to decode.

That's why I said shitty version... Because some developers think encoding is encrypting if you add some random characters to the start first

PJH

@DCoder said in GDPR! It'll keep you safe! You should implement it!:

The second fix took care of that. Now the token is generated from random data, but still in a curious way: substr(md5(openssl_random_pseudo_bytes(20)), -32). This will generate 20 random bytes, calculate the MD5 hash of those bytes as a hex string, and take the last 32 characters of that hex string. Why they felt the need to explicitly take the last 32 characters of a fixed 32 character string remains a mystery.

In case, in future, md5() starts returning more than 32 characters.

Obvs.

marczellm

@masonwheeler said in GDPR! It'll keep you safe! You should implement it!:

@JBert said in GDPR! It'll keep you safe! You should implement it!:

"In before people theorizing about the peak level of entropy or stupidity: I don't want to know what the absolute upper limit is

[insert obvious politician joke here]

I don't think you meant