Hi Alex, Some nice conversation points, indeed. I do have a few follow-ups below: On Thu, May 19, 2011 at 3:16 PM, Alex Nikitin <niksoft@xxxxxxxxx> wrote: > PHP_SELF requires no processing (i.e. there is no need to do basename()) > Actually, the way Tedd is using the info, PHP_SELF would potentially be unsafe (unless it's been updated to correct for this type of issue (you'll see the blog post has it's own security issues with some missing plugins): http://www.mc2design.com/blog/php_self-safe-alternatives So, it would require processing either where Tedd performed the processing -OR- at in the markup to properly escape it. > > strcmp is binary-safe, i prefer and recommend using string-safe comparison > functions for strings... here is an example of why: > > $value = 0; > if($value=="not zero") { > echo "oopsie, how did this happen, lets see how this works with strcmp > (or === which i would advise)"; > if(strcmp($value, "not zero") == 0) { > echo "You wont see this"; > } else { > echo "Because strcmp works correctly"; > } > } > This, in general, is a sound practice, although I would certainly advocate the use of === as opposed to strcmp for performance reasons (as you pointed out.) To be fair to Tedd's code, though, I don't believe this would be an issue, as I believe that the global arrays store the values as strings, so for example: $value = $_GET['test_value']; if($value == "not zero") { echo "oopsie, how did this happen, lets see how this works with strcmp (or === which i would advise)"; if(strcmp($value, "not zero") == 0) { echo "You wont see this"; } else { echo "Because strcmp works correctly"; } } else { echo "Even if you enter a 0, I'll bet you see me."; } You did make several other great points (session hijacking, multiple login attempts), but to be fair to Tedd, there are many levels of security, and I doubt he's trying to educate PHP developers with your background. In the same way that someone's first foray into the world of database access using PHP likely avoids a 20 table database with complex transactions for atomic operations and in-memory queues for eventually consistent data where performance is a must, I see this as a reasonable first exposure to the general principles of how one might use the features of PHP to password protect a group of pages in a site. There are some forms of data I'd protect with an authentication scheme of this simplicity (maybe I just have a mileage app that I'm using to keep track of my weekly running, or maybe my wife has a todo list that she manages, etc.) However, as you pointed out, the code wouldn't merit use in situations where a higher security level is desired. Even your changes have security issues: - You're using a weak hash protocol, and not using a salt: https://www.owasp.org/index.php/Top_10_2007-Insecure_Cryptographic_Storage https://www.owasp.org/index.php/OWASP_Top_10_Threats_and_Mitigations_Exam - You don't mention using HTTPS, and session fixation, even if you use other techniques (session_regenerate_id after changing auth level, etc.) can't be prevented without this (let alone protecting the passwords from a man in the middle attack.) For developers who are first starting to think about a basic form of authentication, the code is a nice start. Perhaps Tedd could point out the shortcomings and provide some follow-up examples that provide progressively higher levels of security. That would be a nice, and I'm sure those on the list with your background would help on provide feedback on the more complex examples that progressively help new developers achieve higher levels of security. That said, you made some really nice points, and I'm hopeful Tedd considers them carefully. His site is a nice resource for many PHP developers already (especially those just starting out), and these changes can only make it better. Adam -- Nephtali: A simple, flexible, fast, and security-focused PHP framework http://nephtaliproject.com
