The Perils of Partnership

If you’ve ever received an email offering to partner with you or to join an affiliate network or to help you earn money for your plugin, it’s probably a scam.

In the last three months, we’ve seen a serious uptick in emails like “please join our affiliate network” or “I can help you earn money” or “increase your plugin’s SEO” sent to plugin developers. On review, every last one that looked iffy has turned out to be by a nefarious or malicious group of people, who want to either install backdoors into plugins or black hat SEO links.

These deals should sound too good to be true, and they are. They can irreparably harm you, your reputation, and your standing on WordPress.org. Our reaction, when we see it, is to remove the plugin and revoke all SVN access from the developers involved. We don’t always restore access, especially if we feel you may fall for such a scam again or your online behavior is inherently insecure.

I know some of you are reading this thinking “Who falls for stupid stuff like that!” and the reality is anyone. All it takes is one mistake, one moment where you’re not thinking all the way through, and you’ve shot yourself in the foot.

There are some simple tips you can take to protect yourself.

  • Never let anyone else use your SVN account. If you work with a team, everyone should use their own account. This will help you track changes too.
  • Look up the people. Check that they seem legit. Are they using wordpress in their domain name (which you know is not permitted)? Do they already have any plugins? Are they active in the community?
  • What other kinds of plugins do they own? If the plugins are all over the place, ask yourself: Why would they want MY plugin? Companies that make a grab for a lot of different plugins are often trying to find ones with a high user count in order to spam.
  • Preview the code. Never add anything you’re not 100% sure is safe. If the code that gets added has links that look like http://api.wp' . '-example.com/api/upd' . 'ate or 'ht'.'tp://wpcdn.example.com/api/update/ then it’s not trustworthy (those aren’t the real URLs).
  • Does the email look like a form letter? WordPress is such a small community that people generally reach out like human beings. If someone’s spam-blasting a form, it’s sketchy.
  • Check spelling and grammar. If it’s `Wordpress` with a lower case P, or `JetPack` with an uppercase one, it might just be an innocent mistake, but it might not. Businesses should care about these things. After all, you do.

Above all, if you see something, say something. If you get an email like that, forward it on to plugins@wordpress.org with as much information as possible. We would love to see some code samples, for example, as we can add it to our scan routines.

#reminder, #security

Security Alert: Httpoxy

You may have heard about this already. Even so, please read this post. Normally we email all possibly impacted developers directly. In this case, trying to generate a list gave me over 6 gigs of results. I trimmed it down, but given the volume of people using Guzzle and possibly using suspect code, it was more straightforward to post an alert.

httpoxy is a set of vulnerabilities that affect application code running in CGI, or CGI-like environments. It comes down to a simple namespace conflict:

  • RFC 3875 (CGI) puts the HTTP Proxy header from a request into the environment variables as HTTP_PROXY
  • HTTP_PROXY is a popular environment variable used to configure an outgoing proxy

This leads to a remotely exploitable vulnerability.

You can read about the entire situation on httpoxy.org. While the fix is, as most are, a server one, all developers should be aware of this.

Don’t bother doing the following:

  • Using unset($_SERVER['HTTP_PROXY']) – it does not affect the value returned from getenv(), so is not an effective mitigation
  • Using putenv('HTTP_PROXY=') – it does not work either (to be precise: it only works if that value is coming from an actual environment variable rather than a header – so, it cannot be used for mitigation)

You can prevent and mitigate some of this in your code. Read up on httpoxy Prevention.

#security

Reminder: Your Email Account Must Be Valid

Since the only way we have to get in touch with plugin authors is their emails, we’re going to be enforcing that you have a valid email that goes to a human being for you plugins.

This simple statement covers a multitude of situations but to clarify, we’re talking about the email associated with the user accounts that have commit access to your plugins.

Go to https://wordpress.org/plugins/YOUR-PLUGIN/admin/ and look at the people listed under Committers. Those accounts are who we email when there’s an issue with a plugin, or when we’re alerting you to new WordPress updates. Those emails must go to real human beings. It can be a shared email box (goodness knows plugins is a shared email box) but real people have to read those emails because without that, we cannot communicate with you.

We strongly suggest you whitelist plugins@wordpress.org

The following email situations may result in your plugin being closed if we can’t find a way to communicate with you:

Invalid Emails

