WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#10727 closed defect (bug) (fixed)

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

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

Description

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 5 years ago.
10727.2.patch (1.1 KB) - added by hakre 5 years ago.
upgrade of phpass to version 0.2
10727.3.patch (1.1 KB) - added by hakre 5 years ago.
Updated for compability reasons

Download all attachments as: .zip

Change History (14)

hakre5 years ago

comment:1 hakre5 years ago

  • Keywords has-patch added

comment:2 hakre5 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.

comment:3 hakre5 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.

hakre5 years ago

upgrade of phpass to version 0.2

comment:4 hakre5 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.

comment:5 hakre5 years ago

forgot to mention. original code is:

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

comment:6 hakre5 years ago

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

comment:7 dd325 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.

comment:8 hakre5 years ago

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

hakre5 years ago

Updated for compability reasons

comment:9 westi4 years ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

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

comment:10 westi4 years ago

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

comment:11 ryan4 years ago

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