Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118299
MIME-Version: 1.0
References: <CABTDVhC4J8WuKj4SdVK6hbZX2k7qrPDVCQ2g1Yb_4k4Hf5MEQQ@mail.gmail.com>
 <aa395f78-aebe-d52c-a419-aa0effeb64df@bastelstu.be>
In-Reply-To: <aa395f78-aebe-d52c-a419-aa0effeb64df@bastelstu.be>
Date: Mon, 25 Jul 2022 20:14:09 +0900
Message-ID: <CABTDVhDC6K37N6E_wJxzyrqSX2RsPL-OryW6BOOApUD2W_e94Q@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>, 
	PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000009282e05e49f49c8"
Subject: Re: [PHP-DEV] What do you think CSPRNG in PHP
From: zeriyoshi@gmail.com (Go Kudo)

--00000000000009282e05e49f49c8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2022=E5=B9=B47=E6=9C=8817=E6=97=A5(=E6=97=A5) 6:33 Tim D=C3=BCsterhus <tim@=
bastelstu.be>:

> Hi
>
> On 7/15/22 17:54, Go Kudo wrote:
> > However, there are several challenges to this.
> >
> > - Increased maintenance costs
> > - Requires optimization for CPU architecture
> > - Requires familiarity with CSPRNG
> >
> > PHP already bundles xxHash and appears ready to make this happen.
> >
> > Also, an appropriate CSPRNG implementation may be able to resolve the
> > current complex macro branching.
> >
> > What do you think about this?
>
> This would be a strong no from my side. There's all types of failure
> modes that decrease the security of the CSPRNG (i.e. making it insecure)
> and we really don't want to be the ones to blame if something goes
> wrong. And historically many non-kernel CSPRNGs later proved to be
> insecure in specific situations.
>
> I also would assume that for a typical PHP application both of the
> following is true:
> - The majority of the requests don't need any randomness.
> - The majority of the requests that *need* randomness don't need any
> significant amount of randomness.
> - The majority of the requests that need significant amounts of
> randomness are fine with a regular PRNG (e.g. Xoshiro or Pcg).
> - The cost of a few getrandom() syscalls is not really measurable
> compared to the time spent waiting for the database, file IO or template
> rendering.
>
> Attempting to optimize the speed of the CSPRNG is premature
> optimization. That also the reason why I suggested to use the 'Secure'
> engine by default in the Randomizer: It's a safe default choice for the
> vast majority of users.
>
> Personally I likely wouldn't have merged the PR in question for the same
> reasons. But at least in that case glibc is at fault :-)
>
> Best regards
> Tim D=C3=BCsterhus
>

Hi Tim.

You are right. Implementing a CSPRNG on your own obviously increases
maintenance costs and security risks.

However, I still think the overhead of the getrandom syscall in a Linux
environment is significant and should be considered.

I would suggest deprecating mt_srand()/srand() and using php_random_bytes()
in sessions etc. for PHP 8.3 for better security.

However, as Nikita mentioned before, the overhead of the getrandom syscall
in a Linux environment is significant and this proposal may result in
performance degradation.

https://github.com/php/php-src/commit/53ee3f7f897f7ee33a4c45210014648043386=
e13

Therefore, I have created a PoC to buffer php_random_bytes. This has
resulted in a significant performance improvement.

https://github.com/zeriyoshi/php-src/tree/random_buf

```
# perf result of current implementation:
Samples: 120  of event 'cpu-clock:pppH', Event count (approx.): 30000000
Overhead  Command  Shared Object          Symbol
  32.50%  php      [kernel.kallsyms]      [k] preempt_count_sub
  30.00%  php      libc.so.6              [.] syscall
  18.33%  php      [kernel.kallsyms]      [k] do_el0_svc
   2.50%  php      php                    [.] execute_ex
   1.67%  php      [kernel.kallsyms]      [k] __this_cpu_preempt_check
   1.67%  php      [kernel.kallsyms]      [k] __uaccess_mask_ptr
   1.67%  php      [kernel.kallsyms]      [k] _raw_spin_unlock_irqrestore
   0.83%  php      [kernel.kallsyms]      [k] __arm64_sys_getrandom
   0.83%  php      [kernel.kallsyms]      [k] __kern_my_cpu_offset
   0.83%  php      [kernel.kallsyms]      [k] __mod_lruvec_state
   0.83%  php      [kernel.kallsyms]      [k] __mod_memcg_state
   0.83%  php      [kernel.kallsyms]      [k] __next_zones_zonelist
   0.83%  php      [kernel.kallsyms]      [k] arch_local_irq_restore
   0.83%  php      [kernel.kallsyms]      [k] arch_local_irq_save
   0.83%  php      [kernel.kallsyms]      [k] check_stack_object
   0.83%  php      ld-linux-aarch64.so.1  [.] 0x000000000000a260
   0.83%  php      php                    [.] php_random_bytes
   0.83%  php      php                    [.] syscall@plt
   0.83%  php      php                    [.] zend_hash_add
   0.83%  php      php                    [.]
zend_hash_graceful_reverse_destroy
   0.83%  php      php                    [.]
zend_string_init_interned_permanent

$ time ./sapi/cli/php -r 'for ($i =3D 0; $i < 65535; $i++){ random_bytes(4)=
;
}'

real 0m0.069s
user 0m0.025s
sys 0m0.044s

# PoC result:
Samples: 19  of event 'cpu-clock:pppH', Event count (approx.): 4750000
Overhead  Command  Shared Object      Symbol
  31.58%  php      [kernel.kallsyms]  [k] __flush_cache_user_range
  15.79%  php      php                [.] _efree
  10.53%  php      php                [.] zend_register_functions
   5.26%  php      [kernel.kallsyms]  [k] __rcu_read_unlock
   5.26%  php      [kernel.kallsyms]  [k] page_add_file_rmap
   5.26%  php      [kernel.kallsyms]  [k] pfn_valid
   5.26%  php      [kernel.kallsyms]  [k] preempt_count_sub
   5.26%  php      libc.so.6          [.] cfree
   5.26%  php      libc.so.6          [.] 0x0000000000097b40
   5.26%  php      php                [.] execute_ex
   5.26%  php      php                [.] zend_register_constant

$ time ./sapi/cli/php -r 'for ($i =3D 0; $i < 65535; $i++){ random_bytes(4)=
;
}'

real 0m0.016s
user 0m0.009s
sys 0m0.007s
```

I think this is a safe implementation due to the nature of CSPRNG, what do
you think?

Best Regards,
Go Kudo

--00000000000009282e05e49f49c8--