Project

General

Profile

Actions
Copy link

Bug #12801

closed
VG VG

User password hashes pseudo-random number generator may return insecure salt value

Bug #12801: User password hashes pseudo-random number generator may return insecure salt value

Added by Viktor Gurov over 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Viktor Gurov
Category:
Authentication
Target version:
2.7.0
Start date:
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
22.05
Release Notes:
Default
Affected Version:
2.6.0
Affected Architecture:

Description

https://github.com/pfsense/pfsense/blob/master/src/etc/inc/auth.inc#L819:

$salt = substr(bin2hex(openssl_random_pseudo_bytes(16)),0,16);

openssl_random_pseudo_bytes(16) omits the strong_result parameter (null)

from https://www.php.net/manual/en/function.openssl-random-pseudo-bytes.php:

Parameters 

length
The length of the desired string of bytes. Must be a positive integer. 
PHP will try to cast this parameter to a non-null integer to use it.

strong_result
If passed into the function, this will hold a bool value that determines 
if the algorithm used was "cryptographically strong", e.g., safe for usage with GPG,
passwords, etc. true if it did, otherwise false

must be:
openssl_random_pseudo_bytes(16, true)

JP Updated by Jim Pingle over 4 years ago Actions
Copy link
 #1

That second parameter needs to be a variable -- it's not a flag telling it to use a secure method, it's a variable which returns true/false based on whether the underlying result came from a secure source. This may already be true which makes this a bit of a non-issue, but we should check and test it. If it contains false after the call we should use an alternate method of obtaining randomness like random_bytes().

VG Updated by Viktor Gurov over 4 years ago Actions
Copy link
 #2

  • Subject changed from User password hashes use an insecure pseudo-random number generator to User password hashes pseudo-random number generator may return insecure salt value

Jim Pingle wrote in #note-1:

That second command needs to be a variable -- it's not a flag telling it to use a secure method, it's a variable which returns true/false based on whether the underlying result came from a secure source. This may already be true which makes this a bit of a non-issue, but we should check and test it. If it contains false after the call we should use an alternate method of obtaining randomness like random_bytes().

It's better to use random_bytes() than to try N rounds of openssl_random_pseudo_bytes() until strong_result == true.

JP Updated by Jim Pingle over 4 years ago Actions
Copy link
 #3

That is likely the better choice overall.

VG Updated by Viktor Gurov over 4 years ago Actions
Copy link
 #4

  • Assignee set to Viktor Gurov

JP Updated by Jim Pingle over 4 years ago Actions
Copy link
 #5

  • Status changed from New to Pull Request Review
  • Target version set to 2.7.0
  • Plus Target Version set to 22.05

VG Updated by Viktor Gurov over 4 years ago Actions
Copy link
 #6

  • Status changed from Pull Request Review to Feedback
  • % Done changed from 0 to 100

Applied in changeset commit:961f240c18f8421b0a28ee192ffa041e754e8f8e.

JP Updated by Jim Pingle over 4 years ago Actions
Copy link
 #7

  • Status changed from Feedback to Resolved

The correct function is in place now and working properly.

JP Updated by Jim Pingle over 3 years ago Actions
Copy link
 #8

  • Private changed from Yes to No
Actions
Copy link

Also available in: Atom