If your email bounces, your plugin gets closed. We can only assume that a dead email means you’re done with things, and since we have no way to contact you, your plugin can only be considered unsupportable. If you notice your plugin is closed and you didn’t get an email from us, check your account’s email. If that’s not right, that’s probably why.

Auto-Replies

If your email has an auto-reply, such as the sort that goes to a support ticket generator, stop it. This makes it nigh impossible for us to communicate with you, we can never tell if a human has read the email, and we get a mail box filled with auto-replies which means you’re the reason plugin reviews are backlogged. We will normally email you one sternly worded warning about this. If it keeps up, your plugin may be closed.

2-Step Verification

If your email auto-replies and asks people to click or reply in a special way to ensure our email gets to you, guess what? Half the time that doesn’t work. We often get expired tokens because it takes us more than 24 hours to get through all the emails in our queue, and once that happens we have no way to get our email to you.

Deceased Authors

This is a touchy subject so I apologize in advance. If a plugin author has died and we can verify this, we remove their account’s access to their plugins (and usually reset their passwords to something random). This is in the interest of security, as doing so will prevent any possible issues if their account is hacked. We do not close the plugins. If there are co-committers, they will be notified. Otherwise the plugin will simply remain in place. Taking over those plugins is a similarly touchy subject, and priority will be given to their coworkers or close friends/family who are also WordPress developers.

#email, #reminder, #repository, #security

Your Plugin Committers Should be Your Developers

After we started pushing back on the auto-reply stuff, a couple plugin devs said that they used their support accounts (like support@example.com) as their committer so that they could get email updates from the forums.

This is a terrible idea. It’s insecure and rather dangerous.

That means the support account is the one with write access to your plugin. And that means anyone with access to your group email (or your support tool) can click on reset password, get the password, change it, and blow up your plugin. Obviously that’s a major security issue. The only people who should have write access to your plugin should be people who know how to code and are responsible and reliable and trustworthy. And remember, people can go nutters in a company of any size and seek out revenge in weird ways. Limiting the damage they can do is your responsibility.

This also means that when we send you an email about your plugin, you may be accidentally sharing privileged information with people who have no business knowing these things. With a support account for a company of four people, it may be okay for everyone to know your plugin was pulled from the repository for a security hole. When you have a company of 300 and you use a system like ZenDesk (not to pick on them, but they are popular), now everyone knows. This may not seem like a big deal, but if one person tweets “OMG! Plugin FOO has a security hole!” then there’s a major risk that you’ve opened up the floodgates of potential hacks. Limiting the risks you put on your users is important.

Only allow your developer account(s) to have commit access to your plugin.

If you want one joint email-alias (wp-plugin-dev@example.com) that forwards to everyone, that’s okay, but not great. Remember, if everyone has their OWN user account, then you can easily track who pushed what change to a system. If you’re only using SVN as your version control, it’s a good idea to make sure you know who did what. If you’re using Git or Mercurial or your own SVN to track the changes, then make sure that only responsible, reliable, people have access to that dev account. Again, remember that we’re talking about access to push code to (say) a million users.

Remember: Anyone listed as a developer has the ability to remove anyone else as a committer. So you really want to limit those users.

Make a separate account to handle support

Make a separate account (wp-plugin-support@example.com) that does whatever it needs to do. Then you can sign up for email alerts. Go to https://wordpress.org/support/plugin/YOUR-PLUGIN and scroll to the bottom where it says “Subscribe to Emails for this Plugin”:

Click "Subscribe to Emails for this Plugin" when logged in.

Click “Subscribe to Emails for this Plugin” when logged in.

Click the link and baboom, that account gets email alerts. You can do the same for reviews at https://wordpress.org/support/view/plugin-reviews/YOUR-PLUGIN if you want to catch the inevitable ‘this review should have been a support post’ threads.

Remember: If you sign a support account up for getting those emails, you should still disable auto-replies. Otherwise you’ll be generating a lot of unnecessary email every time someone replies to your threads and you may get caught as a spammer.

Add your support accounts as Contributors

Contributors are the people you list under the ‘Authors’ field on your readme. They do not have any commit access to a plugin. They can’t edit the code.

Example: “Automattic” has an account for Jetpack. That account can be a placeholder account. It can be a support account if you want to use it in the forums. It can be marked as a Contributor in the plugin’s readme.txt in order to get special markings in the forums for replies from that account for that plugin.

