PHP Developers Network

Okay, story time! So, I was on a chat I frequent, and some guy came on, pestered the admin over some small amount of HTML code that he didn't likem, and started attacking us. He saw this code I posted, and then made fun of me. It's clearb that this guy was a troll, but damn what he said was really burned in my mind. He particularly focused on the "genSalt" function, and said that I can't even indent properly. Okay, now to relevant info (some of the stuff he said was rediculous), he said that the function was stupid and wrong, because it meant that Apache has access to the urandom device, which was supposedly a huge security flaw. Honestly, I feel like ranting, if you want the story, please ask. He also said that I should've used the random_bytes() function instead of the function I used. He said the whole thing was terrible, but made no other specifications than that.

EDIT:
I have updated the code, how is it now? I kind of understand exceptions much better, but I don't see how it's better than just using error codes. It seems much simpler and easier, not to mention they seem to work better. I honestly can't handle PDO and its object orientation. I'm not going to, use anything other than mysql anyways. I like the idea of prepared statements, but I don't see where in my code they'd work better than escape strings. I may use them in the future though. What do you guys think? The particular functions I'm concerned about it chkUser and loginUser.

Well, that's completely wrong. I hope he's a troll because otherwise it would mean he's stupid. Could be both.

General comments:

- Never connect to the database as root. Even with a secure password. Always create a user dedicated to whatever purpose you need, and only the minimum privileges necessary on only the databases required. So basically SELECT, INSERT, and UPDATE for all tables, then DELETE for your thing but restricted to the users table.
- Avoid deleting stuff from the database. "Deactivating" or "hiding" is almost always better, partly for record-keeping and partly for security.
- Don't use return codes for functions. If I call rmUser and it returns 3, what does that mean? I have to hunt around for an answer. Don't simulate true/false with 1/0 either - it's unnecessarily confusing. (a) Throw exceptions for each circumstance, like DatabaseConnectionException (extends DatabaseException) and DatabaseQueryFailureException (ditto), or (b) return true or false and log the error somewhere accessible. Exceptions are better.
- Avoid abbreviations and acronyms unless the full meaning is well known in the industry (eg, within your company or in the PHP development world) and it legitimately saves you time and effort. "rm" is well known but isn't saving you much over "delete", and "auth" is well known and does save you from having to type "authenticate" every time, but on the other hand "chk" really should be spelled out. Readability is far, far more important than using fewer strokes on a keyboard.
- Do not expose MySQL error messages. They can contain information about your queries and database structure that a malicious user could use to attack your application. Display a generic message and log the original message someplace safe. Ditto for other situations where error messages can contain sensitive information.
- There's a good chance you're not familiar with prepared statements. Learn about them and what they're for. Some people insist on using prepared statements for everything because it virtually eliminates the possibility of SQL injection, but a knowledgeable developer can use mysqli_real_escape_string just fine.
- Consolidate your database connection code into one place. If you find yourself copy/pasting code into more than a couple locations, or creating *shudder* "snippets", then it needs to be refactored.
- Learn how to do password hashing correctly. (1) Do not generate your own salts. (2) Don't store the salt separately from the password because there's no need to. (3) The final "password" hash will contain both the hashing algorithm, the salt, and the hashed value. This is fine. Don't touch it and just store it as-is. It won't contain any binary data either so bin<->hex conversions are unnecessary - and actually detrimental. (4) When verifying a password, get the hash value from the database and use password_verify() to validate it; the only time you ever call password_hash() is to generate the password hash you're going to store.

Specific comments on your code:

- User functions: Doing a "SELECT * FROM users" will query for and return every single record in the table. Using PHP code to sift through that looking for one record is horrifically inefficient and wasteful. God kills a puppy and kitten every time someone does that. Instead, construct a specific SELECT query with the conditions already inside it. For example, after you've fixed the password stuff such that there's only the one password hash column in the table, then

Remember that you're using PHP to check the password, so there shouldn't be anything in the query about a hash you've calculated (not the least bit because there's no way for you to do so at that point).
- logUser: Sounds like it's logging something about a user. This problem goes back to avoiding abbreviations. Call it the full "logInUser".
- logUser: If the session is empty but already started then session_start() will raise a warning. Putting aside the fact that there are better methods for checking if the session has started than by looking in $_SESSION, starting the session should be done at a global level that is not directly tied to some specific action. There's no problem with always starting a session on every page and simply not using it if you don't need its information.
- genSalt: This function should not exist at all.
- chkPermit: Why are these level/merit things stored as numbers in files? This sounds really bad. It should very likely be stored in the database instead.

mysqli vs. PDO? Either. Both have advantages and disadvantages over the other. As most people are never going to change database systems, it comes down to which API you prefer.

Wow, that's a lot of valuable information. I guess I have a lot of re-writing to do. Also, the "merits" aren't stored in a file, but in the db. The rw file that it pulls the info from is the merits needed to do a thing. I know it's silly, it's kind of for fun. I was trying to simulate Unix permission thingies, I guess.

About return codes for functions, why not? The intended thing is that I write a documentation file that has the return codes at the very beginning, and it makes it much easier for a dev to write a file that includes mine. Or even if I provide a table in the comments, wouldn't that also work? I was kind of following the "Rule of Repair: Repair what you can — but when you must fail, fail noisily and as soon as possible." As you can see, I'm more of a C/C++ guy. I'm also obsessed with Unix philosophy.

What are the harmful things an SQL error can contain?

