Bonesnap;11061131 wrote:Can you elaborate on this? What can be improved to "steer a dev toward best practices"?
I think your class could benefit from more detail in the comments. Your most recent version of the class makes no mention at all of CSRF prevention -- this is its primary purpose, right? Or might it be used for something else? Or consider this function. Some detail in the comments might guide someone using it to better behaviors by providing guidelines for injected values
/**
* Generates a cryptographically signed token for use in forms to prevent CSRF exploits; assign to a hidden input in your form
*
* @param string $serverSecret The server secret: a cryptographically secure series of random bytes in string format; avoid storing this site-wide credential in web root or in your git repo
* @param string $userSecret The user's secret: a cryptographically secure series of random bytes unique to each session; used to generate and validate CSRF tokens; never display this value in HTML; should be stored in SESSION; regenerating or refreshing may cause AJAX functions to stop working in SPA
*
* @return string Returns the new token in string format
*/
public static function generate(string $serverSecret, string $userSecret) : string
{
$token = bin2hex(random_bytes(32));
$hash = hash_hmac(self::ALGORITHM, $token, $serverSecret.$userSecret);
$signed = $token.'-'.$hash;
return $signed;
}
Bonesnap;11061131 wrote:In regards to protecting against misuse... I have mixed feelings about hand holding when it comes to this kind of stuff.
I have mixed feelings too. CodeIgniter has a config value that you switch on or off and a handful of other config values:
/*
|--------------------------------------------------------------------------
| Cross Site Request Forgery
|--------------------------------------------------------------------------
| Enables a CSRF cookie token to be set. When set to TRUE, token will be
| checked on a submitted form. If you are accepting user data, it is strongly
| recommended CSRF protection be enabled.
|
| 'csrf_token_name' = The token name
| 'csrf_cookie_name' = The cookie name
| 'csrf_expire' = The number in seconds the token should expire.
| 'csrf_regenerate' = Regenerate token on every submission
| 'csrf_exclude_uris' = Array of URIs which ignore CSRF checks
*/
$config['csrf_protection'] = TRUE;
$config['csrf_token_name'] = 'csrf_test_name';
$config['csrf_cookie_name'] = 'csrf_cookie_name';
$config['csrf_expire'] = 7200;
$config['csrf_regenerate'] = TRUE;
$config['csrf_exclude_uris'] = array();
You set these values and then you have to take care to display the csrf token in every form yourself. The base class for a controller exposes a security property which lets you retrieve CSRF-related values:
$this->security->get_csrf_token_name(); // returns the input name that will be checked when you submit a form
return $this->security->get_csrf_hash(); // returns the current expected CSRF token
I think it's pretty nice that CodeIgniter handles many aspects of the operation (generating the tokens, setting session/cookie/whatever, etc). On the other hand, I'm not entirely sure about the precise details of its functioning. I generally assume the CodeIgniter devs (or the CI community) has examined the internal behavior to make sure it does its job well. I realize this may not the case. If I had any comlaints about how it works in practice, a few come readily to mind:
code comments reveal almost nothing about how it works and little about how to use it.
The CSRF check seems to happen early in the application's bootstrapping code and afaik there is no way to intercept the event when CSRF fails -- and the error message is not very informative.
The docs don't offer much more detail about how things work.
There's no advice or guidance about how to deal with AJAX situations or any way to force regeneration of the token.
In a lot of ways, I prefer your approach because it's more transparent. It doesn't really offer any advice on how to use it or what parameters to provide or what care one might want to take with the secret values supplied.
Bonesnap;11061131 wrote:At its core this class was created so I can use it when I start new projects. Instead of constantly copying a folder from one project to another I could simply just run a command and bring it in, or if I make changes to it I can run another command and it will update my project for me. I was going to keep it private on Bitbucket because Packagist supports private repositories (you just need an SSH key). But I thought or I could make it public and if someone finds a use for it, then all the better. If you're saying that this is harmful in general to the PHP landscape because of the potential for misuse, then I can just keep it private. But there is a laundry list of methods, classes, etc. in the PHP library that can be misused.
Bonesnap;11061131 wrote:All the situations was explained earlier in the thread. It's being used to generate a token that's passed with AJAX requests. If the token validates then the AJAX request proceeds otherwise there is an exception and an error message is returned to the user.
I went back over the previous posts and perhaps I missed the PHP code that shows this class in use. Perhaps you could refer me to the post with PHP code where you generate a user secret, store it in session, and then check on form submission?
