Hot answers tagged

18

There's a common credo believed when evaluating the security of software: If it's homemade, it's unlikely secure. Sec.SE has a Q&A about homemade algorithms, which is somewhat germane to your circumstance. I suggest you look into preparing your queries, as that would be your best action to take in this situation. It's essentially what you're trying to ...


17

I don't know that my code is vulnerable to SQL injection. Yes, it is. You should never put any variables directly into SQL statements. Even if you think that the variables may possibly be safe, it's just really bad practice, and you will mess it up sooner or later. In your case, an attacker could use the profile fields, which would very likely lead to SQL ...


14

Note: I don't think this is what you want to hear. But I have to write "the truth". This is my personal opinions about your code. Give me parameterized SQL or give me death! tell me if there are any bugs or improvements to be made. Don't suggest prepared statements. I'm sorry, but those two requests really don't fit together in the same Code Review ...


14

Yes, there are several things I can see with your implementation, from most critical to less critical: SQL Injection - You has it! Since you're already using mysqli, learn about prepared statements with mysqli and apply them. You are very vulnerable to Little Bobby Tables. Your password hashing is not secure - sha256 is insecure for password hashing in on ...


11

Security This code is not secure. SQL Injection You are trusting the user input completely (except in the login function where you do use mysqli_real_escape_string, which is not secure enough). Use prepared statements instead. You should also read up on SQL injections in general. Other This is all minor in comparison to the SQL injection. don't ...


11

Correctness Also, all of the code works. No, it doesn't, so let's start with that. $username = mysqli_real_escape_string($conn, $_POST["username"]); ... $result = mysqli_query($conn ,"SELECT * FROM users WHERE username='" . $username . "'"); if(mysqli_num_rows($result) > 0) ...


10

Alright, here we go. There's quite a bit so grab a drink, go to the bathroom, etc... Unsetting Variables I don't think unsetting the $dbPass was necessary as it only exists in the local scope of the constructor method, so a var_dump() shouldn't reveal it unless you have a var_dump() in the actual constructor. But I've been known to be wrong. Would be ...


10

Give me parameterized SQL or give me death! Seriously. Use prepared statements! Don't use mysqli_query as that makes your code vulnerable to SQL Injection. By using prepared statements, you won't need the mysqli_real_escape_string calls anymore. This code looks nasty: $result1 = mysqli_query($con, $sql); if (!$result1 || mysqli_query($con, $sql)) { ...


10

Right now, you're using the same salt for every user. This pretty much defeats the purpose of using salt in the first place. The point of using salt is to ensure that even if two users choose the same password, the hashed value will still be unique for each. So, to be useful, you compute the salt as a random string for each user, then store that random ...


10

Security Focusing on only the password hashing for a second, let's talk security. public function hash($string) { $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-"; $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+"; $sting = md5(md5($fSalt) . md5($string) . md5($sSalt)); for($i=0; $i < 3; $i++) { $string = md5($string); $...


10

A few things here: Comments: Your comments are all compeletely useless.. nothing personal and no offense. That is a beginner-mistake. You don't need to comment everything. Comments should explain the why and not the how or the what. Exceptions: License comments Documentation (python's docstring, java's javadoc, c#'s summary, ...) Naming: $con = //... $...


10

SQL Injection-wise, this is completely safe. You don't run any risk of SQL Injection. However, some parts of your code is not optimal: mysqli_stmt_execute($stmt); mysqli_stmt_bind_result($stmt, $user['username']); mysqli_stmt_execute($stmt); Why are you executing the statement twice? Now, imagine that you hade your if-else switched so that you wanted to ...


10

Security I'm not all that comfortable with echoing unsanitized user input: echo '<form action=' . $_SERVER['REQUEST_URI'] . ' method = "post">';. I did not find any way to exploit it though. I asked a question on security.SE, maybe they can find a way. I would probably use SCRIPT_NAME instead. you should regenerate the session id via ...


9