The other accounts should be individual accounts, belonging to the devs, and preferably using their company email addresses. This way, if the organization changes, an individual leaves, etc, the email address still goes to the company and the plugin can be recovered, if necessary.

Back on Jetpack, there are over 70 people listed as ‘authors.’ They all get that happy plugin author green lozenge in their forum replies and they can officially help people without you worrying they’ll miss a semi-colon and take down 20 thousand users with a bad push of code.

Remember: Anyone listed as an author is going to get that green lozenge. If you don’t want people representing you, credit them in the readme but remove them as an author.

#reminder, #security

Reporting Plugin Issues

Note: I’ll be using Hello Dolly as my example ‘bad’ plugin for this post. It’s fine and not (to my knowledge) vulnerable.

There are a few reasons people report plugins but the main two are as follows:

  • Guideline violations
  • Security vulnerabilities

If you report a plugin, you can make everyone’s life easier if you do the following:

Verify that it’s still applicable

Before you do anything, check if the exploit is on the latest version of the code or not. If it’s not, we may not do anything about it, depending on how popular the plugin is.

Use a good subject line

“Plugin Vulnerability” is actually not good at all. “Plugin Vulnerability in Hello Dolly – 0 Day” is great.

Send it in plain text

SupportPress is a simple creature. It doesn’t like your fancy fonts and inline images. Attachments are fine, but we cannot read your ‘Replies in-line in red’ so just keep it simple.

Link to the plugin

https://wordpress.org/plugins/hello-dolly/

Yes, it’s that easy. Put the URL on it’s own line, no punctuation around it, for maximum compatibility. With over 35k plugins, and a lot with similar names, don’t assume, link.

If the plugin is not hosted on WordPress.org, I’m sorry, but there’s nothing we can do, so please don’t bother reporting it to us. We have no power there.

Explain the problem succinctly

Keep it simple.

“Hello Dolly has an XSS vulnerability” or “The Author of Hello Dolly is calling people names in the forums” or “Hello Dolly puts a link back to casino sites in your footer.”

Think of your intro like a tweet. Boil it down to the absolutely basic ‘this is what’s wrong.’

Keep the details clear

If someone’s acting up in the forums, link to the forum threads.

If you know that on line 53, the plugin has a vulnerability (or a link back to that casino site), then you can actually link right to that line: https://plugins.trac.wordpress.org/browser/hello-dolly/tags/1.6/hello.php#L53

We love that. If you don’t have that line, it’s okay. Tell us exactly what you see. “When I activate the plugin using theme X, I see a link to a casino site by my ‘powered by WordPress’ link.” Perfect. Now we know where to look when we test.

Show us how to exploit it

Don’t ask us ‘Can I send you an exploit?’ Just send us all the information. If the exploit’s already up online, like on Secunia, link us to it.

If you know exactly how to exploit it, tell us with a walk through. If the walkthrough involves a lot of weird code, you may want to consider using a PDF.

We’re going to take that information and, often, pass it on directly to the developers.

Tell us if you want them to have your contact info

We default to not passing it on, out of privacy, so “If the developer needs more help, I can be reached at…” is nice. Even “You can give the developer my information so they can credit me…”

We’re probably not going to follow up with you

We love the report, we review them, but we’re not going to loop you back in and tell you everything that’s going on for one very simple reason. We don’t have the time. If you told us to give the dev your contact info, then we did, but we don’t have any way to promise they will, and we don’t have the time to play middle management.

Emailing us over and over asking for status gets your emails deleted. It’s not personal, it’s seriously a time issue. We’re nothing more than gatekeepers, we are not a security company and we’re not equipped for keeping everyone up to date. We don’t have an administrative assistant to handle that. We work with the developer to fix the issue and we work with the .org team to see if we need to force update the plugin, and that takes a lot of time.

We don’t do bounties

This is a little interesting but basically we’re not going to pay you. A lot of people ask for ‘credit’ so they can ‘earn’ a bounty, and that’s cool, but we’re not going to report that for you. Generally if you say you want a bounty, we give your info to the plugin dev, though, so they do know you’re interested.

How do you report?

You can report plugins by emailing plugins@wordpress.org

That’s it 🙂 Thanks!

#repository, #security

Fixing add_query_arg() and remove_query_arg() usage

Background: Due to a now-fixed ambiguity in the documentation for the add_query_arg() and remove_query_arg() functions, many plugins were using them incorrectly, allowing for potential XSS attack vectors in their code.

