[php2cpg] feat: add multi array dimension assignment support

[TNSelahle]

• Add multi array dimension assignment support
• Fix tmp var numberings not being unique when a tmp variable is generated while in a block scope

Closes https://github.com/ShiftLeftSecurity/codescience/issues/8691

[TNSelahle]

@ml86 Since the PHP parser doesn't provide code to use for populating the CODE property, getting this became a bit tricky in this implementation. See codeForSingleArrayElemAssign and codeForMultiArrayDimAccess in AstCreatorHelper.scala

For an expression like $arr[$expr][], I generated the asts for the array access dimensions simply to get the code, which is a waste. However, I couldn't think of a better way to do this. An improvement would be to store some sort of cache for the code mapping a PhpExpr node to its generated code so it can be reused in other places.

[johannescoetzee]

This lowering is fine in the case where $xs does not already exist, but I think it could be problematic if $xs[1][2] is an array that was already tainted. This step plus the next would then overwrite the tainted array.

The lowering in general looks to be the other way around from what we discussed, but I also wasn't involved in later discussions. Did the plan change after we last spoke?

[TNSelahle]

Hmm, do you mean in that case, the previous taint information would be lost?

Regarding the lowering, the plan initially didn't change but when I started with the implementation, it was a bit more intuitive implementing it going top-to-bottom instead of bottom-to-top.

[ml86]

The lowering looks good to me. If an array is tainted with a wildcard access path the assignment $xs[1][2]=... does not remove the taint.

[johannescoetzee]

For $xs[][2][] = $val; this would be fine, but for $xs[1][2][] = $val we run into an issue. As it is lowered here, the first couple of assignments effectively reduce to

$xs[1] = array(2 => $val)

which means if we had something like $xs[1] = array(1 => "bad data"), that first element would be removed after the lowered assignment above.

[johannescoetzee]

Edit to add the two specific lines in the lowering I'm concerned about:

assignTwo.code shouldBe "$foo@tmp-1 = array($foo@tmp-0)" // effectively = array(val)
assignFour.code shouldBe "$xs[1] = $foo@tmp-3"           // $foo@tmp-3 == array(2 => foo@tmp-1)

To give a concrete example, it's the difference between

<?php

$xs = array(array(1 => "bad"));

$xs[0][2] = "good";

var_dump($xs);

// output
array(1) {
  [0]=>
  array(2) {
    [1]=>
    string(3) "bad"
    [2]=>
    string(4) "good"
  }
}

and

<?php

$xs = array(array(1 => "bad"));

$xs[0] = array(2 => "good");

var_dump($xs);

// output
array(1) {
  [0]=>
  array(1) {
    [2]=>
    string(4) "good"
  }
}

[johannescoetzee]

I've been thinking some more and I think I didn't really understand the implications of using wildcard access paths for taint. If we're just using wildcards, then

xs[0][2] = ...

and

xs[0] = array(2 => ...)

should be equivalent since from the DF engine's perspective, it's unknown what's being overwritten in any case.

[TNSelahle]

Very good catch! I missed this. This poses a problem anytime an index in the chain is pointing to an array. Whoops.

[ml86]

Ok, now I see your point @johannescoetzee. I think we can relatively easy fix this buy just emitting a normal access operator chain for every up to the first [] from where on we know that no old taint can be overwritten.

[johannescoetzee]

A test for something like $xs[1][][2] not ending with a dimensionless index access is also missing.

[johannescoetzee]

I'm guessing these changing are due to the duplicate ASTs being generated for the code? I do think we should find another option there. Duplicating work generating the AST isn't ideal, especially if it's only for the code field

[TNSelahle]

These are changing due to my updates to the getNewVarTmp method in Scope. The method previously used the counter from the top scope if it was a method-, class- or namespace- scope. If the top scope was a block scope, a new counter would be used and but the tmp var name would still be dependent on the enclosing method, type, or namespace name. So then it was possible to get a name that wasn't unique.

[ml86]

Please separate the temp variable name fix into a separate PR.

[johannescoetzee]

This is also not correct. This is saying that $val appears as the last child in the method body, but that should not be the case

[TNSelahle]

The whole assignment expression is represented by the BLOCK node and so I've made the $val the last child of that. I think you may have confused that block as being the method block.

[johannescoetzee]

You're right, I did miss one of the astChildrens in the query.

[ml86]

Lets avoid creating a temp variable also for the right hand side of the assignment. We already create one for each array invocation which makes the lowering complicated enough.

[ml86]

Use the right hande side of assignment in this case $val as last identifier of the block.

[TNSelahle]

The Ast for the right-hand side of the assignment is generated somewhere within the recursive method traverseArrayDimExpr. I didn't want to over-complicated the implementation. I initially called astForExpr for the RHS just before creating the Ast for the block but then since the RHS expression gets passed into traverseArrayDimExpr, astForExpr will be called again on the RHS expression.

I assign it to a tmp variable since calling astForExpr twice on the same PhpVariable isn't as bad as doing it on whatever complexion expression might be on the RHS