Why are you starting a new database connection for each class? Pass in a database connection object to the constructor: public function __construct(mysqli $dbConnection) { This allows your class to work without worrying about actually doing the connection. The class's job is to handle users, not handle database connections! You not only said LIMIT 1 in ...


9

Madara Uchiha sums up some problems. Here are some more: Too many if conditions A methods containing multiple if's smells. It makes testing harder since there are more edge cases. It also creates a mess where none should be. For instance your getError method. A better approach would be: function getError() { return $this->error; } And if you ...


8

This looks pretty good. I'm glad to see the improvement. You inexplicably call $connect->bind_params() using variables that have not yet been set. The main issue I see now is $QuizAdministrationid = $maxId + 1, which is prone to a race condition. If two users submit the form simultaneously, it's possible that both pages could see the same result from ...


8

I want to focus principally on this section of your code: $mascow_sub_area = array(//'Aviamotornaya','Avtozavodskaya','Akademicheskaya','Aleksandrovskiy_Sad', 'Alekseevskaya', 'Alma-Atinskaya','Altufevo','Annino','Arbatskaya','Aeroport','Babushkinskaya','Bagrationovskaya','Barrikadnaya','Baumanskaya','Begovaya','Belorusskaya','Belyaevo','Bibirevo','...


8

At a quick look: Your code is vulnerable to SQL Injection: assume the user wants to hurt you, so always parse superglobals $_GET and $_POST https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php To check if variable have values: // good practice if (isset($userName, $userPass)) // bad practice if ($userName && $userPass ) ...


8

First up, that's a pretty good effort for a couple days of experience, you have obviously done some reading yourself. To be honest it is, what I call spaghetti code, echo html statements in between logic. While it works fine (and is how most people learn), it becomes a pain to maintain in the long term. Rather then straighten out the spaghetti, I have ...


8

Your code will work, of course, but you go to a lot of trouble to grind the result set into an array when that capability is already available. Mysqli would provide the fetch_all() function if you had the mysqlnd drivers available; fortunately, PDO has provided it since PHP 5.1. You will need to create a PDO connection with $dsn = "mysql:host=" . $hostname ...


7

XSS attacks affect the whole web, regardless of the stack you're using, you always have to be careful. Based on the redirect url you provided and the queries you execute, I'm assuming you're building an on-line address book. If a client were to fill in the form, saying his name were: <script> alert(document.cookie);</script>">Gotcha Your ...


7

Brevity of code seems an odd point of focus, so I plan on ignoring that. I do, however, have quite a few suggestions, and, convieniently for you, those will probably shorten the code. Don't assume user provided variables are actually set. For example, don't access $_POST['url'] directly. Either cover it in a isset guard like you've done other places, or use ...


7

I've never played with mysql, so this may be completely wrong, but if I get it right the WHILE loop would be the equivalent of a T-SQL CURSOR, which is inherently slow. You're essentially looping on payment_id, incrementing at each iteration - this assumes the ID's are contiguous, which isn't a safe assumption to make with data: if records were deleted, you ...


7

Similar to Mat's answer, in stage 2, you are calculating a random value for each customer, and processing the customers one-at-a-time. It would be faster to process them all together, but, the rand() becomes hard to do because it changes value each time you call it, and you need to change the 'obvious' odds of things as you go. Your procedure (with a test ...


7

You are vulnerable to SQL Injections. Even when you are using Prepared statements, doing It wrong will kill you. $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?"; This makes you exposed to SQL injections because your function is not private. You should not allow "user" access to "critical" functions. make that function private and you should ...


7

Spaces are free! It's a good idea to space out your code for readability. @Vogel612 did this in his answer (perhaps subconsciously), but it doesn't hurt to say it explicitly. SELECT multiple columns without *. You mentioned using SELECT *. This is typically considered poor design; it's preferable to explicitly list the columns you want SELECTed, so you ...


7

This is going to be harsh, continue reading at your own risk! A few points I want to make here, that are definitely missing for this to be anywhere near production: Security Security in this class is nonexistant. You get a query from somewhere and run it. no checks no nothing. While in itself this could possibly even work, you definitely shouldn't do this....


7

Is this method safe? Well, assuming ord is safe, it should be safe, because you will only ever have strings containing numbers in your query. But what is also safe are prepared statements, and they are well tested over the years. And prepared statements don't have the downsides of your solution: untested in practice bad performance time wise (...


7

- How may I make it more flexible? Depend on abstractions not on concretions (SOLI D ) __construct(Request $request, Response $response, Mapper $mapper) Each concrete (Request, Response, Mapper) could be replaced with a Interface making the code more flexible for changes in the dependency and not avoiding tight coupling. - Is my code efficient? How may ...


7

Security SQL Injection It's probably[*] secure, but it's not the right way to do it, see for example here why prepared statements are better than escaping. Sooner or later, you will mess up when escaping. Prepared statements are not difficult to use, and result in code that is more secure and more readable, there is really no good reason not to use them. ...


Only top voted, non community-wiki answers of a minimum length are eligible