Both add_query_arg() and remove_query_arg() have an optional argument to define the base query string to use. If this argument is undefined, it will use $_SERVER['REQUEST_URI'], which is unescaped. When printed out to a page, this could be used as an XSS attack vector.

The easiest way to fix this in your plugin is to escape the output of add_query_arg() and remove_query_arg(). When it’s being printed to a page (for example as a link), you should use esc_url(). When it’s being used in HTTP headers or as part of a HTTP request (for example, as part of a location redirect header or in a wp_remote_get() call), you should use esc_url_raw().

Edit by Ipstenu: Also read Sucuri’s reasonable disclosure on the matter. Many plugins have been patched and auto-updated in a massive coordinated effort to stem this one before it gets nasty.

#security

Automatic Plugin Security Updates

The WordPress.org security and plugin review teams have recently been working together to push automatic security updates for plugins to fix critical vulnerabilities. These updates are supported by WordPress 3.7+.

Andrew Nacin, a fellow lead developer of WordPress who helped write this post, wrote this after WordPress 3.7 was released:

“The automatic updater also supports themes and plugins on an opt-in basis. And by default, translations (for themes, plugins, and eventually core) are updated automatically. At some point in the future, the WordPress.org plugin security team will be able to suggest that installs automatically update malicious or dangerously insecure plugins. That’s a huge win for a safer web.”

Some have interpreted this as the end-user is required to opt-in, but it’s always been the case that it could be opt-in by either the site administrator, or by the WordPress.org security team if we deemed an issue severe enough to warrant it.

Back in April of 2014, the WordPress.org security team was contacted by Automattic with the details of a security issue affecting Jetpack, looking for help to get the update out to affected users as fast as possible (you can read more about that release over on the Jetpack Blog).

The team ultimately decided that leveraging our ability to issue a background update was the best option for the security of any WordPress site running the plugin. This decision was not made lightly, as it was the first time we would use the functionality.

A situation where we would have used automatic plugin updates was the security incident in July of 2011 where accounts of three plugin authors were breached and malicious updates were released. We were able to confirm that no other plugins were affected, as a precaution we reset the passwords of all WordPress.org users, but tens of thousands of sites were updated to a malicious version during a narrow window.

Unfortunately we weren’t so lucky back then, as we didn’t have automatic updates available to us. Thankfully, the malicious updates were detected quickly. But if a plugin author’s account is ever compromised again in the future, we’ll be able to remove the malicious update, and then push a security update for any site affected ASAP.

Since WordPress 3.7 was released, many sites have used the plugin automatic updates functionality, either by opting in directly through filters, or by using one of the many remote management services for WordPress that are available. We’ve had very few bug reports from those early users of the automatic plugin update functionality.

What is the process for the security team to push an update for a plugin?

The WordPress.org security team has only recently started to push more of these updates, only a handful of plugins have received the treatment, with vulnerability severity ranging from major to critical, affecting anywhere from 10,000 active installs to more than a million (Such as the WordPress SEO plugin this past week).

The process of approving a plugin for an automatic update, and rolling it out to WordPress users, is highly manual. The security team reviews all code changes in the release, verifies the issue and the fix, and confirms the plugin is safe to trigger an update. Rolling out an automatic update requires modification and deployment of the API code. This is the same standard and process for a core security release.

Because the process of pushing these updates is relatively recent, we haven’t previously formulated any guidelines as to when these pushes happen. We’re still iterating on them, but the current criteria we take into consideration for a security push is a simple list:

  1. Has the security team been made aware of the issue?
  2. How severe is the issue? What impact would it have on the security of a WordPress install, and the greater internet?
  3. Is the fix for the issue self-contained or does it add significant extra superfluous code?
  4. If multiple branches of the plugin are affected, has a release per branch been prepared?
  5. Can the update be safely installed automatically?

These requirements are defined in a way that anyone should be able to tick each box. (If a plugin author needs help, we’ll help them to make that happen.)

The first criterion — making the security team aware of the issue — is critical. Since it’s a tightly controlled process, the WordPress security team needs to be notified as early as possible. Letting us know is as simple as emailing us at plugins@wordpress.org with the details. If you’re not the plugin author, we’ll put you in touch with the plugin author and help coordinate the fix.

