Array assignment fails when the array has been resized in error handler

[m4p1e]

Description

The simple reproduction code as follows.

<?php
$array = range(0, 7);

class helper{
  public $a1;
  public $a2;
  public $a3;
  public $a4;
  public $a5;
  public $a6;
  public $a7;
  public $a8;
  public $a9;
  public $a10;
  public $a11;
  public $a12;
  public $a13;
  public $a14;

  public function hello() {
    echo "maple";
  }
}
set_error_handler(function($err, $msg) {
  global $array;
  global $helper;
  $array[] = 1; // force resize
  $helper = new helper();
});
 
function crash() {
  global $array;
  global $helper;
  $array[0] = $var; // undefined notice
  $helper->hello();
  $helper->$a1 = 1337;
}
 
crash();

Resulted in this output:

crashed for segmentfault.

But I expected this output instead:

no crash.

This has been an issue for years (at least I learned about it 4 years ago). I wrote a paper for exploiting this as a security vulnerability.

PHP Version

PHP 8.3.3

Operating System

Ubuntu 20.04

[iluuu1994]

Hi @m4p1e. Thank you for your report. We are aware of various such issues, but they are unfortunately fundamentally hard to solve for all cases. My latest attempt is here: #12805 Luckily, such issues only happen in code that is seemingly explicitly crafted to cause it.

I haven't read your paper yet, but I will spend some time on it.

[m4p1e]

Hi @m4p1e. Thank you for your report. We are aware of various such issues, but they are unfortunately fundamentally hard to solve for all cases. My latest attempt is here: #12805 Luckily, such issues only happen in code that is seemingly explicitly crafted to cause it.

I haven't read your paper yet, but I will spend some time on it.

Thank you for your prompt response. In my paper, I've stated, "This problem is not destined to be easily resolved." Why do I care about it? I've been deeply involved in a project concerning the PHP sandbox for running untrusted PHP code. This issue fundamentally breaks the sandbox, hence we have to disable the function set_error_handler. Thanks again for letting me know that there is a possible solution. I will check it soon. I also provide a possible fix in my paper. It involves modifying the existing interpretation process of array assignement or fecthing to support multi-index in one opline. It's the most direct approach in my opinion.

[iluuu1994]

Creating multidimensional DIM opcodes would solve this problem, but not others.

$foo->bar['baz'] = $baz;

This issue is very similar.

0000 V2 = FETCH_OBJ_W (dim write) CV0($foo) string("bar")
0001 ASSIGN_DIM V2 string("baz")
0002 OP_DATA CV1($baz)

V2 is an indirect value to the bar property. If ASSIGN_DIM still emits the "Undefined variable" warning before the offset is assigned, the error handler can still change the zval pointed to by V2. For example:

class Foo {
    public $bar = [];
}

$foo = new Foo();

set_error_handler(function () {
    global $foo;
    $foo->bar = null;
});

$foo->bar['baz'] = $baz;
var_dump($foo);

Compile php-src with --enable-address-sanitizer and then run with USE_ZEND_ALLOC=0 php test.php.

The error handler swaps the zval pointed to by V2 on-the-fly, after we have normally already established the indirect value to be an array. However, it now contains an unexpected null value. There's a plethora of similar issues. Destructors are also very similar. (Edit: Actually, looking at ASSIGN_DIM, the problem is probably that the array is freed before it is assigned to, because fetching of the "container" operand happens before the fetching of the assigned value, i.e. op_data).

The approach from #12805 tries to delay the emission of warnings to a safe point in time instead. More specifically, this is when the current opcode no longer accepts indirect values. The PR is relatively straight-forward. However, it is unfortunately not compatible with the JIT, and I did not invest more time to try and fix that, because the JIT is extremely complicated and I already invested too much time on this theoretical issue. There are also other challenges, namely manually identifying exactly what warnings need to be delayed.

[m4p1e]

Creating multidimensional DIM opcodes would solve this problem, but not others.

You're right. I missed the cases in object assignment and fetch.

The approach from #12805 tries to delay the emission of warnings to a safe point in time instead.

I was thinking of exposing all possible warnings as early as possible to make sure they don't affect following operations, which is exactly the opposite of your approach.

The PR is relatively straight-forward.

I agree. I have closely reviewed your PR, and it is great. In particular, I like the idea of delaying the warnings from sequential fetches until their results have been used.

However, it is unfortunately not compatible with the JIT, and I did not invest more time to try and fix that, because the JIT is extremely complicated and I already invested too much time on this theoretical issue.

The reason I'm reporting this issue is also because of JIT. With the rapid development of JIT technology in PHP today, this issue may become increasingly challenging to fix, and in turn, may hinder the progress of JIT technology in PHP. So, I think it should be fixed as soon as possible.

There are also other challenges, namely manually identifying exactly what warnings need to be delayed.

That shouldn't be very difficult to just fix the user error handler-related issues. But if the delayed error is introduced as a more general mechanism in PHP, its implementation needs more discussion.

[iluuu1994]

I was thinking of exposing all possible warnings as early as possible to make sure they don't affect following operations, which is exactly the opposite of your approach.

That's exactly right. All the problematic opcodes are listed here:

198b22a#diff-773bdb31563a0f907c75068675f6056b25f003e61f46928a31d9837ae107460dR8103-R8113

They can be combined in all kinds of ways, so I believe creating compound opcodes is not a viable option.

Feel free to pick this PR back up and try to add JIT support. I won't have time in the immediate future, but I'm happy to discuss/help out where possible.