Jacques1

It's actually a much bigger problem than most people realize. And, no, I'm not talking about hotspots or the infamous “internet café” scenario. There are many other legitimate ways to access the internet, be it a VPN, Tor or a proxy. And while it may be reasonable to trust your ISP, it's hardly reasonable to blindly trust a Tor exit node or a small VPN provider. Network sniffing is a very real threat, not just a horror story for network administrators. Maybe it's less of a problem for you, but that doesn't mean everybody else is in the same situation. So I'd be careful with such statements.

Richard, why did you even ask when you appearently think that we have no idea what we're talking about? I took your idea very seriously and told you exactly what I think is wrong and what you should do instead. If you won't even read that, well, what's the whole point? Just do whatever you want to do and save everybody the time of writing useless replies. I'll happily explain why I think that TLS solves your problem, and I'll happily read your explanations for why you think that TLS is not the solution. But if you just reject any criticism and “close” the thread, there's really no basis for a discussion.

That's because you have output buffering enabled on your localhost. Either way, the structure is wrong.

Note that it's not enough to install some new Wordpress version and restore a backup. There's obviously something wrong with the server configuration itself, because no application should ever be allowed to create PHP scripts. So the first step is to fix your server (file permissions, insecure FTP accounts etc.). If you don't do that, you'll have the same problem again when the next attack technique comes out. Well, of course. There could be a new admin account, there could be embedded code which gets executed when it's loaded into the application, anything. You need to actually check the data before you restore it.

You do not revert the file. If somebody was able to create a backdoor, there's something very, very wrong with your system, and nobody knows what else they've done. There may be dozens of other backdoors. You need to take your site offline, fix your server configuration (preferrably after a complete “factory reset”) and then carefully restore your website from a non-infected backup. Otherwise you're just gambling: Maybe you've stopped the attack, maybe not.

Never invent your own security protocol. As a layman (which we all are), your chance of success is exactly zero. And even if you were a security expert, designing protocols is a matter of many years and extensive peer reviews. It's not something you do on your own in 5 minutes. The solution to your problem is TLS/SSL. This reliably prevents people from sniffing or manipulating the network traffic. You find it too heavy for your purposes? Well, there's a reason why it's heavy. If there was a shortcut, trust me, we'd already use it. So for an actual website, any custom protocol is out of the question. If you're doing this purely for educational purposes, the whole idea is still heavily flawed: First of all, it's not even clear what you're trying to protect. The MD5 hash is the password to your application, which means you still transmit it as plaintext. Anybody who intercepts the hash can log in as that user. The only theoretical benefit is that the attacker doesn't immediately see the original input, so if the client reuses the same password on a different site (which of course you should never do), then the account on that other site may survive a bit longer. But for your own site, there's absolutely no benefit whatsoever. Then of course there's a conceptual error: As you've already realized, anybody who is able to manipulate the network traffic might as well remove all the fancy JavaScript code and grab the original user input. There's no solution to this. You simply cannot create trust within an entirely untrusted environment. You need something to start with. In the case of TLS, that's what certificates are for. So at best, you get protection against passive attackers which only listen and never manipulate the traffic. But since we cannot choose our attackers, this isn't very useful. Last but not least, JavaScript is not always available. For example, many people turn it off for security reasons. What now? Does it mean the plaintext password gets sent around? Does the entire site cease to function? Note that those are not suggestions for fixing the protocol. It cannot be fixed. The only option for a production website is TLS.

There's no need to use curly braces when the branches only consist of single statements (it does improve readability, though). Another option is to use the ternary operator which yields one of two values depending on a condition: echo $account_required ? '<small>' . __( '(optional)', 'wp-jm' ) . '</small>' : '(required)'; Whether you prefer this or the more verbose if statement is up to you.

So how does your syntax look like? This isn't any special syntax, it's a normal function which happens to be called “__”. And if you have a look at the Workpress manual, you'll see that it's used for localization.

It's destroyed after the variable has been overwritten with the new result object (hence the problem above).

The results are automatically freed upon destruction of the object. And if your variable is the only reference to the object, then, yes, overwriting it will eventually destroy the object. Note, however, that the old object still exists when you create your new query. That's a problem if you use an unbuffered result and haven't fetched all rows: $stmt = $mysqli->query('SELECT 1', MYSQLI_USE_RESULT); $stmt = $mysqli->query('SELECT 1'); This gives you an “Command out of sync” error. If you explicitly free the result, there's no such problem: $stmt = $mysqli->query('SELECT 1', MYSQLI_USE_RESULT); $stmt->free(); $stmt = $mysqli->query('SELECT 1');

PDO always uses prepared statements internally, even if we call the query() method. So it's not like we had a choice. See the source code: PDO::query() calls the mysql_handle_preparer() C function. And this function creates a prepared statement.