We’ll work with the plugin author (and the reporter, if different) to study the vulnerability and its exact exposure, verify the proposed fix, and determine what versions will be released and when.

As with WordPress core security releases, we prefer to see plugin releases which fix only the security issue, with minimal code changes and with no unrelated changes. It allows us to review the changes quickly and to be far more confident in them.

If a plugin has a security vulnerability in versions 2.0–2.1.1, and 2.1 introduced several new features but 2.1.1 only fixed a few minor bugs, we’d enable an automatic update for 2.1.x to 2.1.2 but not for 2.0.x to 2.1.2. If a significant amount of installs still used 2.0, we’d ask for a 2.0.x release to be made, which 2.0.x users would be updated to, securing their installations, but without significantly changing the plugin they run. (For Jetpack’s release, release packages were generated from 11 different branches.)

We want code changes to be minimal. The plugin shouldn’t require additional assistance during the update process in the form of user interaction or an upgrade routine — we want the process to “just work” every time.

Millions have received automatic updates for security releases of WordPress core. We want automatic plugin updates to be as safe, and as trusted.

We hope this clarifies why and when we’ll push automatic plugin security releases. It isn’t a decision we make lightly. The WordPress.org security and plugins teams only want to make the web a safer place for you and your visitors.

 

FAQ

Q: Why did plugin A get a automatic update, but plugin B didn’t?

It’s not bias from WordPress.org, it’s just a throwback to the manual process we’ve been using. If we’re alerted to an issue, we’ll work to handle it. If we find out several days later, the window of opportunity to get the fix rolled out has usually passed and it won’t be as effective.

If any plugin authors reading this have recently issued a security update and would like us to consider pushing an automatic update for the remaining users, please get in touch with us and we’ll do whatever we can to help.

Q: I keep my plugins updated already and don’t wish to have plugin security updates pushed automatically. How can I disable them?

There are several options to disable this functionality. The previous article for disabling core automatic updates applies here. Anything that disables all automatic update functionality will prevent plugin updates.
If you only wish to disable plugin updates, whether for all plugins or a single plugin, you can do so with a single filter call. See this Codex article for more.

Q: If I discover a security issue in my plugin, what should I do?

Email plugins@wordpress.org to seek support from us. We’re here to help you. You should start working on a fix for the issue, and share a patch of the changes for review before you release it if you want us to review the change or if you think an automatic update could be needed.

Q: Will you ever push an automatic theme update?

One day, perhaps — but not until we can do this safely. This is a complicated problem to solve. We don’t currently have a way to verify whether a theme was edited to customize a site. We never want an update to break a site or lose customizations.

You can always enable automatic theme updates using the filters outlined in this Codex article. Also, the theme review team catches most issues long before the theme is available for download, making theme updates far less likely to be needed.

Q: How can I get involved with the plugins review team?

As the plugins team deals with very sensitive issues, it’s a small group of well-known, highly trusted community members. It’s grinding work with a seemingly never-ending queue. If you’re interested, email plugins@wordpress.org.

Q: How can the WordPress security team trigger a background update for a plugin?

The auto_update_plugin filter is run on a flag present in the plugins update API response. By default, the flag is false, but it can be specifically enabled for a plugin. Core and translation background updates use the exact same mechanism, the only difference being their API responses have the flag enabled by default.

#security

How to fix the intentionally vulnerable plugin

If you have not reviewed the intentionally vulnerable plugin, then I suggest you do so before reading on.

The intentionally vulnerable plugin demonstrates a range of security vulnerabilities that are commonly seen in WordPress plugins. In this post I aim to go through each vulnerability giving a little background on why it is dangerous and how it should be fixed.

SQL injection

This is a rather famous vulnerability that can be exploited by an attacker to modify database queries. This can allow the attacker to read sensitive information from the database, modify data stored in the database, and possibly even execute commands on database server, among other things. The basic idea is that if untrusted user input is included directly in a query then maliciously formatted input can be used to alter the query’s syntax.

The first SQL injection vulnerability is caused by the incorrect usage of the wpdb::prepare() method on line 42:

$wpdb->query( $wpdb->prepare( "INSERT INTO login_audit (login, pass, ip, time) VALUES ('$login', '$pass', '$ip', '$time')" ) );

