Double check your codes so that it meets the coding convention. Also files made by other authors should meet the style for uniformity.

Codes must not look like two different people made this.

I believe you need a user class, and a user management class. You can also put those management functions (such as edit, create, delete) in the same User class, but as static methods.

Yes, US keyboard like a zillion keyboards out there, but I think you need to provide a way to supply other common keyboards, like AZERTY and so on.

The new modified file DatabaseManager contains an SQL function. IF in that function the connection is not set by the user properly, ERROR is thrown that call to function on null object. We have to handle this error.

Please always use braces, even if it's only one line. It's also in the PSR that this project uses.

Wrong:

if ( $foo )
    blah();

Right:

if ( $foo ) {
    blah();
}

The wrong way is a constant source of maintenance bugs, so let's just be strict about it.

This feature is deprecated in 5.3 and exits with a fatal error in 5.4 and 5.5.

This should be a successful test, because the encrypted string is the direct result of a previous call (and manual fixing the resulting syntax error).

$this->assertSame("root", confidentialString(':/X6NSUlAagxmmLNWRZBA8fyJbmQZmAB7VcgzHHfTxwA='));

The test is failing because the returned result of confidentialString appends an amount of zero-bytes. This will affect the usage of the string in undetermined ways.

There are two occurrences of this function:

/libs/core/functions.php
/libs/crypto/confidentialstring.php

Just dump the db schema with a few sample rows, so that other developers can set up the framework for testing and development.

The function will work with ASCII as usual, but whenever it comes to more unusual characters, especially from other languages and in different encodings, things will get interesting.

Why "echof" near line 230?

echof(fread($f, $advanceBy)); //read the specified number of bytes.

The correct value of the currently requested URL must use the value from HTTP_HOST, because that is the value the client actually used for the request.

Note that SERVER_NAME actually IS the value from HTTP_HOST if the Apache VHost is configured with UseCanonicalName off - in this case the error does not occur. Setting it to "on" will lead to wrong results if there are entries for ServerAlias.

Remove the trash folder please, we're using a version control not a directory. If we need something later, we have it in the history track.
someone that pulls this project gets all those files, and it increases the size considerably.

This issue is meant to serve as a placeholder for all discussions regarding the http request handling library.

isBruteForce must return "TRUE" if time between two consecutive attempts is less than "bruteForceLockTimePeriod" , but according to the definition it will return TRUE only when time when last login attempt is less than bruteForceLockTimePeriod as well as total login attempts have exceeded bruteForceLockAttempts. Below is one of the definitions of "Brute Force" described in the class :

public static $bruteForceLockTimePeriod = 5; //5 SEC - This defines the time-period after which next login attempt must be carried out. E.g if the time is 5 sec, then time-period between two login attempts must minimum be 5 sec, otherwise it will be considered brute-force attack.

Attackers can use resetPassword in two ways:

a) enumerate valid user account names
b) denial of service users by causing their password to be reset

The design of the resetPassword must cope with providing business logic in such a way that account enumeration is not possible.

I strongly suggest that resetPassword does not change the user's password until the temporary token supplied to them is validated.

Please disregard.

Advanced password management is extending User, which is very odd! Either the User is not the User class (maybe its UserManagement, and not a single user?) or advanced password management is not about password management, but extended user management.

All those setters and getters need to go away. Code that when you write, feels not smart, should not be there. Code is art, and its only something humans should be able to do.

Do not define temp variables unless necessary, for your queries you do:
$query = " ... "
$args= array( .. , .. , .. );
SQL($query, $args)

instead do SQL(...,..,..,..)

I am sad to state that this library currently limits it's usefulness by ignoring any of the good PSR standards - and PSR-0 as the most basic and useful standard for autoloading.

PSR-2 states that PSR-1 must be followed. PSR-1 states that PSR-0 must be followed for namespaces and classnames. And PSR-0 has the following mandatory requirements:

• A fully-qualified namespace and class must have the following structure \VendorName(Namespace)*ClassName
• Each namespace must have a top-level namespace ("VendorName").
• Each namespace can have as many sub-namespaces as it wishes.
• Each namespace separator is converted to a DIRECTORY_SEPARATOR when loading from the file system.
• Each _ character in the CLASS NAME is converted to a DIRECTORY_SEPARATOR. The _ character has no special meaning in the namespace.
• The fully-qualified namespace and class is suffixed with .php when loading from the file system.
• Alphabetic characters in vendor names, namespaces, and class names may be of any combination of lower case and upper case.

