-
Posts
4,207 -
Joined
-
Last visited
-
Days Won
209
Community Answers
-
Jacques1's post in Apostrophe in search term kills my page was marked as the answer
You're inserting $_POST['search'] straight into the query string, which means that a single quote will terminate the SQL string after the LIKE keyword and change the entire query. It's a self-inflicted SQL injection. What's much worse than your current bug is that anybody can manipulate the query and fetch sensitive data (passwords, e-mail addresses etc.) or even take over the server.
To fix this vulnerability, stop inserting PHP string into queries. The whole point of prepared statements is that you use a constant query with placeholders:
<?php const DB_HOST = 'localhost'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; const DB_CHARSET = 'UTF8'; /* * Set up the database connection * - the character encoding *must* be defined in the DSN string, not with a SET NAMES query * - be sure to turn off "emulated prepared statements" to prevent SQL injection attacks * - turn on exceptions so that you don't have to manually check for errors */ $dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET; $databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [ PDO::ATTR_EMULATE_PREPARES => false, // activate real prepared statements PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, // activate exceptions PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, // fetch associative arrays by default (this isn't critical) ]); $songStmt = $databaseConnection->prepare(' SELECT artist, -- always select *concrete* columns title FROM songs WHERE artist LIKE :artist_search_term OR title LIKE :title_search_term LIMIT 100 '); $songStmt->execute([ 'artist_search_term' => $_POST['search'], 'title_search_term' => $_POST['search'], ]); foreach ($songStmt as $song) { var_dump($song); } -
Jacques1's post in Session is dropped after redirect? was marked as the answer
You're also terminating the session with session_destroy() and then try to write data to it. This of course makes no sense. Get rid of the statement. I guess what you actually want is session_regenerate_id(true).
-
Jacques1's post in Dont understand this was marked as the answer
The array_intersect_key() stuff is a very obscure way of removing all ($key, $value) pairs from $data where $key isn't in $fields.
So $fields is basically a whitelist of allowed keys (as explained in the comments).
-
Jacques1's post in Handling exceptions by loading a page with a safe message. was marked as the answer
Ideally, you shouldn't need any handler.
Disable both display_errors and display_startup_errors to simulate a production environment. Then create a script with a fatal error and watch the developer tools of your browser. You should see a blank page and a 500 response code. If this works, you have to make Apache intercept this error and display a custom error page. How do Apache and PHP communicate on your production server (not the XAMPP testing stuff)? Via FastCGI? mod_php? CGI?
If you don't want to (or cannot) intercept the error with the webserver, you should register a shutdown function as explained in the Dev Shed thread. This function can then render a user-friendly error message.
-
Jacques1's post in aes js & php was marked as the answer
Both the key and the IV must be raw, unencoded strings with exactly 128 bits (or 16 bytes). Right now, all your strings are hex-encoded and also way too long.
const CRYPTO = require('crypto'); const CIPHER = "AES-128-CBC"; const MASTER_KEY_HEX = "5e2a0626516f3108e55e25e4bb6a6283"; const MASTER_KEY = new Buffer(MASTER_KEY_HEX, "hex"); // encrypt var plaintext = new Buffer("attack at dawn"); var initVector = CRYPTO.randomBytes(16); var cipher = CRYPTO.createCipheriv(CIPHER, MASTER_KEY, initVector); var ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]); console.log( ciphertext.toString("hex") ); // decrypt var decipher = CRYPTO.createDecipheriv(CIPHER, MASTER_KEY, initVector); var decryptedPlaintext = Buffer.concat([decipher.update(ciphertext), decipher.final()]); console.log( decryptedPlaintext.toString() ); -
Jacques1's post in aes - php alternative was marked as the answer
First off: I hope this is not your actual master key? If it is, you now need a new key.
Node.js is perfectly capable of performing cryptographic operations. In fact, it's a lot better than the half-assed PHP/OpenSSL extension we're stuck with. Just make sure to use crypto.createCipheriv() so that you can pass the stored initialization vector and master key to the function. The master key should be placed in a separate configuration file outside of the document root (this is also a lot safer than embedding it in the application code).
-
Jacques1's post in Need advice in making databases was marked as the answer
You should definitely have a single database for the entire application. Creating a new database for every school is a bad idea. Not only is it insecure, it's also a huge problem for backups and updates, because you'd have to repeat everything for each database.
MySQL was designed to handle hundreds of millions of rows. I'm fairly sure your school project needs a lot less than that.
-
Jacques1's post in PHPMailer errors was marked as the answer
Pass true to the constructor to enable exceptions.
Note that there are many different reasons for why a mail isn't delivered, and not all of them can be fixed with code. Anyway, let's see what PHP has to say.
-
Jacques1's post in mysqli check if 2 value exist was marked as the answer
OK. In that case, the code will be good enough.
To distinguish between a duplicate name and a duplicate address, you can either parse the error message or make a second query after the INSERT query failed: Simply fetch all users where the name or the address matches the submitted data, then inspect the resulting row to find out which of the two has caused the problem.
-
Jacques1's post in selecting user info if 2 rows in mysql exist was marked as the answer
SELECT
-- select concrete columns, not just "*"
FROM
mail_list
JOIN rosters USING (Code)
WHERE
rosters.SectorDate IN ('2016-01-04', '2016-01-24')
GROUP BY
-- repeat all the columns from the SELECT part
HAVING
COUNT(DISTINCT rosters.SectorDate) = 2
;
-
Jacques1's post in some object oriented problem was marked as the answer
Try to slow down and write your code more cleanly. This includes proper formatting. When you rush it, you'll spend most of your time debugging errors (or waiting for others to debug them for you), which is somewhat frustrating.
An IDE (integrated development environment) like Netbeans or Eclipse can help you write good code, because it will notify immediately when there's an obvious problem (like a parameter which isn't used anywhere).
-
Jacques1's post in Issue with session variables was marked as the answer
You cannot have output before session_start(). Since HTML markup is output, your script blows up. To avoid this problem now and in the future, I strongly recommend you keep your PHP code and your HTML separate instead of mixing them: All the PHP logic goes to the top of the script, all the HTML markup goes to the bottom.
The second problem is that you try to access $filename outside of the if statement which sets this variable. In other words, even when the condition isn't fulfilled and no variable set, you still try to use it.
And of course the whole database code is obsolete and wide open to SQL injection attacks. We use PDO with prepared statements now.
-
Jacques1's post in $stmt->get_result error 500 was marked as the answer
The get_result() method is only available for the MySQL native driver (which your server appearently doesn't have). You either have to change the PHP installation or fetch the data with the classical bind_result/fetch.
By the way, remove this filter_input() stuff. It will damage the incoming data and serves no purpose whatsoever.
-
Jacques1's post in JS Basic Question was marked as the answer
This array feature which you appearently know from PHP is actually very exotic and really only makes sense in the super-sloppy PHP philosophy.
Technically speaking, you simply cannot apply the index operator to the empty value t['key1']. There's no such thing as null['key2']. If you try this in Ruby, your script will blow up with an error message. The same is true for Python. The only reason why PHP doesn't blow up is because it's specifically designed to accept errors. Instead of telling you to fix your code, it will guess what you mean and create a new subarray for you.
So this isn't how languages normally works. It's a PHPism.
-
Jacques1's post in remove malicious php from image uploads with IMagick was marked as the answer
Yes. Again: A PHP script which serves image data with the right content type behaves exactly like a physical image in every aspect. It's the exact same thing.
In the HTTP context, an “image” is really just a URL pointing to image data. Where this data comes from is irrelevant. It might be an actual file served by the webserver, it might be a PHP script reading files from outside of the document root, it might be dynamically generated with no files involved. The client neither knows nor cares.
As requinix already told you, there is no difference. There cannot be a difference. In both cases, the browser makes a request to the server (unless the image is already in the cache), fetches the image data and displays it.
There is no magical “View Image” request in the HTTP protocol. It is possible to suggest to the client that a file should be downloaded, but that's an entirely different feature which none of us has suggested.
If you don't believe us, try it yourself.
-
Jacques1's post in form action php script was marked as the answer
The effective form action is determined by the browser, and the browser doesn't know anything about PHP or how you've organized your scripts on your server. If there's no explicit action, the form data is sent to the document's address which may or may not point to the script which was original called.
-
Jacques1's post in Question about deleting where no matching record to join was marked as the answer
The second query is syntactically invalid. See the manual for how to write a multi-table DELETE query.
-
Jacques1's post in CRYPT_BLOWFISH and supplied string length was marked as the answer
Trust me, it's not “just a wrapper”. Even the original author of the password API got it wrong a couple of times, because the crypt() function is weird as hell and had several bugs in different PHP versions.
Unless you've actually inspected the C source code, read through the bugtracker and fully understand how bcrypt works, it's just dangerous to use crypt() directly.
The salt is not an arbitrary string, and the manual is very misleading. What the crypt() function actually expects is a sequence of 128 random bits generated with a CSPRNG and encoded with a special Base64 variant (not the usual Base64). If you fail to provide a valid salt, pretty much anything can happen.
And that was the easy part.
How do you get a proper CSPRNG accross all PHP versions and operating systems? Are you aware of the bcrypt input limitations (no more than 56 bytes, no null bytes)? How do you enforce that? Note that strlen() may either return the number of bytes or the number of characters, depending on whether it has been overloaded by the Mcrypt extension. Are you aware of the various error representation and output bugs? How do you validate the resulting hash? How do you prevent timing attacks when you compare the calculated hash with the expected hash? ... I strongly recommend you use a library which has already gone through this like password_compat.
-
Jacques1's post in Regex : better solution than ^[0-9]+(,[0-9]+)*$ ? was marked as the answer
Try possesive quantifiers:
'/\\A\\d++(,\d++)*+\\z/' -
Jacques1's post in converting escaped characters was marked as the answer
So the strange hex sequences you talked about in your first post have somehow disappeared, and now all you want to do is convert the input document from ISO 8859-1 (or Windows-1252) to UTF-8?
How exactly does iconv() “fail”? No, PHP's file functions are not limited to ASCII. They simply read bytes, so they work for any encoding. This works just fine:
<?php header('Content-Type: text/html; charset=utf-8'); $input = file_get_contents('input.html'); $output = utf8_encode($input); echo $output; -
Jacques1's post in Brute Force prevention - little stuck was marked as the answer
You haven't really solved anything, because the code has fundamental problems beyond this little redirect issue.
Your code has a time-of-check-to-time-of-use defect (like in your previous thread, by the way): You first check the current warning counter, and if it's below the limit, you allow the user to log in. But who says there haven't been any attempts in the meantime? If an attacker sends a large number of concurrent requests, your application will accept all of them, because it still “sees” the old counter value. After a short while, the counter will be incremented, but then it's already too late. Banning IP addresses is a bad idea, because it doesn't stop attackers and at the same time hurts legitimate users. An attacker can easily switch to a different IP address and start over (botnets are cheap, and there are countless public proxies). But a legitimate user normally uses just a few IP addresses which may be shared by thousands of other people. If one person exceeds the log-in limit, everybody will be banned for an entire month. In other words: After a short while, you'll have locked out half of the world population. This makes no sense whatsoever. The whole implementation is very cumbersome with lots of duplicate code and no clear structure. Personally, I think that brute-force checks are rather pointless and give users a false sense of security. Anyway, if you insist on implementing this, you need a very different approach:
Assign a counter to each user account, not each IP address. In addition to that, you could maintain a global counter to recognize attacks against multiple accounts. Apply a less user-hostile form of protection. For example: Instead of banning people, make them solve CAPTCHAs. Or implement small delays. If you insist on bans, use realistic time limits (not a whole month!) and give your users a chance to unlock their account (e. g. via the I-forgot-my-password procedure). The check and the increment of the counter must happen in a single atomic operation so that the limit cannot be circumvented with concurrent requests. For example: // update the counter, and simultaneously save the new value in a user variable $updateLoginAttemptsStmt = $databaseConnection->prepare(' UPDATE users SET login_attempts = (@login_attempts := login_attempts + 1) WHERE user_id = :user_id '); $updateLoginAttemptsStmt->execute([ 'user_id' => $userID, ]); // get the counter value of the previous operation $loginAttempts = $databaseConnection->query('SELECT @login_attempts')->fetchColumn(); if ($loginAttempts <= MAX_LOGIN_ATTEMPTS) { // user may try to log-in } else { echo 'You have exceeded the maximum number of log-in attempts.'; } -
Jacques1's post in formatting data was marked as the answer
Simply format the number in your application when you display it.
Or if you insist on a doing this with MySQL, use FORMAT() in your query.
-
Jacques1's post in Trying to update database using PDO connection, says successful but no update was marked as the answer
Remove the single quotes surrounding the placeholders. You should also get rid of the useless transaction. A single UPDATE query either suceeds or fails, there's nothing in between.
To check if the update actually changed the data, use PDOStatement::rowCount().
-
Jacques1's post in help with database class was marked as the answer
You're randomly mixing mysql_* functions with mysqli_* functions. You cannot do this. They belong to two entirely different PHP extensions which are not related to each other (besides the fact that they both deal with MySQL). Actually, the mysql_* functions are hopelessly outdated and will be removed with the release of PHP7.
The “var” keyword you're using in your class is also obsolete. It dates back to PHP4 which died somewhere around 2008. So it looks like you really need to update your learning resources. Nowadays, we use access modifiers (private, protected, public) instead of just “var”. What's weird is that you already use those modifiers for your methods. So I guess the “var” stuff was copied and pasted?
Your database code is also somewhat shaky (no security, no proper error handling). I don't know. Maybe you should write your first class for a problem that you really understand. This database wrapper stuff usually just ends in a truckload of SQL injection vulnerabilities.
-
Jacques1's post in is this AJAX code good enough ? was marked as the answer
Your code has several problems:
It is vulnerable to cross-site scripting attacks, because you insert the user input straight into your HTML markup. Of course this is just a toy example, but when you write real applications, you need to be much, much more careful when using innerHTML. You've forgotten to URL-encode the content of str before using it for the name parameter. This will lead to a misinterpretation of the input if certain characters are present. For example, try entering “Paul&Mary” or “Joe#Blow”. You'll see that both are truncated, because “&” starts a new parameter, and “#” denotes a URL fragment. You need to run str through encodeURIComponent(). You have no error handling. If the HTTP response code is something other than 200, you don't do anything. You're using the ajax() function as an event handler before it's even defined. Your ajax() function has a parameter, but you never use it. The code you've wrapped in a try statement cannot possibly throw an exception, so the whole statement is useless. I guess what you actually meant to do is catch exceptions in the function. So put the try statement into the function. In general, people today rarely write low-level JavaScript code to make Ajax requests. This is certainly useful for learning, but in the long run, you should look into frameworks like jQuery. Then you don't have to write tons of boilerplate code for such a simple task.