Make WordPress Core

Opened 15 years ago

Closed 14 years ago

#10727 closed defect (bug) (fixed)

Update phpass to version 0.2 (check /dev/urandom before accessing it)

Reported by: hakre's profile hakre Owned by: westi's profile westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8.4
Component: General Keywords: has-patch tested early
Focuses: Cc:


the phpass class is using the @ error operator to suppress messages when /dev/urandom is not accessible but does not check wether it is readable before.

accessing such a resource on systems where it does not exists (win32/winnt operating systems) this will lead to a warning.

this warning is suppresd by the @ operator but handeled over to the error handler anyway. it therefore stands in the way if you implement own error handlers and throw exceptions then like

set_error_handler(create_function('$errno, $errstr, $errfile, $errline', 'throw new ErrorException($errstr, 0, $errno, $errfile, $errline);'));

the @ operator is considered bad practice not only because of that and it's usage should be reduced.

attached you will find a patch that is preventing errors on windows systems (and others where /dev/urandom is not accessible) and therefore should improve it.

I contacted the class author as well so there is a chance to have this upstreamed.

Attachments (3)

10727.patch (463 bytes) - added by hakre 15 years ago.
10727.2.patch (1.1 KB) - added by hakre 15 years ago.
upgrade of phpass to version 0.2
10727.3.patch (1.1 KB) - added by hakre 15 years ago.
Updated for compability reasons

Download all attachments as: .zip

Change History (14)

15 years ago

#1 @hakre
15 years ago

  • Keywords has-patch added

#2 @hakre
15 years ago

I had contact with the class author and he decided positivly for the patch per se. I think this should go clean here & upstream in parallel and therefore test & green while this is patched directly here. It has been even checked being PHP 3 compatible. Just as a sidenote.

#3 @hakre
15 years ago

  • Keywords needs-patch added; has-patch removed
  • Summary changed from check /dev/urandom before accessing it to Update phpass to version 0.2 (check /dev/urandom before accessing it)

phpass has been updated to version 0.2 containing that change. I had not noticed that in the time of my last comment. so this can be updated in wordpress as well. I will create an updated patch that reflects these changes.

15 years ago

upgrade of phpass to version 0.2

#4 @hakre
15 years ago

second patch is upgrading to the actual version. one part of the file has been untouched:

$this->random_state = microtime() . (function_exists('getmypid') ? getmypid() : '') . uniqid(rand(), TRUE);

The original code does not check for getmypid here. I assume this check is made because some hosters might disable that function. I suggest to put that into pluggable or into some other layer sothat the class can be kept untouched and the fix is provided globally.

#5 @hakre
15 years ago

forgot to mention. original code is:

$this->random_state = microtime() . getmypid();

#6 @hakre
15 years ago

  • Keywords has-patch tested added; needs-patch removed

#7 @dd32
15 years ago

$this->random_state = microtime() . (function_exists('getmypid') ? getmypid() : ) . uniqid(rand(), TRUE);

IMO, the usage of getmypid() should be removed entirely, Its not a reliable source of crypto-safe random data.. This is due to Process ID's having weak entropy..

To quote the PHP Manual even: "Process IDs are not unique, thus they are a weak entropy source. We recommend against relying on pids in security-dependent contexts."

I'd support removal of the getmypid() branch all together, and rely upon uniqid(rand(), TRUE) instead, as its going to be actually random.

#8 @hakre
15 years ago

Yeah I read that as well in the manual. I will update the patch and propose the changes upstream.

15 years ago

Updated for compability reasons

#9 @westi
14 years ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

Don't want to change this late in the dev cycle.

#10 @westi
14 years ago

  • Owner set to westi
  • Status changed from new to accepted

#11 @ryan
14 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.