So how does your code without the ON DUPLICATE clause look like? We cannot debug code which isn't there.

This has nothing to do with PDO, it's the same thing with MySQLi. In fact, you'd have to write twice as much code for MySQLi.

I think this is a misunderstanding. If you put the table name into an SQL string, you'll get a syntax error: SELECT * FROM 'this_is_invalid'; Identifiers are not strings, so you cannot use PDO::quote(). Like I said, there's no easy solution. In fact, it's impossible to prevent SQL injections with the function itself, because you want it to accept complete SQL clauses (like the WHERE clause). Your only chance is to carefully validate all input before you pass it to the function. You should also consider using professional database frameworks like Doctrine. They don't entirely solve the problem either, but they're more mature and easier to control.

You mean for identifiers? This doesn't work. mysqli_real_escape_string() only works within quoted string values. It only prevents users from “breaking out” of the quoted expression. If there are no quotes, the whole function is useless. By the way, the PDO equivalent of mysqli_real_escape_string() is PDO::quote().

OK. But you have to admit that a demonstration which breaks a bcrypt hash in 7 seconds is slightly misleading. If an uninformed user reads this, they may very well conclude that bcrypt is useless and not worth the trouble. You do bring up a valid point, though: Password strength is important, no matter what algorithm you use. So it was just a clarification. Don't forget that the Internet is full of wrong information, and many programmers don't know anything about hashing. If they read about it on this forum, I think it's important that they don't get any wrong impressions. They shouldn't leave with the thought: “A guru at PHP Freaks has broken bcrypt in 7 seconds, so I think I'll stick to MD5 for now.”

You indeed cannot pass identifiers like column or table names to a prepared statements, and that's indeed a problem. There are basically two solutions: Either you avoid this scenario altogether, or you use a whitelist of acceptable identifiers. Theoretically, you might also try a kind of generic validation so that you can use entirely dynamic queries (which seems to be your goal). But this is risky and fragile, so I'd avoid it at all cost. Even the big database frameworks shy away from this.

I find this comment rather misleading. Of course bcrypt does not magically block brute-force attacks or turn bad passwords into good ones. Nobody made this claim. The point is that bcrypt massively increases the resources needed for an attack. If we use MD5, then an attacker can try billions of hashes per second on stock hardware. If we use bcrypt, they're down to maybe one password per second (depending on the cost factor). This means if you choose a decent(!) password, then bcrypt makes a brute-force attack infeasible. Yes, you do need a good password. If you use “phpfreaks”, then no algorithm on this planet can help you. Secondly, the goal of hash algorithms is to protect the passwords against offline attacks where the attacker has obtained the raw data and can attack it directly. Your example shows an online attack against the log-in form of the application. That's not what bcrypt is about. To protect yourself against online attacks, you don't need hashes at all. It's enough to artificially slow down or limit the log-in attempts.

You need to actually submit the form. There are no POST parameters before that. If the notices bother you, then of course you might add all kinds of checks for the request method and the individual parameters. But the checks weren't there before, and they are not necessary to get the application up again.

Well, this is not PHP syntax: $query = ("SELECT * FROM user WHERE `username` = '$username'", What's the comma doing there? Why is there an opening parenthesis, and where's the closing parenthesis? Please check the code line by line to make sure this is actually what you want. You should also install an IDE like Netbeans so that you're immediately informed about syntax errors. This way we don't have to play human debugger.

I don't see the error. But $_SESSION['firstname'] should read $firstname.

For a quick hack, you just need to replace the variables which PHP used to define automatically due to the register_globals “feature”. If you're not using an IDE like Netbeans, install it now. This will mark all undefined variables. For each variable, you have to find out where it's supposed to come from and then replace the variable accordingly. Take $employee as an example: This obviously comes from the POST parameters after the form has been submitted. So you replace $employee with $_POST['employee']. You cannot learn anything from this code – except how not to program. If you want to learn PHP, check out modern applications, not shitty amateur code from the late 90s.

You're using the name attribute, but I'm fairly sure you want the value attribute. Besides that, the code layout is rather strange. Why is the entire form in the while loop of the user query? Why is there a loop at all? Shouldn't there only be one user with that name? Then you only need a single fetch_array() call.

It makes little sense to focus on one particular XSS vulnerability (the PHP_SELF stuff) when the entire script is full of them. The question is what exactly you're supposed to do. Is this just a quick hack to get the application up again, no matter how rotten it is on the inside? Or are you supposed to actually repair the code and maybe even modernize it? Those are two entirely different tasks. The former will be relatively quick, but of course all problems are still there and may pop up at any occasion. The latter will take much, much more time, because you'll have to rewrite large parts of the code. If you're not sure, ask the people in charge.

The member Strider64 has a decent tutorial on his website.