Re: A Review Request

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [PHP Home]     [Apache Users]     [PHP on Windows]     [Kernel Newbies]     [PHP Install]     [PHP Classes]     [Pear]     [Postgresql]     [Postgresql PHP]     [PHP on Windows]     [PHP Database Programming]     [PHP SOAP]

  Powered by Linux