Hi there , i want to ask for some opinion about my ‘play-around’ CMS that i manage to build just for educational purpose . Its not finished yet but in this state i want to know did I am on the right way from actually build something useful or just another crap of code.
Thanks!
There are several problems with your CMS. They are
- The password hash you are using isn’t a password hash. Any SHA functions shouldn’t be used for password hashing. Use PHP’s default hash. It’s much much more secure than SHA functions and the regular hash function.
- As I have said many many times to people, don’t use isset($_POST[‘submit’]) or isset($_POST) or anything that deals with $_POST in general for your form submit checking. The only reason why you should use isset is on $_GET parameters, checking if a session was set, and checking if a selection option was selected. Checking for selected option is the only exceptional part of $_POST because you can’t do
if($_POST['select'] == '') {
It has to beif(isset($_POST['select'])) {or your validations will be bypassed. For some reason this happens. I’m not sure. I could be wrong. Anywho, use $**_**SERVER[‘REQUEST_METHOD’], here is an article that I keep giving people to look at. http://stackoverflow.com/questions/10943060/isset-postsubmit-vs-serverrequest-method-post - It is recommended to use prepare statements. Prepared statements aren’t really necessary at all times, but if you are using WHERE clause, you should be using prepared statements. If you stuff the actual data into the query string, you are prone to SQL Injection. MySQLi_* has prepared statements as well as PDO.
- Letting anyone go to the admin page is bad. You should make user levels so that if someone were a regular user, they can’t access the admin page. Only users with admin permission should be able to access the admin page. If you had pages that listed user’s personal information and you allowed anyone to go to the admin page, your users wouldn’t want to use your service because their information could be compromised.
- Don’t use * (asterisk) in your query string. This could lead to your whole system being compromised. Since you are using regular queries and not prepared statements, if a hacker broke into your server, they could obtain all of your tables because your server is compromised.
Let’s say you have this table structure.
+----------------------------------------------------+
| sample_table |
+----------------------------------------------------+
| COLUMNS | DATATYPE |
+----------------------+-----------------------------+
| sample_column | int(11) NOT NULL |
+----------------------+-----------------------------+
| sample_name | varchar(32) DEFAUL NULL |
+----------------------+-----------------------------+
| sample_description | longtext DEFAUL NULL |
+----------------------+-----------------------------+
And all you are looking for and outputting is just the sample_name column. Well, using the * (asterisk) will allow the hacker to see all 3 columns of your table and
+---------------------------------------------------------------------+
| admin_table |
+----------------------+----------------------------------------------+
| COLUMNS | DATATYPE |
+----------------------+----------------------------------------------+
| id | int(11) NOT NULL AUTO_INCREMENT PRIMARY KEY |
+----------------------+----------------------------------------------+
| email | varchar(255) DEFAUL NULL |
+----------------------+----------------------------------------------+
| password | varchar(60) DEFAUL NULL |
+----------------------+----------------------------------------------+
| signup_date | int(11) NOT NULL |
+----------------------+----------------------------------------------+
| ip_address | varchar(20) DEFAUL NULL |
+----------------------+----------------------------------------------+
| user_agent | text DEFAUL NULL |
+----------------------+----------------------------------------------+
| user_browser | text DEFAUL NULL |
+----------------------+----------------------------------------------+
But wait, you didn’t specify admin_table did you? Well, using * (asterisks) will allow hackers to see all data tables whether you like it or not.
All this aside, I actually like it that you are using OOP for some parts. I recommend reading up on MVC. It could help you a lot.
NOTE: If I had said or made any mistakes, please correct me. I don’t want to look stupid.
1 Like
Hi @insahn,
Welcome to the forum.
Every GitHub expects a README. file with brief details describing your project, installation requirements, etc
Can you supply more information of your project.
Much appreciate your comment! Thanks! I totally agree with your opinion about security risks and to that moment i did not realize the (asterisk *) could lead to SQL injections or database compromising thanks for mentioning it. About user level login i specify only one level of 1 that should be the admin level regular user won’t have account for the admin panel . Or maybe i should rename the table ‘users’ to ‘admins’ another misunderstood mistake. I hardly understand PDO at the moment that’s why i’m not approaching with it but if it’s necessary for better security will manage to use it more. About hash and sha1 can you lead me to the differences because i cant see in the educational lessons someone to tell benefits from using those two in different situations. And final should i restructure my database or just continue to fill it that way . Thanks again for the important advices. 
I totally forgot to include a README.file terribly sorry for that but at the this state there is not much to be expected from the project that’s way i posted it here to look for some advice from more experienced people .
Short info: It represents a simple website that fetches dynamically its content ,pages images links navigation menus from database and if someone wants to edit the information (it has to be registered admin in the database) can restructure the whole site from the admin panel [its basically the same as index page as style and graphics] .Right now the admin panel is empty it only shows logged admin name and that’s it. I will manage to code it in a few days . There is no installation instructions because at this stage i’m only doing the back-end logic . That’s why your opinions and advices are necessary for continuing.
1 Like
If a hacker broke into the server then surely he has access to all the data and the system is already compromised. What does using * have anything to do with it?
1 Like
PHP’s password hash uses the BCrypt cipher by default. Passwords will be stored with their own individual salts.
SHA1 is just a hash. The way you have it, if your password is password it will always store the string 5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8, which is about as good as storing the password in plain text. Even if you salted the individual passwords it won’t be enough to make the database very secure at all because a computer could generate a very large list of precomputed hash functions quickly.
The PHP documents cover it pretty well:
1 Like
What I meant is since OP is using regular queries, by adding the * (asterisk) wildcard to the query line, it will be even worst. Let’s say OP insists on using regular queries and happens to get SQL Injected, well if OP only specified sample_name column, all the hacker is going to see is sample_name column and nothing else. But if OP uses the * (asterisk) wildcard with the regular query, the hacker will see sample_column, sample_name, sample_description, and many others if OP intends to use the * (asterisk) wildcard in the query string.
It is also best practice to just specify which column you need if you only need to use 1 column.
I could be wrong. Correct me if you think I am wrong.
You don’t need to use PDO, I just mentioned it because just like MySQLi_, PDO has prepared statements as well. I linked MySQLi_'s prepared statement in my above post.
SHA and password_hash are 2 different things. SHA is common amongst bad practice when it comes to password hashing. I am a shamed admitting that I too used to use SHA, but I used SHA256, a much more stronger hash, but then I learned about password_hash and moved on. Any hash such as SHA, MD5, and the regular hash can be deciphered. Other than seeing people use it in a password hash, I’ve seen other places too like OpenSSL and some files. On github, some people use the hash for files, not sure what it’s meant for.
With password_hash, you can only go one way. You can’t hash it and un-hash it. This makes deciphering a password much harder to do, but not impossible.
Here is a very detailed image of what a password_hash looks like from the link @mawburn supplied.
http://php.net/manual/en/images/2a34c7f2e658f6ae74f3869f2aa5886f-crypt-text-rendered.svg
And no, you shouldn’t restructure your database. It is fine the way it is. It’s just the PHP codes that need a little more working on.
1 Like
A couple of additional benefits from using password_hash
- If you use it with the other password functions then the password can have a new hash using a different salt created every so often even though the password itself hasn’t changed.
- There is provision for better hashing algorithms to be introduced to make the passwords more secure in the future and the passwords can be updated to use that as people log in without affecting the validation of the password against the old hash. Not all passwords need to be using the same hashing algorithm.
1 Like
Something my Ethical Hacking professor told me, “Leave the encryption to the experts.” He told us a story that once in his past, a company hired him to secure their stuff with an encryption of some sort. He said he thought of many ways on how to do it and decided to use his own. He said looking back at it, he could of stolen a lot of stuff or even screw up their whole system because of how unsecured he made the encryption.
It wasn’t even close to how secure the password_hash is now a days. If anyone insists on re-inventing the wheel, try asking an encryption guru first. They can give you a better idea than I can.
I never knew about this. You learn something new everyday.
Another thing to note when it comes to user passwords. Don’t limit the amount of characters they can use and don’t limit the characters they can use. This makes the password even weaker than it should be.
Give a good reason why little Daisy here can’t use the password
`i$jm'/#@zn*^
Just because special characters like the back-tick, dollar sign, single quote, forward-slash, pound sign, at sign, asterisk, and caret is being used in the password, doesn’t mean it’s meant for SQL Injection. What if that is a password they’d like to choose because it isn’t an average password like password123. And it is more secure than password123.
I hate using services that only let me use alphanumeric characters. It scares me to see that my info can easily be obtain through rainbow tables.
2 Likes
And what happens if someone starts hitting your site with megabyte passwords? Try hashing one of those. Can you say “denial of service”?
1000 characters or so is a reasonable limit.
What about md5 ? less secure or same as sha1 .
Neither are secure alone. They basically turn text into defined length text. They aren’t meant for storing passwords, but have been used to do so for years using the salts I mentioned. It’s easier to just use PHP’s password hash function. BCrypt is solid and there an implementation in just about any language you want.
Now it is clearly enough to understand what should i use instead of those two regular hashes . Much appreciate your advices .Thanks a lot !
I manage to figure out usage of PDO last night . I’m gonna use PDO prepared statement query for selecting user when log in to the admin panel . Should i use prepared state for selecting content from database or i can approach selecting content like that →
$query = $con->query(“SELECT title,id FROM pages WHERE visible=‘1’ ORDER BY position”);
while ($r = $query->fetch()) {
$ptitle = $r['title'];
$pid = $r['id'];
Denial of Service (DDoS) has nothing to do with password hashing. DDoS is the action for script kiddies trying to take down a server. What you probably meant was buffer-overflow.
But yes. Obiviously there should be some point in time where you have to limit the characters or a buffer-overflow will happen. Since your table structure has a limit, you have to limit the long passwords. But what I meant in my earlier post was not to limit to let’s say 5 characters long. That’s not long enough.
With the snippet you provided, there are actually a few problems.
- You are still putting the actual user input into the query string without knowing this could be a problem that’s going to lead to SQL Injection. You actually don’t have to use prepared statements if you don’t want to. However if you take this approach, you need to sanitize user input, validate user input, make sure that the input meets your standards, .etc.
- The snippet doesn’t have any error handling. If the visible number 1 doesn’t exist in the database table, your while loop will be in an infinite loop causing your code to have an exhaust error or maybe even crash the page.
Here’s a more secure approach. Assuming you are using PDO.
$visible = filter_var(1, FILTER_SANITIZE_NUMBER_INT, FILTER_FLAG_STRIP_HIGH); // Force this string to be a number because it should not be anything else.
$sql = "SELECT title,id FROM pages WHERE visible = :visible ORDER BY position";
$query = $con->prepare($sql);
$array = array(':visible' => $visible);
$query->execute($array);
if($query->rowCount()) {
while ($r = $query->fetch()) {
$ptitle = $r['title'];
$pid = filter_var($r['id'], FILTER_SANITIZE_NUMBER_INT, FILTER_FLAG_STRIP_HIGH); // Force this string to be a number because it should not be anything else.
print('PTitle: ' . $ptitle);
print('<br>PID: ' . $pid);
}
} else {
print('Nothing here to find.');
}
In this snippet, on the first line, we force the $visible variable to be an integer. This will cause any string that isn’t a numeric character to be blank triggering our very last few lines of code. Without being a numeric character, the result will output Nothing here to find.
Going down the lines. In the query string, we use :visible so that we can bind the variable later on. This will reduce our chances of being prone to SQL Injections. After that, we are just going to make the code a little neater so we don’t really need to have the whole "SELECT ...... " inside the ->prepare(). You can do that if you want to. It doesn’t really matter right now.
Next, we bind :visible to it’s appropriate variable. You can actually use bindValue if you want to. This is just an easier way of binding a bunch of variables together instead of having a bunch of lines that have bindValue.
Following that, we execute the array of values by appending the variable to the execute line.
Next, we check to see if the query returns a value, most likely a 1 or a 0. 0 being no results, 1 being a result or in my terms (true or false). If it returns a 0 (false), it’ll trigger our last lines of code. Resulting Nothing here to find. The equivalent in MySQLi_* is num_rows.
After that, we call for the while loop. Then lastly, we force any integer being output to be an integer otherwise, it’s not an integer at all. And finally, we output what PTitle and PID are.
You should also escape on output, sanitize on input.
If you are looking for a MySQLi_* version, here it is.
$visible = filter_var(1, FILTER_SANITIZE_NUMBER_INT, FILTER_FLAG_STRIP_HIGH); // Force this string to be a number because it should not be anything else.
$sql = "SELECT title,id FROM pages WHERE visible = ? ORDER BY position";
$query = $con->prepare($sql);
$query->bind_param('i', $visible); // Bind the param, i being integer
$query->execute(); // Execute the query
$query->store_result(); // Store the result for later checking
// Equivalent to PDO's rowCount()
if($query->num_rows) {
// Bind the result from the query string and append their own variables
$query->bind_result($ptitle, $pid);
while($query->fetch()) {
$pid = filter_var($pid, FILTER_SANITIZE_NUMBER_INT, FILTER_FLAG_STRIP_HIGH); // Force this string to be a number because it should not be anything else.
print('PTitle: ' . $ptitle);
print('<br>PID: ' . $pid);
}
} else {
print('Nothing here to find.');
}
Comments are added to avoid this post being uber long.
1 Like
Well let have a look whether I’m clear with this: My snippet i just do regular query fetch to database with PDO same security problem persist like the old one fetching without PDO . Now i understand every query to database has to be prepared or sanitized before actually selecting content.
→ Thanks for the clear and showcase answer . Helped me a lot for understanding this.
Technically, you don’t have to use prepared statements on all of your queries. You only need to use prepared statements on queries that have user input or the WHERE clause. Most likely, the WHERE clause will be from either $_GET or $_POST. Also, same with SET. Always sanitize it, validate it, .etc. or use prepared statements because SET is another user input clause.
Here is a perfectly fine query that doesn’t need to be converted to a prepared statement.
$sql = "SELECT sample_name FROM sample_table";
$query = $con->query($sql);
However, you should always be escaping on output so that any HTML entities in the table won’t be overwriting your page. So if you don’t escape let’s say.
<style type="text/css">
navbar {
display: none;
}
</style>
The user can actually overwrite the current navbar’s CSS properties. When you escape HTML entities, that whole code turns into
<style type="text/css">
navbar {
display: none;
}
</style>
And HTML will do the rest.
I made a custom function that i always call when have to deal with GET OT POST
function norm($var) {
$var = stripcslashes($var);
$var = htmlspecialchars($var);
$var = trim($var);
return $var; //cleaned var //
If you haven’t look into the setup file of the project all get vars if they are set will be cleaned from the function.That is one solution i came up with correct this if its totally wrong 
when it comes to POST vars I use this function again but in the place where the form is submitting or in the object logic .
$_GET['cat']= norm((isset($_GET['cat']) ? $_GET['cat'] : ''));
if isset it’s going to be sanitize other way it will be emty
$_GET[‘’];
They are both secure when used for their original purpose of testing whether a file has been tampered with. Neither is secure when used with passwords.
Megabytes are small when dealing with hashing algorithms. Hashing was originally intended for being able to tell if a file had been tampered with so a 10Gb video file is a quite acceptable input to a hashing algorithm.
As passwords use these same hashing algorithms with a few modifications, the limit on password lengths is going to be thousands of times larger than anyone will ever try to use and so no upper limit needs to be imposed.
If they want to paste a 10Gb video file as their password you should let them.