codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.39k stars 1.9k forks source link

Bug: random_string() Implicit conversion from float 4.5 to int loses precision #6330

Closed turbopixel closed 2 years ago

turbopixel commented 2 years ago

PHP Version

8.1

CodeIgniter4 Version

4.2.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux Debian

Which server did you use?

fpm-fcgi

Database

MariaDB

What happened?

I'm using the codeigniter build in helper helper('text'); and want to create a random string called the function random_string("crypto", 9); With an odd number I always get the error: Implicit conversion from float 4.5 to int loses precision. So the numbers 3, 5, 7, 9, 11 (...) are not usable. With even numbers I always get a random string.

What function is called?

if (!function_exists('random_string')) {
  /**
   * Create a Random String
   *
   * Useful for generating passwords or hashes.
   *
   * @param string $type Type of random string.  basic, alpha, alnum, numeric, nozero, md5, sha1, and crypto
   * @param int $len Number of characters
   */
  function random_string(string $type = 'alnum', int $len = 8) : string {}
}

This case I call with random_string("crypto", 9):

return bin2hex(random_bytes($len / 2));

Steps to Reproduce

Didn't work:

helper('text');
$pw = random_string("crypto", 9);

Works fine:

helper('text');
$pw = random_string("crypto", 8);

Expected Output

A random string like b3da1087d or something else.

Anything else?

Last two Backtrace rows:

  1. {PHP internal code} — CodeIgniter\Debug\Exceptions->errorHandler ()
  2. SYSTEMPATH/Helpers/text_helper.php : 576 — random_bytes()

grafik

Solution maybe

Cast the result to an integer:

return bin2hex(random_bytes((int)($len / 2)));
kenjis commented 2 years ago

Thank you for reporting!

It seems random_string("crypto", 9) is a misuse. Because it is impossible to generate strings with the length of 9. The output length is 8 if you use PHP 8.0. You must set an even number in the second parameter.

So we should throw an InvalidArgumentException?

turbopixel commented 2 years ago

I see it differently. The problem is that the function random_bytes() expects an integer, but gets a float. So it would be right (In my opinion) to round up the number or cast it as int.

paulbalandan commented 2 years ago

@turbopixel You're correct in a way that the float to int conversion is the immediate problem which can be patched by simply rounding up the float or discarding the decimal thru casting to int. But, if you'll look at the design deeper, the $len is first halved before being passed to random_bytes which accepts an int. Since dividing an integer by 2 gives either an integer (even) or float (odd) and the consuming function needs an integer, I think it should be right to assume that we should pass only even numbers in the $len in the first place.

On the other hand, another way is to just pass the $len argument as-is. Why does it need to be halved, anyway?

turbopixel commented 2 years ago

@paulbalandan The secret will probably be the PHP function itself. In the example you can see that a length of 5 is specified and the return value is 10 characters long. This will probably be the reason why the originally specified length is divided by 2.

https://www.php.net/manual/en/function.random-bytes.php#refsect1-function.random-bytes-examples

Now, there are two options:

  1. The specified length is given directly (without / 2) and the return value of bin2hex() is shortened afterwards.
  2. @kenjis idea will be implemented.