When using the prepare() method placeholders, such as %s and %d, should be used in the query string which is the first argument of the method — the method is not magic, it cannot parse the query to determine what was user input! The variables are then passed in as separate arguments so that they can be escaped properly before being included in the query. This rather common error has been made more obvious since the release of WordPress 3.5 which required more than one argument to be passed to the method.

The correct usage, and the fix, is:

$wpdb->query( $wpdb->prepare( "INSERT INTO login_audit (login, pass, ip, time) VALUES (%s, %s, %s, %s)", $login, $pass, $ip, $time ) );

The other two SQL injection vulnerabilities, on lines 102 and 127, are caused by incorrectly escaping user input that is not used in a quoted context of an SQL query. The esc_sql() function can only be safely used to escape input that will be used within quotes. However, the two queries are using input as a numeric argument that is not enclosed within quotes:

$log = $wpdb->get_row( "SELECT * FROM login_audit WHERE ID = " . esc_sql( $id ), ARRAY_A );
// ... and ...
$wpdb->query( "DELETE FROM login_audit WHERE ID = " . esc_sql( $_POST['id'] ) );

To fix this you should use the prepare() method with a %d placeholder:

$log = $wpdb->get_row( $wpdb->prepare( "SELECT * FROM login_audit WHERE ID = %d", $id ), ARRAY_A );
// ... and ...
$wpdb->query( $wpdb->prepare( "DELETE FROM login_audit WHERE ID = %d", $_POST['id'] ) );

It is also possible to sanitize numeric input by casting to int or using the absint() method.

Cross-site scripting

Cross-site scripting (XSS) is one of the most prevalent security vulnerabilities in web applications. XSS is also commonly found in WordPress plugins, so it features heavily in the intentionally vulnerable plugin. XSS is often classed as either “persistent” or “reflected”. An XSS vulnerability is persistent if the unsafe user input is stored in the database, and then output to other users in future requests. It is known as reflected if the unsafe user input is output immediately in response to a request containing the dangerous input. To exploit a reflected XSS vulnerability the attacker would have to convince a victim to click on their specially crafted link that triggers a malicious script. However, a persistent XSS vulnerability will allow the attacker to input their malicious script themselves and then wait for unsuspecting users to visit or click on benign looking links.

There are two instances of persistent XSS in the intentionally vulnerable plugin, in dvp_view_all_logs() and dvp_view_log(). Both of these functions output data straight from the database without escaping it. All of this data originated from the user in dvp_log_failed_login(). For example, an attacker could purposefully fail a login attempt using a password of <script>alert(1)</script>. The fix for this would be to use an appropriate escaping function, such as esc_html() or esc_attr() when outputting data from the database. Data can also be sanitized when being stored in the database using KSES, or strip_tags() when no HTML is expected, in addition to escaping on output.

There are also multiple examples of reflected XSS. On lines 104 and 115, the $id variable, which was originally $_GET['id'], is output without any escaping. Line 104 should be fixed with esc_html(), and line 115 should be fixed with esc_attr(). Another problem is the use of $_SERVER['PHP_SELF'] on line 116 to get the current URL. This is an extremely common problem that allows XSS since an attacker can control its value and include malicious HTML that will break out of the attribute and execute a script. This should be fixed by using a WordPress function, such as menu_page_url(), to get the correct URL.

Cross-site request forgery

Another extremely common vulnerability in web applications is the failure to prevent cross-site request forgeries (CSRF). This type of attack exploits a request handler that checks authorisation, but not intention. A malicious website is able to send a request to another site that the user is authenticated to. The vulnerable target site will accept this request since it is sent with valid authentication cookies. However, the user did not intend for the action to be performed. WordPress plugins can defend against this type of attack by using nonces. These are action dependent random strings that cannot be predicted by an attacker, but can be verified by WordPress. So, if a request does not include a valid nonce then it is rejected.

The intentionally vulnerable plugin looks as if it makes use of nonces to defend against CSRF, but the author has made a couple of mistakes that leave it exploitable. Firstly, the use of nonce generation and verification functions without passing the $action parameter is insecure. Lines 114 and 123 should be edited to add the use of an action. Note that this mistake would be caught when developing with WP_DEBUG enabled as this use of check_admin_referer() raises a notice.

The second mistake is more subtle. On line 137 the following check is made:

if (isset($_REQUEST['nonce']) && ! wp_verify_nonce($_REQUEST['nonce'], 'dvp_settings'))
    // ... failed nonce check

