We get a lot of submissions to the WordPress.org plugin repository, and so there is often a lot of dangerous code submitted. Usually this isn’t malicious, it’s just by people who honestly don’t know that their code has problems. Understanding those problems is the first step to fixing them.
So here’s one common vulnerability we see in code submissions a lot: SQL Injection
To understand SQL Injection, let’s quote Wikipedia for a moment:
SQL injection is a code injection technique, used to attack data driven applications, in which malicious SQL statements are inserted into an entry field for execution
Here’s a piece of code made for WordPress, which is querying the database for a post:
// bad code, do not use $results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = $id" );
If you don’t see the problem with this code right away, then you should continue reading this post.
(Yes, this article shows the basics of the prepare() function. If you already know about the prepare() function, you might be shocked at the number of people who do not.)
The problem with SQL Injection vulnerabilities is that sometimes they can be hard to spot. The issue with the above code is actually context-dependent. The question you must answer is “What are the possible contents of the $id variable?”.
If $id = 123, then all is well.
But, if it is at all possible for $id = “-1; SELECT * from wp_users;” then you might have a real problem.
Sometimes we see code like this in submitted plugins:
// bad code, do not use $results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = ". $_GET['id'] );
Now, we have no idea what “id” contains, and in fact, we’re leaving that entirely up to the visitor of the site. Or the hacker of the site, in this case.
There should be no case where user-input can make it into an SQL statement without being first checked for sanity.
Sometimes, that check is easy. In this case, the ID should always be a number. So we can secure the query like so:
// kinda bad code, still, do not use $id = (int) $_GET['id']; $results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = $id" );
This is relatively safe, but not the recommended solution. It’s the naive approach, because we’re thinking that hey, we can just check the value ourselves and handle it accordingly. For integers, sure, but for more complex cases we can’t. Sometimes we can’t even do it for integers, so it’s best to avoid this sort of thinking entirely.
Don’t try to sanitize your inputs to SQL functions yourself. Let the sanitization functions do it for you. WordPress includes a function called prepare() to handle this safely.
The right way:
// good code $results = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $wpdb->posts WHERE ID = %d", $id ) );