Haha I'm sorry about the "snippets" thing. This is the only file that'll contain SQL at all.

How do I separate the salt that's stored with the hash to verify the password, assuming I have to do it myself.?

A friend recommended PDO over mysqli, so I guess I might change it to that and rewrite my SQL to use prepared statements.


Also just a snippet of what the troll said "Apache2 is the most pwned bloatware of them all. You really are a skiddy."
"You can't even indent the code properly."

Please tell me I'm not a skiddy, that's the worst insult in all of software history. Seriously, I've been called a lot of horrible things, but that hurts the most.

Ah, a C guy. That explains so much.

Having programming experience is great, but don't take that knowledge and try to fit it into the PHP paradigm. Square peg, round hole. Learning PHP is about more than just syntax and function names - you have to learn how to write PHP code too. How the language works, what it can and cannot do, and the best way(s) it provides to accomplish a goal. The knowledge you should be carrying over is about code theory, not code implementation.

C doesn't really have any good mechanism for indicating various forms of failure. No exceptions, no classes. You call a function, it returns a value (and maybe has a few pointers for output parameters). That's all you have to work with. Actually, to be more precise, that's the C paradigm: you could use structs, or arrays, or another structure, but it cuts into efficiency and performance. And C is big on efficiency.

PHP, being a higher-level language, is not like that. Efficiency is good, of course, but the most important aspect of PHP code is its maintainability; over-optimization will hurt you in the long run. It has exceptions you can use to interrupt program flow for "exceptional" circumstances, and it has classes where you can bundle together data and behavior into a single cohesive unit. The PHP paradigm is about writing descriptive and hopefully self-documenting code.

As for the Unix philosophy, that's cool and all but remember that it's not programming philosophy. There are certain beneficial aspects like its separation of concerns, but not everything will be helpful with PHP.

Here's what I suggest: learn about the nature of PHP - like, go through the reference section of the manual. Learn about classes, interfaces, extensions, INI settings, exceptions, databases, resources... all the tools available to you as a PHP developer. Then consider what those tools can do to help you accomplish a goal. You need to switch your mentality from "how would I write this in C and translate it into PHP" to "how would I write this natively in PHP".


That's the C mentality. Set that aside.

What does PHP provide that is relevant here? The best answer is booleans and exceptions. If your function should return a simple yes or no for success then it can return true/false; if your function should generally work but sometimes there may be a problem then it can throw an exception. Let's set aside the "best way" to use exceptions and just stick with the base Exception class.

Take rmUser, which we'll now call "deleteUser" (remember: descriptive and self-documenting). For the most part it will delete a user successfully, but there are multiple circumstances where it might not succeed. As such the exception strategy is more appropriate than true/false.

In pseudocode:

Since we're using exceptions, the poorly-named "singleFunctionToGetDatabaseConnection" (too descriptive) could throw its own exception if it fails to connect - that's another example of "should generally work" code.

Exceptions require a bit more code to work their best: try/catch blocks.

Without a try/catch any exception that is thrown will cause the script to abort; to borrow C terminology, exceptions are like longjumps to the closest try/catch block, and no block in the call stack means a crash.

Does that make sense?



If there is a syntax error in a MySQL query, like

then the error message will look like

That is, it contains a portion of the query itself starting at the location of the syntax error. For this query it's not much of a problem, but if you put password hashes or secret keys or other information that needs to be kept from the user, they may be able to see it in the error message.


It's a pet peeve. Many people have a "snippets" or "shared code" file with stuff they've written and rewritten over the years, available so they can reuse it without having to write it yet again. Personally, I think it promotes copy and paste programming, which is something I really hate.


You don't. It's one of the things that PHP does almost uniquely from every other language out there: hashes contain all the information necessary.

Consider a simple MD5 hash on a salted password. The algorithm itself will spit out a 128-bit string, which most people (including PHP) represent as a 32-hex-character string. That's the whole hash. To replicate that hash you need to know the original input and the salt... and the algorithm. Normally you would store the hash and salt separately, and simply program MD5 into your application, but PHP can combine them all together into one string in the form

or more generically


Here, MD5 is represented by the identifier "1", the specific information is a salt is 12 characters long (other algorithms need more than just a salt), and the hash is 32 characters long.
(Why 12 for the salt? I don't know. But if you want to use the built-in mechanisms for this unified string thing then the salt needs to be that long. Which is fine.)

The most obvious benefit with this methods is that there is only one piece of data to manage, but there's another: you can use this exact string to duplicate a hash as well, as the string itself functions as a "salt" with the $1$ indicating the algorithm and only the first however-many characters (here, 12) being used for the actual salt to the algorithm.

What I've described is specific to crypt and password_hash. You can always "roll your own" using other functions.


Good.


He's a troll. Don't feed the troll.

You're not a skiddy. Your issue is not that you don't know what you're doing, but that you know what you're doing in one language and trying to apply it to another. On top of that you're still learning PHP. Obviously you're not going to be best coder around - what matters is how you proceed from here.

Thank you, that's very helpful. I think that answers all my questions.

EDIT:
I have updated the code, how is it now? I kind of understand exceptions much better, but I don't see how it's better than just using error codes. It seems much simpler and easier, not to mention they seem to work better. I honestly can't handle PDO and its object orientation. I'm not going to, use anything other than mysql anyways. I like the idea of prepared statements, but I don't see where in my code they'd work better than escape strings. I may use them in the future though. What do you guys think? The particular functions I'm concerned about it chkUser and loginUser. Can this be done more efficiently?