And finally the page that retrieves the data from DB show.php
/// show.php
// The data is shown here
//Get post id
if( isset($_GET['id']) )
{
$id = (int) $_GET['id'];
if( $id == 0 )
{
redirect("index.php");
exit;
}
}
else
{
redirect("index.php");
exit;
}
$sql = $db->prepare("SELECT * FROM test WHERE id = :id");
/*** bind the paramaters ***/
$sql->bindParam(':id', $id, PDO::PARAM_INT);
/*** execute the prepared statement ***/
$sql->execute();
/*** fetch the results ***/
$result = $sql->fetchAll();
foreach($result as $row)
{
$id = $row['id'];
$name = $row['name'];
$title = $row['title'];
$phone = $row['phone'];
}
The code is bigger than this, but it pretty much the same method.
So the hacker was nice, and he/she said “FIX YOUR CODE”. I don’t even have a login system!! So, what could be wrong with this code??? I thought PDO is the king in filtering/sanitizing inputs!!
At a glance, I don’t see any security holes in what you posted. Youll need to give us more information and certainly more code. What did the hacker do? What leads you to believe that the snippets you pasted here are the culprit?
@Jeff, I have few more text fields and few Select Options. Users post and retrieve data. No, login system or anything.
I’ve tried to post xxs attack and PDO didn’t filter/escape anything. If a user post something like:
The scripts you have provided above are not vulnerable to injection attacks; it’s as simple as that. PDO’s prepared statements are doing exactly what they were designed to do; escape user input to prevent injections into the database.
What you are overlooking, however, is displaying the user’s output. You have not bothered to sanitise the data upon output, leaving you wide open to Cross-Site Scripting (or XSS) attacks. Anyone could enter <script>alert(‘Hacked’)</script> into any of the fields and they would have created a permanent XSS attack on your site.
To fix it, just apply htmlentites() to anything that you are outputting from a user’s input (this includes the session data you are returning to pre-fill the form input values):
Nor should it. Prepared statements protect against SQL injection, not XSS. To do that, you’ll always have to pass any untrusted data through either htmlentities or htmlspecialchars, like modernW showed.
Good point I really thought POD statements protects me from XSS. I just switched to PDO thou. I will read more tuts about it, I’ll update my code and see if the hacker comes by again.
Adding some validation to check what has been entered into the form is actually meaningful would help. Then you would reject the name <script>alert(‘Hacked’)</script> before it got anywhere near the database.
Validating properly when first reading the values into the script takes care of 99% of the possible security issues before the data gets used at all. Then only those fields that can validly contain values that could cause problems will get beyond that first step - which is completely missing in the code the OP posted.
I’m not sure that’s actually the best solution, because sometimes a user might enter that text for completely legitimate purposes, like you just did in your post. There are certainly good uses of validation, but I don’t think this is one of them.
You are talking nonsense. If that value is valid for the field then the validation would allow it through and you’d be relying on the htmlentities or similar call to prevent XSS. For the 99% of fields where it isn’t a valid entry you want the validation to reject it first thing before you end up with your database filled with a billion entries with stupid names like that one that are obviously NOT the person’s real name or address or phone number or email address or favourite colour etc.
As I said VALIDATION takes care of 99% of security plus it prevents you having a database full of junk. The ONLY fields you really need the prepare/bind and htmlentities calls for are those where the field VALIDLY contains a value that would otherwise cause confusion - for example a name can validly contain a ’ but not a < and so simply validating for valid characters would not prevent SQL injection (although other aspects of the validation might mean that an actual injection attack would still be rejected as an invalid name) but XSS of a valid name would be impossible.
Without validation you may as well not bother trying to prevent SQL injection or XSS as even if you do prevent those your database will be filled with meaningless garbage and may as well not exist.
You may have gotten a bit carried away here. First, just because a user can’t enter certain characters, such as “<”, doesn’t mean your database won’t accumulate junk. How many web sites out there do you think have registered e-mails for abcd@abcd.com? It’s a lot. And second, just because a database has accumulated some junk doesn’t mean it’s all junk, or even mostly junk. The vast majority of your data will probably be legit. And third, even if your database is mostly junk, that’s still no excuse to willfully leave you app unsecured.
There’s nothing wrong about relying on htmlentities (or htmlspecialchars) to prevent XSS because it’s effective. In fact, I think it is good practice to output any textual data from the db through htmlentities - do you suggest that we should use htmlentities only on data that has been properly validated? This is crazy, htmlentities is used in the code that is responsible for outputting data to the browser (like templates or views) and thinking whether each db field has the code somewhere else to validate data or not is unnecessary burden and doesn’t belong there. And what if one day you change how a field is validated? You’d have to go through all its usages in the templates and add or remove htmlentities - this results in nothing but a mess. All textual data should go through htmlentities or htmlspecialchars regardless of how much validation they have gone through. Besides, sometimes data can enter the database via other ways than your validating script so you can never be sure if it is safe to avoid escaping.
It looks like you are mixing validation with security and it can lead to unnecessary mess. I think validation should be kept separate from security (escaping). If you rely on proper validation to guarantee security then it becomes easier to find a dangerous loophole. Validation is about making sure that only data that make sense go into the db. But regardless of what data gets into the db, if this data is text entered by a user, the application should be made so that it remains secure no matter what garbage it is fed. And in most cases htmlentities is all that is required for displaying in HTML.
Also, I think it is good not to over-validate data if not absolutely necessary. For example, I see many people strip_tags() from every piece of text that comes from a form into the db. This way if someone enters some malicious html/javascript code it gets cleaned but at the same time I, as the owner of the site, don’t really know what was entered. The data may be rejected altogether by the validation scripts but this doesn’t mean I shouldn’t secure my application in the templates. If there is an attacker I prefer to know what he was trying to do and I want to see all the code he wanted to inject into my website, because that may help me identify the threat and learn more about it. It’s really not that hard to remember to escape all text data to the browser and then all such malicious scripts pose no threat since they are simply displayed as text for everyone to see.
I agree - even when you know that the field shouldn’t contain any data that could be harmful your application is more secure if you do that.
I think you are mixing escaping with security and it can lead to a huge mess. Both validation and escaping are important aspects of security. Validation is the more important of the two because it always applies whereas escaping is only necessary where data and code can be jumbled together.
Validation doesn’t guarantee security but not having validation almost guarantees that the opposite will be the case. One of the first things you need to concern yourself with in applying security to any application concerns tainted and untainted fields. Tainted fields ($_POST, $_GET $_COOKIES etc in PHP) can contain anything whereas untainted fields are ones where the data has been validated or at least sanitised to remove anything that is obvious garbage for that particular field. Until you have done that no statement that you feed the data into can be considered safe to run. You would have many potential security holes in your code long before you get to the point of trying to save the data into a database.
Both validation and sanitising are filtering techniques. As Chris Shiflett says on page 8 of his book Essential PHP Security - “Filtering is one of the cornerstones of web application security”. He doesn’t get to XSS and escaping data until page 23 because filtering provides protection against a greater range of security threats than escaping does. Some of the security issues of not using filtering are mentioned as early as page 2. On page 4 he introduces Defence in Depth which introduces the concept of escaping data even though it shouldn’t need it just in case something happens (such as you changing the validation to allow characters that would otherwise be problematic).
With a web application today the only place you need escaping is when outputting data into a web page. Everywhere else you can keep the data separated from the code. Filtering of all the tainted fields either through validation or sanitisation is an essential first step for every tainted field though since you do not want to be running any processing on tainted data as that processing will be insecure.
In this particular case I’m not mixing escaping with security because escaping is all that is needed for security - here. But I’m talking about the specific case of unstructured textual fields that the OP presented like name, title, etc. Such fields have no predefined structure, they are not intended to be parsed and processed in any way except some basic text manipulations like truncating, changing case, etc., in which cases even utter garbage will not do any harm. The data is supposed to be written in the db and then displayed to the user, maybe also searched by and that’s it. In this case proper escaping is all that is needed for security. Sure, garbage can be problematic somehow depending on the case but will be harmless.
But in more general terms you are right - if a field has a required data structure then validation is essential for security. For example, if there is a textarea/input for inputting JSON or XML code, or even email address, then I’d consider validation essential.
I think everyone here of course agrees that both validation and escaping are essential. The only point that I, and I think others here, would make is that validation should not try to do the escaper’s job. Validation should not try to forbid “<” characters, or anything like that. Or even if your goal is just to prevent a user from entering dummy data, then you’re fighting a losing battle. What’s to stop me from entering “blah blah blah”? You’ll never be successful at blocking dummy data, but you run the risk of creating false positives. Historically, we’ve seen false positives most often when developers incorrectly tried to validate email formats.
Since you are using enctype=“multipart/form-data”, this tells me that you are allowing a file upload.
What is there to prevent someone from uploading a .php file to your server?
If you allow people to upload php scripts to your server it’s game over no matter how secure you’re SQL is.
Iit should when such characters are not meaningful for the particular input field - Do you have an example of anyone whose name, address, or phone number contains that character? Validation of any of those particular input fields should treat that character as invalid.
The OP has both name and phone number as inputs and escaping those fields is only be necessary to protect against errors in the validation - such as this case where the validation of the fields is completely missing and Mr <script>alert(“you’re hacked”);</script> with his phone number of <script>alert(“you’re dead”);</script> has his name and phone number accepted by the system - with the escaping of course preventing his name and phone number being misinterpreted as containing HTML tags. If that name and phone number are valid then produce someone whose name and phone number actually look like that as evidence that such entries should be considered valid.
The security process to apply is called “defense in depth”. By validating the fields so that garbage characters can’t get in the field in the first place you make sure that any vulnerability in the escape function cannot be utilised to bypass that part of the security because it also has to get past the validation. It has been known for security holes to be found in code (such as escape functions) in the past and so further security holes are likely to be found in the future.
I really thought POD statements protects me from XSS. I just switched to PDO thou. I will read more tuts about it, I’ll update my code and see if the hacker comes by again.