Website Updates - May Critical Patch NotesPosted by Incineroar on 2020-08-21 00:18:38So finding this vulnerability was actually the result of testing the newer, updated website that I'm working on (Yes, I really am working on the comments update, which will be coming soon!) and wanted to find a way to test sites for vulnerabilities so that I can ensure the safety of data on Pushing Buttons. Inadvertently I actually ended up discovering a bug in my code that was accessible on the older version of the software and was able to quickly roll out some additions and changes to the code to ensure that the vulnerability cannot be used. I will detail what the bug was, what the vulnerability was, how it worked, and how it was fixed. The code here is being disclosed due to the fact I plan to open-source this software in the future, and I welcome any and all suggestions on code improvement. I want to clean it up anyway, it's not the best code in the world I'll admit.
What's The Bug?
A issue was identified where you could use what's called an SQL Injection Attack to add malicious code to a piece of information that gets sent to a server, and if the server does not handle this data correctly, the code can be executed, and can do all sorts of damaging things, such as accessing databases, or deleting them entirely. In this case, it was a minor oversight, however, content could be accessed. Fortunately, due to the current nature of this website, no user data was able to be accessed, as users currently cannot leave anything on the site (And thankfully, this was found before the comments update was rolled out!)What's The Vulnerability?
In this particular exploit, it used what was referred to as a "AND/OR time-based blind" attack, more commonly referred to as Blind SQL Injection. What happens is a query is sent that uses time to differentiate between a true or false answer, and depending on how long it takes for the server to respond, it can determine if it's true or false. It then slowly can figure out things like a database name, names of tables in the database, and much more, since it's now known where a query can be placed to be able to ask it these questions, over and over, such as "Does the first letter of this database begin with A?", followed by B, then C, etc. Once it can get the name of the database, it then can probe the database for things like tables or contents in that database, or, check if other databases exist.How was the vulnerability fixed?
To see how it was fixed, we first need to understand how it was broken. Let's look at the query that ended up being vulnerable first:function getBlogPostById($id) {
$db;
global $DATABASE_NAME, $DATABASE_PASSWORD, $DATABASE_USERNAME;
try
{
$db = new PDO("mysql:host=localhost;dbname=$DATABASE_NAME", $DATABASE_USERNAME, $DATABASE_PASSWORD);
}
catch (PDOException $e)
{
die("An error occured while establishing a connection with the database. Check the settings for the SQL user and try again.");
}
$id = $db->quote($id);
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=$id AND DELETED=0");
}When you loaded this post, the URL in your address bar has a value called "postid". When you access any posts on this site, that postid value changes, depending on the post you want to view. The error comes in how that value was handled. If we add some rogue SQL code to that value, it'll causes problems in this above function, specifically, this line:
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=$id AND DELETED=0");If used properly, the query used to load this page would have a postid value of 7. And when it gets put into that query after being copied to the variable $id, it'll look like so:
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=7 AND DELETED=0");Note that the 7 is substituted into the query. HOWEVER, the way the software handled that postid value wasn't thorough enough to make sure that it was being handled correctly. Each post is given a numeric value, but what happens if we add letters to that value? Obviously, since it won't match any posts in the database, it'll say a post wasn't found since letters are not numbers, and trying to find a match for a value that has only numbers will automatically fail, right? What if we put in just the right numbers AND letters such that we have a correct value, but we make it do more things? Take a look at a modified URL (Note that using this URL will not work properly anymore, as of the time of writing this bug has been successfully patched and tested) that can be put into your address bar:
https://pushingbuttons.tech/postview.php?postid=7 AND SLEEP(10)It'll look weird, that's because the spaces were converted to '%20' but the server will recognize it just the same as a space. What gives? Well, that query above will now change. $id will now contain the value "7 AND SLEEP(10)" which is now a problem. Why? Well, that's code we can just put in, which means that query will change. It will now look like this:
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=7 AND SLEEP(10) AND DELETED=0");This is not what we want to have happen! You can put anything in place of "SLEEP(10)" and have the server execute queries on your behalf, without having to have access to anything, and it can open the doors up nice and wide. But, how does it receive this query in the fist place? It was just a single line of code missing that caused this problem. Let's look at postview.php, where this code is run just before the page gets served to the reader:
[...]
if (isset($_GET["postid"]))
{
if ($_GET["postid"] == "")
{
$valid = false;
}
else
{
$postid = test_input($_GET["postid"]);
}
}
else
{
$valid = false;
}
#check the post ID and see if it's soft deleted or if it exists in the database at all.
if (isset($_GET["postid"]))
{
$post_contents = getBlogPostById($postid);
}
[...]The first part checks if there is a postid value in the URL. If not, the request is not valid. If it is, it checks that the value is not empty, and if it isn't empty, it runs a function called "test_input" that attempts to remove any issues from the value to prevent attacks, however, it's not perfect. Once that's done, it assigns it to a variable called $postid. Later on, it accesses a function called getBlogPostById, which is the function up above. We assume that test_input works, however, it failed in this case in our particular use. The fix is extremely simple though:
[...]
if (isset($_GET["postid"]))
{
if ($_GET["postid"] == "")
{
$valid = false;
}
else
{
$postid = test_input($_GET["postid"]);
$postid = preg_replace("/[^0-9]/", "", $postid ); //removes non-alphanumeric characters from the postid variable. This fixes a vulnerability.
}
}
else
{
$valid = false;
}
#check the post ID and see if it's soft deleted or if it exists in the database at all.
if (isset($_GET["postid"]))
{
$post_contents = getBlogPostById($postid);
}
[...]Highlighted in red is the fix. It does exactly what it says on the tin: It does a search over the $postid value, and looks for any values in that variable that DO NOT match numbers 0-9, and replaces them with an empty character, "", essentially deleting it, then assigns it back to the $postid value. If you want to turn that from two separate steps into one more elegant step and make it slightly more fancy, that would look like so:
$postid = preg_replace("/[^0-9]/", "", test_input($_GET["postid"]) );Either way, that will then change the result of that modified URL from earlier so the query goes from
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=7 AND SLEEP(10) AND DELETED=0");to
return $db->query("SELECT POST_NAME, POST_CONTENT, POST_DATE FROM BLOG_POST WHERE POST_ID=710 AND DELETED=0");which may or may not be correct, depending on if a 710th post exists on the site, but that doesn't matter because we're cleaning up an input that was incorrect in the first place, and that's our attempt to clean it up. We can't trust what someone types in, so we have to ensure it's correct, even if it means we give another incorrect response to the user. One golden rule is to never trust what a user types into your applications, and this is a reason why that's the case.
Anyways, with this bug fixed I'll continue working on the post comments update that should be coming soon. I want to launch it before I go back to work, but I also want a sufficient amount of time to test this new feature so it's not ludicrously broken like this one is. Please look forward to the update!