Jump to content

Jacques1

Members
  • Posts

    4,207
  • Joined

  • Last visited

  • Days Won

    209

Everything posted by Jacques1

  1. Personally, I would put the form and the processing code into the same script. This is by far the easiest option and avoids “helper scripts” like this tryagain.php altogether. If that isn't an option, I'd store the message in the session or a cookie. When you use URL parameters, the message may appear unexpectedly (reloads, bookmarks, URL sharing, ...).
  2. Another way to separate the PHP code from the SQL query is to use clear formatting: $addDiscussion = $db_con->prepare(' INSERT INTO discussion (user_id, motion_id, discussion_text) VALUES (:userid, :motionid, :text) '); But really the most important step is configure the error handling. No database API just silently refuses to execute a query, there's always a detailed error message available. PHP usually expects you to fetch it manually (which is unrealistic), but when you enable exceptions, you'll get it automatically.
  3. You should actually clean up your code right now, because this is far too complicated. The entire procedure can be done in one run: You iterate over each character, get its numeric value, increase the value and then write the new character back into a string. As pseudo code: encrypt_caesar(plaintext, offset): ciphertext = "" for each char in plaintext: // convert character to number char_number = char_to_numeric(char) // add offset to number; warning: the result can get bigger than 26, so you need to wrap it (using the modulo operation) char_number = char_number + offset // convert number back to character and append that character to the ciphertext chipertext = ciphertext + numeric_to_char(char_number) return ciphertext See ord() and chr() for conversions between characters and numbers.
  4. Once again: Read the previous posts before you reply. It's great that you want to help, but it's not helpful to post false information which contradicts the facts that have already been established. No, the code is not OK. As I've already said, the query is syntactically wrong. Looking somewhere else isn't going to fix that.
  5. ... or argument unpacking $stmt = $databaseConnection->prepare('SELECT x, y from test'); $stmt->execute(); $number_of_cols = 2; $results = array_fill(0, $number_of_cols, null); $stmt->bind_result(...$results); $stmt->fetch(); Not much better, especially since the resulting array is numerically indexed.
  6. The proper way is to replace mysqli with PDO. If that's not an option, get_result() is OK as a workaround. It does require the MySQL native driver, though (which isn't available on all systems). It's theoretically possible with call_user_func_array() and an array of references, but this is really ugly.
  7. There's a missing parenthesis in the query, and PDO would have told you that if you had enabled exceptions as I told you to.
  8. Can you be a bit more specific? We can neither see your screen nor read your mind. What exactly happens? An error? A blank page? Something else? If it's a blank page, enable PDO exceptions, turn your error reporting all the way up and make PHP display the messages on the screen (assuming this is your development machine, not the live server).
  9. What you'll (hopefully) realize soon is that a lot of the “smartass answers” are far more valuable than the literal answers. When somebody asks you about the best way to drive a nail into the wall with an old shoe, you can tell them to use the sole. Or you can give them a hammer. Do you think the second option would make you an asshole who doesn't answer questions and wants everybody to become a professional craftsman? Or isn't it simply the right kind of help? The proposed alternative to your code is this: $userStmt = $databaseConnection->prepare('SELECT * FROM users WHERE username = :username'); $userStmt->execute(['username' => $search]); $user = $userStmt->fetch(); if (!$user) { echo 'User not found.<br><a href="tryagain.php">Go back</a>'; // not perfect, but definitely better than a page refresh exit; } This simultaneously solves your problem prevents your server from being compromised makes your application survive the next PHP update replaces the hacks with decent code And it's not exactly rocket science either. Sure, you're new to PHP, but you don't seem to be stupid. So why not spend a few minutes on learning something new instead of wasting days on ancient stuff that you'll sooner or later have to rewrite anyway?
  10. You have an entire collection of errors. First, you're obviously accessing the script without a v parameter in the URL, because the value of $video is null. You have no check for that case either; you just assume that the parameter is always present (which it isn't, as you can see). Then you're using emulated prepared statements (which is the default) instead of real ones. This means the parameters are literally inserted into the query string instead of getting sent to the database. Since your parameter is null, you end up with a LIMIT of NULL -- which is syntactically wrong. The query itself is also fishy. Why are you doing an offset search when you want to look up a particular ID? This will give you nonsense results. You don't even have an ORDER BY clause, so the offset could start absolutely anywhere. Long story short: Validate the user input and handle missing parameters; don't just assume that you get what you expect Disable statement emulation Fix the query. You probably want a WHERE clause.
  11. Guys, please read the question. The OP knows rowCount(). He mentioned it right in the first post. What he actually wanted was an existence check, and he already got that (see replies #3 and #5). So this problem is solved. You can stop replying now.
  12. The approach to security is also somewhat naive. Sessions stored in a database are much more vulnerable than file-based sessions due to the prevalence of SQL injection attacks. With your current approach, an attacker can obtain all session IDs if they find a vulnerable SELECT query manipulate the session (e. g. make themselves an admin) or perform an object injection attack through the serialized data if they find a vulnerable UPDATE or INSERT query That's pretty bad. The sessions should definitely be protected with a cryptographic layer: Only store hashes of the IDs instead of the plaintext IDs. SHA-256 is enough. This prevents the IDs from being read and simultaneously fixes the length problem. Protect the integrity of the serialized data with a message authentication code. For example: $session_mac = hmac('sha256', "$hashed_id:$access_time:$data" , $session_auth_key); This will detect any manipulations.
  13. A CSRF attack uses your admin session to make requests. When you're logged in and visit any other site, then that site can send arbitrary requests to your application on your behalf. It's generally a bad idea to just assume that a vulnerability cannot be exploited. An experienced attacker knows a lot more about vulnerabilities than you do and will use methods you never even thought about. So whenever you encounter a problem, fix it. Don't make up excuses. The function generates its IDs by shuffling the 7 digits 0...6. There are theoretically 5040 possible IDs, but collisions will occur much, much earlier due to the birthday paradox. You can actually try it out: <?php function randomGen($min, $max, $quantity) { $numbers = range($min, $max); shuffle($numbers); return array_slice($numbers, 0, $quantity); } $generated_ids = []; for ($i = 0; $i < 200; $i++) { $id = implode(randomGen(0, 6,6)); // check if ID already exists, otherwise store it if (isset($generated_ids[$id])) { echo 'Duplicate ID '.$id.' after '.$i.' attempts'; exit; } else { $generated_ids[$id] = true; } } A much better way to get IDs is to use auto-incremented fields in the database table. If you absolutely need random IDs, you must use a proper random number generator (not array_shuffle) and generate very long strings. For example: <?php function generate_id() { // the random_bytes() function is only available in PHP 7, but there are implementations for PHP 5 as well: https://github.com/paragonie/random_compat return bin2hex(random_bytes(16)); } echo generate_id(); This requests 16 random bytes from the operating system and encodes them as 32 hexadecimal characters, which makes collisions almost impossible.
  14. A prepared statement doesn't have a fetch_array() method. Read the manual on how prepared statements work. Actually, you don't need this at all, because you have no external input. The entire method can be boiled down to function listMerchant() { return $this->mysqli->query('SELECT col1, col2, ... FROM core_merchant')->fetch_all(MYSQLI_BOTH); }
  15. It means there's no such thing as $this->mysqli. Either that attribute doesn't even exist, or you've failed to assign a connection to it.
  16. Congratulations, you now have three SQL injection vulnerabilities in one statement and a particularly juicy CSRF vulnerability on top. This can be used to steal any data (passwords/hashes, personal e-mail addresses, ...) and take over any account. That weird ID generator also means there's a 50% collision chance after just 84 entries.
  17. Apache has built-in support for password authentication. When you use HTTPS, the bcrypt hash algorithm and a decent password, this is reasonably secure. Make sure to prevent cross-site request forgery attacks with Double Submit Cookies. If you have the PDO database extension, read this great tutorial. For mysqli, read that one.
  18. What's the point of the nested loop? Just iterate over the products: <?php $products = []; foreach ($obj['products'] as $product) { $products[] = ['id' => $product['id'], 'name' => $product['name']]; } In any case, this will take some time, and your script must be able to handle that. Increase the execution limit and run this with a cron job, not via a browser request.
  19. No, the article is simply bad advice. Yes. Read-only files should have 0740 permissions, writable files 0760. Execute permissions are only relevant for binaries and shell scripts with a shebang line. A web application generally doesn't have that.
  20. The code has severe problems and is far from production-ready, let alone ready for big applications. First, there's no synchronisation whatsoever, which means concurrent requests accessing the same session will trample each other. You can actually see that: <?php // include your classes $session = new Session(); if (isset($_GET['inc'])) { $_SESSION['counter']++; exit; } elseif (isset($_GET['show'])) { echo $_SESSION['counter']; exit; } else { $_SESSION['counter'] = 0; } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <title>Title</title> <script src="https://code.jquery.com/jquery-3.1.1.min.js"></script> </head> <body> <script> for (let i = 0; i < 100; i++) { $.get('?inc'); } $.get('?show', function (counter) {alert("Final counter value: " + counter)}); </script>> </body> </html> This is a simple Ajax counter which makes 100 requests to the script, and each request increments a counter in the session. You would expect a final result of 100, but you'll get less than that (I had 60), because the session operations overwrite each other, and so some are completely lost. In an actual application, this is a nightmare. Not only can the error screw up important data. It will also occur randomly. Good luck to anybody trying to debug that. Then your ID field is too short. A session ID may be up to 256 character long, so storing it in a VARCHAR(32) can lead to truncation. The database class doesn't even make sense. For some strange reason, it only supports a single database server (because the credentials are hard-coded) and one query/statement at a time. How is that supposed for work for large applications? And what's the whole point of the class? It doesn't add any new features to PDO and is extremely confusing (for example, query() doesn't actually send a query). Long story short: Stop marketing this as a solution for big applications. It's an experiment at best. You must synchronize session operations. This can be done with advisory locks. Fix the length of the ID field. Throw away the database class and use plain PDO.
  21. Like I said: The webserver must not own any application directory or file, neither inside nor outside of the document root. If somebody told you otherwise, they're wrong. The owner can effectively do anything: They can give themselves all permissions and then read, manipulate or delete their files and directories. When you give that power to the webserver, you also give it to any attacker who manages to find the right vulnerability in your application. This is not just a theoretical risk. You'll find many threads in this forum (mostly Wordpress-related) where scripts have been infected with malware due to an application vulnerability and write permissions. The owner must always be a separate administrative account. No. As explained above, read permissions on a directory means that the directory content can be listed. This is necessary if, for example, you want to iterate over all files in the upload directory. If you don't need that, don't use it. Yes.
  22. I have no idea what you're talking about. “Offset”? “Header argument”? Show us real code and hard facts (debug outputs, screenshots, ...) instead of telling some vague story. We're programmers, not mind readers.
  23. First, every directory and file of the application should be owned by an administrative user (like the one you use to SSH into the machine), not the webserver. The group should be the group of the webserver (usually www or www-data). Appropriate permissions for read-only directories are: read+write+execute for the user, execute for group, none for the world. This means the administrative user can do anything, the group (i. e. the webserver) can only enter the directory, everybody else has no permissions at all. For writable directories: read+write+execute for the user, write+execute (and possible read) for the group, none for the world. The minimal permission required to use a directory is “execute”. “Read” means that the files in the directory can be listed (which is usually not necessary and may be dangerous). And “write” allows creation of new files.
  24. As the error clearly states, you're trying to access the index 0 of an associative array (due to the list construct). That's obviously not possible. You have an associative array, so the keys are strings: $sourceStmt = $databaseConnection->prepare(' INSERT INTO ... '); foreach ($_POST['sname'] as $source) { $sourceStmt->bind_param('issssss', $lastID, $source['sourcename'], $source['sourceaddress'], ...); $sourceStmt->execute(); }
  25. You need to learn basic debugging skills like how to inspect variables: var_dump($sname); This tells you exactly how $sname looks like, so you can stop guessing. Hint: It's an associative array. You're also missing a closing bracket in the sourcename parameter, and your code has XSS vulnerabilities all over the place.
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.