Skip to content

PR for Password Hash RFC #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 48 commits into from
Oct 16, 2012
Merged

PR for Password Hash RFC #191

Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
c77f2c2
Base structure for passsword_create and password_make_salt
Jun 25, 2012
7e41980
Actually complete password_create()
Jun 25, 2012
6574028
Implement password_verify
Jun 25, 2012
f7097d9
Fix memory leak on branch
Jun 25, 2012
18d3bd9
Basic random generator added to make_salt
Jun 25, 2012
618f262
More error checking, and some cleaning up for password.c
Jun 25, 2012
41d7374
Implement openssl support for make_salt
ircmaxell Jun 25, 2012
2d4b7cb
Refactor salt generation, rename password_create to password_hash
Jun 26, 2012
232da90
Implement php.ini setting password.bcrypt_cost
Jun 27, 2012
e505316
Add tests for password hashing
Jun 27, 2012
2b9591f
Update tests to check ini setting
Jun 27, 2012
5f44be0
Add tests and error checking for large salt requested values to preve…
Jun 27, 2012
0dd2f16
Fix formatting issues in password.c
ircmaxell Jun 27, 2012
6bb3865
Refactor crypt to use an external working function
ircmaxell Jun 28, 2012
da3d8bf
Refactor password.c a bit, add different error checking
ircmaxell Jun 28, 2012
9e18e57
Merge remote branch 'upstream/master' into hash_password
ircmaxell Jun 29, 2012
9c1445c
More refactoring of crypt into php_crypt, and fixing memory allocation
ircmaxell Jun 29, 2012
f53112f
Update password.c to use safe_emalloc in sensitive places
ircmaxell Jun 29, 2012
6cc3c65
Remove php.ini setting for default bcrypt cost
Jul 3, 2012
6943f2a
Some more refactoring, make algo no longer optional
Jul 3, 2012
886527d
Update signature info for changing algo to an ordinal
Jul 3, 2012
5160dc1
Implement password_needs_rehash() function
ircmaxell Jul 5, 2012
db86d54
Fix issue with int vs long parameter
ircmaxell Jul 5, 2012
ee7e799
Implement password_get_info() function
ircmaxell Jul 5, 2012
9d3630b
Cleanup whitespace issues
ircmaxell Jul 5, 2012
99b7956
Merge remote branch 'upstream/master' into hash_password
ircmaxell Jul 10, 2012
707c907
Switch second parameter to password_make_salt to be a flag
ircmaxell Jul 12, 2012
e05413c
Remove password_make_salt() from the implementation
ircmaxell Aug 28, 2012
824f1f4
Merge remote branch 'upstream/master' into hash_password
ircmaxell Sep 4, 2012
db41f9f
Refactoring to use size_t instead of int most places
ircmaxell Sep 4, 2012
e8b7f5b
Add tests for password_get_info and password_needs_rehash
ircmaxell Sep 12, 2012
e9a7bde
Switch test to using strict comparison for crypt fallback
ircmaxell Sep 12, 2012
ebe0bd5
Remove bcrypt_cost ini entry from declaration
ircmaxell Sep 12, 2012
76f3295
Expose PASSWORD_BCRYPT_DEFAULT_COST constant and update test to use it
ircmaxell Sep 12, 2012
3e383dc
Merge remote branch 'upstream/master' into hash_password
ircmaxell Sep 12, 2012
7161c3d
Add news entry for password API
ircmaxell Sep 12, 2012
7ec80e1
Fix incorrect arg info required param count for password_hash
ircmaxell Sep 12, 2012
83cfff4
Switch to using an ENUM for algorithms instead of a constant
ircmaxell Sep 13, 2012
e034a46
A bunch of naming convention fixes. No functionality changes
ircmaxell Sep 17, 2012
44c2624
Fix ucwords error casing
ircmaxell Sep 17, 2012
6fd5ba5
Fix arg info for required params passed to needs_rehash
ircmaxell Sep 17, 2012
8bd79d1
Refactor slightly to enable cleaner readability
ircmaxell Sep 17, 2012
4a7d18c
Fix some double free issues, and more cleanup work
ircmaxell Oct 5, 2012
25b2d36
Fix issue with possible memory leak
ircmaxell Oct 5, 2012
1751d5f
Really fix leaks, add test cases to prove it...
ircmaxell Oct 6, 2012
76e83f7
fix allocation and copy issue
ircmaxell Oct 6, 2012
37b2207
Clean up unreported memory leak by switching to zval_ptr_dtor
ircmaxell Oct 7, 2012
0bc9ca3
Refactor to using a stack based zval instead of dynamic allocation
ircmaxell Oct 7, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
More error checking, and some cleaning up for password.c
  • Loading branch information
Anthony Ferrara committed Jun 25, 2012
commit 618f2629567ca3a3d1817ca9c4c62339fb5fb886
18 changes: 13 additions & 5 deletions ext/standard/password.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@
#include <stdlib.h>

#include "php.h"
#if HAVE_CRYPT
#include "php_crypt.h"
#endif

#include "ext/hash/php_hash.h"
#include "php_password.h"
#include "php_rand.h"
Expand Down Expand Up @@ -121,7 +117,7 @@ static int php_password_make_salt(int length, int raw, char *ret TSRMLS_DC)
char *result;
result = emalloc(length + 1);
if (php_password_salt_to64(buffer, raw_length, length, result) == FAILURE) {
php_error_docref(NULL, E_WARNING, "Generated salt too short");
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Generated salt too short");
efree(buffer);
efree(result);
return FAILURE;
Expand All @@ -139,6 +135,12 @@ PHP_FUNCTION(password_verify)
{
zval *password, *hash, *ret;
int status = 0, i;
zend_function *func_ptr;

if (!PHP_PASSWORD_FUNCTION_EXISTS("crypt", 5)) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Crypt must be loaded for password_verify to function");
RETURN_FALSE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this happen? How can a string length be negative? And can size_t ever be smaller than int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic The string length could theoretically be negative if it exceeds the length of an int (overflow). I would rather check if hash_len is less than SIZE_MAX, but that's not defined on all systems...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. But that's a general problem that all functions accepting a string have in PHP. Not sure whether it makes sense to check for it. But if you keep the check the error message shouldn't use ucwords (just normal lowercase).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I wasn't casting to size_t for the internal operations, I wouldn't check for it. But with the cast, I felt that it would be prudent to at least check for it... I'll fix the error message (force of habit)...


if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zz", &password, &hash) == FAILURE) {
RETURN_FALSE;
Expand Down Expand Up @@ -195,6 +197,12 @@ PHP_FUNCTION(password_create)
int algo_len = 0, salt_len = 0, required_salt_len = 0, hash_format_len;
HashTable *options = 0;
zval **option_buffer, *ret, *password, *hash_zval;
zend_function *func_ptr;

if (!PHP_PASSWORD_FUNCTION_EXISTS("crypt", 5)) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Crypt must be loaded for password_verify to function");
RETURN_FALSE;
}

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z|sH", &password, &algo, &algo_len, &options) == FAILURE) {
RETURN_FALSE;
Expand Down