The problem with this check is that it requires the nonce request parameter to be set to ever evaluate to true. This means that an attacker can simply omit this parameter to bypass the nonce verification. This type of logic error means that it is highly recommended that you use check_admin_referer() instead of wp_verify_nonce() directly.

Missing capabilities checks

The lack of capabilities checks in the log deletion handler allows for a privilege escalation attack. Any logged in user is able to delete rows from the login_audit table since there are no privilege checks. Note that unauthenticated users cannot exploit this because the handler is not hooked into a _nopriv admin-post.php action. However, this is still a serious security failure. So, always remember to use the appropriate current_user_can() capabilities checks on your admin pages and privileged request handlers.

Failure to exit after a redirect

When a script redirects the user to another page execution will actually continue on the server side until it exits. This means that it is almost always desirable to make a call to exit or die() when making a redirect. The failure to do this on line 140 means that the capabilities check and CSRF defence (even if it didn’t have a logic error) are rendered useless as a forged request or request made by a low privilege user will actually cause the settings to be updated even though the conditional triggered a redirect. (Note that using check_admin_referer() would have also nullified the problem for a forged request since it calls die() on failure.) The other locations where redirects are used are not vulnerable in the same way because they are called at the end of script execution, but it would be good practice to add exits there as well.

This vulnerability is exacerbated by the fact that the plugin has a ridiculous update loop that would allow a successful attacker to update any option in the database. Instead a plugin should know which options it is in charge of and only modify those whitelisted options.

Open redirect

On a redirect related note, there is an open redirect vulnerability in the deletion handler on line 130. In this plugin the intention is to return the user to the list of failed login attempts, so wp_safe_redirect() should have been used instead of plain old wp_redirect(). However, in this situation it would be even better to redirect to a hardcoded URL using menu_page_url() or admin_url() and then remove the ability to specify the redirect through the request parameters.

IP forgery

The dvp_get_ip() function can be tricked into logging an incorrect IP address since the X-Forwarded-For header is user controllable. This is bad news for a variety of reasons. For security logging purposes it degrades the utility of the audit trail as an attacker pretends to be coming from a legitimate address. If it were used for securing access then it is trivially by-passable. In the case of this plugin this can also lead to SQL injection and persistent XSS.

Conclusion

There are many things that plugin authors can do wrong! The number one rule to remember is that the user is not to be trusted. Always validate and escape anything that could be controlled by a user.

I hope that participants found this to be a useful exercise and that this review post has helped your understanding of plugin security.

#security

Secure SWFUpload

Do you use SWFUpload? Have you been getting a lot of emails from us telling you it’s not secure? We have a solution. Or rather, WordPress has one.

FORK IT

The WordPress security team has officially forked the long-abandoned SWFUpload project and is strongly encouraging all web developers who use SWFUpload to update.

We strongly suggest you do not use SWFUpload. But if you must, use this fork.

Read More at Make/Core

#security

Review an intentionally vulnerable plugin

Imagine that another plugin author has asked you to look at a plugin that is currently in development to check for security flaws and help them fix any that are present. Would you know what to look for and how to fix the problems? Well, a fun challenge has arrived that will test, and hopefully improve, your knowledge in this crucial area of plugin development. I have developed a small, bug ridden plugin that requires a rigorous security review and suggestions for fixes.

The code is available from https://gist.github.com/joncave/5348689.

This is an incomplete plugin that aims to log any failed login attempts. Unfortunately, it actually harms the security of a site rather than enhancing it. All of the interesting parts are in vulnerable.php, so you should focus your review there. Please remember not to run this plugin on any server that is accessible to the internet!

If you spot a vulnerability whilst reviewing the code then make a note of the problem, where it’s located and what the problem is. Then come up with a patch that would solve the problem. It might also be beneficial to create a request that would demonstrate the vulnerability which can then be used to test your fix. I hope that this process will help you understand more about vulnerabilities, what sorts of things to look for when reviewing your own code, how to go about coding securely, and how to fix any problems in your own plugins if a flaw is found.

If you would like individual feedback on your finding and solutions, and to provide me with some information on which bugs people found and fixed, you can submit them via this survey. Please refrain from posting any spoilers in the comments for now.

In a week or so I will write another post detailing each of the vulnerabilities present in the code and how to fix them.

Bonus challenge: with access to a subscriber level account can you find any ways of extracting the data from an option named secret_option?

#security