These exceptions in the mentioned https://github.com/OWASP/phpsec/wiki/Coding-Convention violate PSR-0:

II. All file names are lowercase

This cannot be, because the classes are named in UpperCaseCamelCase (or StudlyCaps). The filename must reflect this.

1. Class names are CamelCase...

This is no exception and shouldn't be listed as such, it is required by PSR-0.

II. 5. One php file can have multiple classes,...

No, this prevents autoloading. If the filename cannot be derived from the namespace/classname, the class cannot be loaded.

Please reconsider your policy of violating PSR-0. Autoloading is a very important feature for performant PHP applications, and one of the core concepts in the dependency management of Composer (see http://getcomposer.org/).

If this library at some point wants to be useful, it will very likely be included via Composer. Do not circumvent it by an unnecessary standard violation.

It violates all our rules, and is not required. Just a simple if!

Just to test cookies, you made a different file. That is unprofessional. Find a way to merge that file into the original test file.

in session library there is a code segment: in existingSession() function line 128-136
$sessionID = $_COOKIE['sessionid'];
$result = SQL("SELECT SESSION_ID, USERID FROM SESSION WHERE SESSION_ID = ?", array($sessionID));
if (count($result) != 1)
{
$this->updateUserCookies(TRUE);
return FALSE;
}
$this->session = $result[0]['SESSION_ID'];
$this->userID = $result[0]['USERID'];
I could not understand why the overhead of getting SESSION_ID from database was employed here?

I see a lot of flaws here.
First, you have two or three very long classes in one file. That is not right.

Second, every static function in BasicPasswordManagement is public. Refer to the convention for access.

Third, User extends BasicPasswordManager. This is essentially wrong. A user does not have a salt. A user might have a password, and that password might have salts and all.

1. You should not pass around db connection. You should handle them somewhere.
2. Why do you get password as a param into newUserObject and existingUserObject
3. There are no optional fields in User object. if someone needs them, they can extend from our User class.
4. It still has a lot of setter/getters. Rid them, this is not Java.
5. namings require much more effort. For example, if you wanted to know if one user's password is expired or not, you would never look for checkIfPasswordExpired. You might probably go for isExpired or isPasswordExpired or passwordExpiry, but not check...
6. I believe you need a user class, and a user management class. You can also put those management functions (such as edit, create, delete) in the same User class, but as static methods.
7. If its authentication library folder, you can move advanced password management here as well.

Before deleting user from database, check for dependencies. Because data such as session and password depends on user. Deleting user directly without checking for those would result in Foreign Key Constraint.

There appear to be a few issues around pertaining to code-style - naming conventions, appropriate use of parameters and the like.

Would you mind tagging these with a 'code-style' tag (or equivalent), as opposed to the 'enhancement' tag?

1. In recent discussions, Shivam mentioned that hashing algo must not be in DB because that reveals too much info in case the DB is compromised. So, if we have to remove the hashing algo from the DB, then that means that the algo would have to be same for all users now [now the developer has the option to keep different algo for different users].
   What will happen now is that once the admin changes the hashing algo, all the old user's passwords will be invalid (which was not in the first case), So, to remove that info from the DB, we have to create a function that automatically forces every user to change their password according to the new algo.
   SO SHOULD WE DO THAT ??

2. Shivam made one more suggestion that we should concatenate the hash and the dynamic salt in the DB, so that even if DB is compromised, the attacker wouldn't know which part is salt and which is hash....that creates confusion which is good. And on top of that since we are also removing hash algo from the DB, the attacker would be really confused. To do this, I would have to first remove algo from the DB.
   SHOULD WE DO THIS ??

Guys. please comment and after that I will make the changes.

We have to run the static parser to determine where we have used "echo" and "print" statements which are insecure. We have to replace them using echof

A few times code style has been mentioned, but I am not sure what the appropriate code style is. Is there a preferred guide located on t'interwebs, perchance?

I have just created a new wiki page containing a few notes with the bits and pieces I have picked up so far - it is very much incomplete and mostly for personal use / reference, but I believe it would be beneficial to add to it every once in a while where decisions are made and/or questions are asked.

1. Filename is inappropriate.
2. It has very little functionality.
3. It should be as transparent as possible.

I'll try to do this one

1. When a function throws exceptions, it should indicate them in its PHPDOC at the top using @throws headers. This way uses will know what possible errors occur in the method, and handle them appropriately without looking into the source code.
2. You have too much code, specially in getters and setters, that are never used. setInactivityTimeout and stuff like that, they just make the code look complicated. We also don't do type checking in PHP unless necessary. Let it error instead of checking it.

Just assign things to properties and all, no getters and setters unless some real magic is needed.

1. Throw base \Exception very rarely, and only when the application really needs to die. No one can catch and handle them properly.
2. Never use underscores in method names.
3. Add comments for properties, in PHPDOC. They must at least define types.
4. I don't get why you are using try...catch blocks in the code. You are not using some other library that throws error! Its futile.
5. No need for destructor. The thing you're trying to achieve happens automatically.
6. Remove underscore from property names, and make them public if they are supposed to be changeable by the user!
7. Use private only when its private. Most of what you're doing needs protected. How to know? Imagine one class inherits from your class to add some functionality, does it need access to something? if yes, go protected.

Thats all for now :D

1. I think its best to move this to http folder, since we're having libs for HTTP Request and HTTP Response.
2. Its better to change MIME function to use an array. It would be much faster than a lot of ifs. Extension in key, MIME type in value.
3. IsModifiedSince should be protected.
4. Same goes for calculate... , also remove underlines from this. http can be lowercase.
5. readFromFile doesnt have a good name. Its not what it does. Also shouldnt be public
6. A large portion of the code is duplicated, merge it.
7. I don't know if Feed is also a good name. If you come up with something smarter use that. If you make everything else protected, anything public is a good remaining name :D

Static salts should be per app, per instance. Please move user.php :: staticSalt to a configuration file to allow the users of the library to override it. It must be a user serviceable value.

If it's buried deep in a library, it's not really a special value. We found that out in Ruby on Rails a few times.

Hi..have you put any function to properly close DB?

You are using regexp to consume code, its invalid. Use php parser (like the one used in jwidget) to parse, also move tools folder out of libs.

I'd really like to see this project implement a no-tabulator policy for the source code. It's the only true deviation from all the PSR coding standards, and it is painful.

Can we simply revert this decision without much ado?

I think the was that session ids are generated is not very random. And this is concealed because of a layer of function calls that all look very good. Please follow me on the code path:

Let's start at Session::newSession https://github.com/OWASP/phpsec/blob/master/libs/session/session.php#L101

As you can see, there is a call to the function random(32), which promises to deliver 32 random characters.

That function https://github.com/OWASP/phpsec/blob/master/libs/core/random.php#L25 is only a simple proxy and forwards the call to Rand::randStr(32).

Now that function https://github.com/OWASP/phpsec/blob/master/libs/core/random.php#L109 actually does something: It grabs some randomness, hashes it into a string of finite length (which would lead to the whole functions so far to NOT deliver e.g. a random string of 2048 characters without hint or error - bad in itself, but not my point).

Ok, randomness... We do see a call to Rand::randRange() https://github.com/OWASP/phpsec/blob/master/libs/core/random.php#L87 with default parameters. This function does some checks, probably reverses $min and $max, grabs some randomness and then simply scales that into the range given. The range for default parameters is $min=0, $max = 2^31. So whatever the random generator delivers, it will here be reduced to 31 bits of randomness.

And the random number generator is in Rand::random() https://github.com/OWASP/phpsec/blob/master/libs/core/random.php#L47 itself has no parameters. In an ideal situation, the very good random function openssl_random_pseudo_bytes() would be asked for FOUR bytes of randomness, which equals 32 bits. In absence of this function, the very bad random generator mt_rand() would only issue 31 bits of randomness.

All in all, the session ids generated by this code will only contain 31 bits of randomness, packed into a nice looking substring of SHA512, suggesting much more randomness being included (SHA512 itself is 128 hex chars long, leaving room for 64 bytes of randomness instead of only 4 bytes).

Even the PHP default session id hash has /* maximum 15+19+19+10 bytes */ = 63 bytes of pseudo-randomness and not-that-easy-to-guess information - and this is criticized as "attackable" by some. (see https://github.com/php/php-src/blob/master/ext/session/session.c#L309)

What are good sources of randomness? I think a good example is given in the random salt generation of the password_hash library: https://github.com/ircmaxell/password_compat/blob/master/lib/password.php#L84

Plenty of sources are being asked, in the hope to find some good randomness:
mcrypt_create_iv(), openssl_random_pseudo_bytes(), fopen('/dev/urandom', 'r') - and only then, as a last resort, mt_rand() with as many calls as there should be bytes in the salt string.

1. Serialize all values

The code on line 698 doesn't check that the userID value in the session is set before using it in a query. This should be improved.

The code assumes if $this->session[0] exists, that $this->session[1] exists and contains a valid userID value. Neither of these things may be true. Please test for variable existence before relying on them.

The code on line 703 doesn't check that the AUTHID cookie is not null in some way, so an equality check with two equals might allow null result from the SQL query with a null cookie value to be considered a successful validation.

$auth['SESSION_ID'] == $_COOKIE['AUTHID']

Can you please test these items in test cases:

Valid session[0] object but logged out (does this mean that the session object is there?)
Valid session[0] object, but AUTHID to null and set session->userID to be an invalid userID

1. The tainted classes should be moved somewhere else, probably in a different file in the same folder.
2. There's no need for the data array to be a different object. Inherit HTTP Request from it and other methods are static. Make the array interface abstract
3. IsCLI should be protected. Outsiders won't look for that in a HTTPRequest class
4. Add two more methods, namely Path and application Path, and also Request and Application Request. Path should contain everything after example.com/path/is/here but not after ? or #. Request should contain those as well.
   The application specific parts of those two, should only return that for the application, i.e if your application is in example.com/application, and the url is example.com/application/somewhere/in/here application path would be somewhere/in/here
5. Make method names the same as those that are available in Javascript's document.location.*
6. Add Scheme method that returns HTTP or HTTPS, and add a changeScheme method that converts one to the other. Its better to have them as constants in the class instead of raw strings.
7. Check if Method is secure, i.e no one can tamper it outside those 5 valid values.
8. When in localhost environment, IP returns ::1 which is localhost address in IPv6. I don't know how we can make that clear to the user.
9. Add serverIP method, which returns the interface which the server is connected to the client on, not the interfaces that the server is listenning on.

thats all for now!

I have this line of code in a test:

$this->assertSame("root", confidentialString("root"));

After running the test (which is successful), this line gets converted into this:

$this->assertSame("root", confidentialString(':/X6NSUlAagxmmLNWRZBA8fyJbmQZmAB7VcgzHHfTxwA=');

Note the syntax error introduced: A closing parenthesis is missing.

Any access inside this class that reaches for $_SERVER['HTTP...'] values has to decontaminate them - otherwise an error is triggered.

Exception messages should not assume anything about how the message will be presented to the user. If any formatting is desired, it would be the task of the error display routine.

Additionally, making the message contain the HTML markup also means that any dynamic values must be brought into HTML context, i.e. they must be htmlspecialchars-escaped. Otherwise the resulting string could not be used for anything.

throw new InvalidFileModifiedDateException("<BR>ERROR: The last modified date of the file: {$File} cannot be determined.<BR>");

Searching for "
" will probably reveal all these instances.

all files are uppercse.
rand -> random

randLen is a bad name
random is private

generateRandom is a bad name
seed should be protected

The last commit "f46151867b98e8e9267431975cc97d372e712b67" somehow went bad. It was my fault. Please check if all your files are intact.

Look for any potential data before you exit the test files. The best way to do this is to define the function tearDown() and use SQL statements to remove any data that is still in the DB.

1. why is a file named model? filenames should reflect what they hold. If you're having it as a base database adapter, put in adapter as base. If it is database manager class, make it manager.php . Model.php is the worst name one can come up with.
2. same goes with db.php, I was actually going to find base class there. dbmanager.php is a perfectly valid filename. manager.php is the same.
3. db.php uses REQUIRE.
4. Do not throw raw exception.
5. If you need to use ../ in your include paths, always wrap them around realpath() php function.
6. We miss PHPDOC in adapters, specially on constructors. How should one know what to send to them?

In auth library, "user.php" which is used for implementing basic password management when creating a "newUserObject", the static salt is also stored in the database. If database is compromised the attacked will have static salt, dynamic salt, hash as well as hashing algorithm which will ease his task of reverse lookup! Is it really
required to store static salt if it is going to be same for all users ?

Hello All,

It was suggested by Abhay, that we should use bcrypt for storing passwords for the reason that its slow. So, even if someone does brute force, it would be slow and provides additional security.