[PHP] Improve Echoed Requests

[shellz-n-stuff]

Description

This PR aims two improve the echoed requests PHP injection rule by doing the followings:

1. Splitting out Print and Echo into seperate rules to allow for Fixes/Autofixes
2. Adding additional sanitisation functions to reduce the number of false positives associated with the rules
3. Remove a false negative associated with "empty" (more on this below)

False Negative

The empty function by itself isn't really a sanitiser as it returns a boolean output rather than an empty string. In theory we could add a sanitiser similar to:
pattern: if(empty($...VARS))

However, this is likely to be just as noisy/false negative heavy. There may be a smart way to check this with metavariable comparisons. If someone wants to try further improve this rule.

Notes

I couldn't find any way to use fix-regex to keep these rules combined but if would love to simplify this if you have any suggestions.

[CLAassistant]

All committers have signed the CLA.

[shellz-n-stuff]

Arguably isset should also be removed as a sanitisation function.

[kurt-r2c]

@shellz-n-stuff I suspect isset is a sanitizer here because the return type is a boolean and standard propagation would consider the return value tainted (which is false, as it's a boolean not the user input)

[shellz-n-stuff]

@shellz-n-stuff I suspect isset is a sanitizer here because the return type is a boolean and standard propagation would consider the return value tainted (which is false, as it's a boolean not the user input)

@kurt-r2c,

I'll update this rule later today to remove isset. I think a pattern like the below would be a fairly common false negative. I can't think of many cases where this would meaningfully increase false positives either

if(isset($VAR)){ echo $VAR; }

I'll also sort out the test failures.

[shellz-n-stuff]

@kurt-r2c updated 🙏

[kurt-r2c]

@shellz-n-stuff removal of isset here will cause false positives as the taint must flow through the function to the sink to be considered sanitized - the sanitizer is not by side effect here.

without isset this will (incorrectly) flag echo isset($VAR)
https://semgrep.dev/playground/s/JDold

the correct behavior is:
https://semgrep.dev/playground/s/5rz6W

note that even with the sanitizer we are still flagging line 13 because an unsanitized reference of $VAR makes it into the sink

Similarly, empty(...) should not be removed.

Please revert these changes and this should be good to merge.

[shellz-n-stuff]

@shellz-n-stuff removal of isset here will cause false positives as the taint must flow through the function to the sink to be considered sanitized - the sanitizer is not by side effect here.

without isset this will (incorrectly) flag echo isset($VAR) https://semgrep.dev/playground/s/JDold

the correct behavior is: https://semgrep.dev/playground/s/5rz6W

note that even with the sanitizer we are still flagging line 13 because an unsanitized reference of $VAR makes it into the sink

Similarly, empty(...) should not be removed.

Please revert these changes and this should be good to merge.

Howdy Kurt,

Happy to make the change, but I think in terms of optimising the rule having this as a valid sanitiser is far more likely to increase false negatives than false positives.

It strikes me as a fairly common coding convention to check a variable isn't null before echoing it out e.g.:

if(isset($VAR)){ 
  echo "your name is" + $VAR; 
}

Whereas the pattern you described where you are echoing out a function that returns a boolean seems less common/likely. Obviously I'm happy to make the change if you disagree as I firmly believe you're in a better position to assess the noise/data quality of these rules ❤️

[kurt-r2c]

@shellz-n-stuff totally understand your concern - you'll note that in the correct behavior with the isset sanitizer (https://semgrep.dev/playground/s/5rz6W) line 9 is flagged (as it should be) which exercises the if(isset(...)){...} pattern you are concerned about.

<?php

function minimalexample() {
    $VAR = $_GET['someval']
    if(isset($VAR)){ 
        // ok: echoed-request
        echo isset($VAR);
        // ruleid: echoed-request
        echo $VAR; 
        //ok: echoed-request
        echo isset($VAR) . "something";
        // ruleid: echoed-request
        echo isset($VAR) . $VAR;
    }

}

[shellz-n-stuff]

@shellz-n-stuff totally understand your concern - you'll note that in the correct behavior with the isset sanitizer (https://semgrep.dev/playground/s/5rz6W) line 9 is flagged (as it should be) which exercises the if(isset(...)){...} pattern you are concerned about.

<?php

function minimalexample() {
    $VAR = $_GET['someval']
    if(isset($VAR)){ 
        // ok: echoed-request
        echo isset($VAR);
        // ruleid: echoed-request
        echo $VAR; 
        //ok: echoed-request
        echo isset($VAR) . "something";
        // ruleid: echoed-request
        echo isset($VAR) . $VAR;
    }

}

Wow, so semgrep's taint tracker mode tracks assignment. That's absolutely unreal. I'll fix this now, I'm super interested in how y'all got that to work in pass by value vs pass by reference.

Apologies for my misunderstanding!

[kurt-r2c]

String interpolation tests are failing - investigating. Not sure why the removal of the parentheses cause this test to start failing.

applied suggestions via web UI

[shellz-n-stuff]

Oh good catch. Thank you @kurt-r2c, my apologies!