Solving EasyRFI via new EasyRFINotice severity

[ScreamingDev]

RFI would be:

require $_POST['x'];
require $foo;
require SOME_BAR_CONST;

After a short search I found no solution but what about this?

require parse_url( $foo, PHP_URL_PATH );

It would be great if the EasyRFISniff has some kind of "EasyRFINotice" severity when the dynamic includes seem to be escaped like this.

[jrfnl]

@ScreamingDev Interesting idea, I've just been looking at that sniff for other reasons.

require parse_url( $foo, PHP_URL_PATH );

For the example you've posted, I still wouldn't consider that safe in any imaginable way - think:

$foo = 'http://example.com/.htaccess';
require parse_url( $foo, PHP_URL_PATH );

require SOME_BAR_CONST;

However, path constants defined within the application could well be safe and it would be a nice option for the sniff to allow for a whitelist of such safe application path constants.

These whitelists could possibly be predefined for the supported frameworks and enhanced by the ruleset used via setting a sniff property in a custom phpcs.xml.dist ruleset.

What do you think ?

[ScreamingDev]

I neither consider this a very safe method. I would say (in general):

• Sniffs could have a third level called "notice" (besides the existing "error" and "alert") to indicate that there is hardening.
• Paranoia-Sniffs should have notices so that devs can exclude them (after they understood the problem)

RFI is an example because dynamic includes are in almost any code out there and anyone using it would get:

• PHPCS_SecurityAudit.Security.BadFunctions.EasyRFI.ErrEasyRFI error for require $_POST['x'] which is good because this is simple to attack
• PHPCS_SecurityAudit.Security.BadFunctions.EasyRFI.WarnEasyRFI warning for require SOME_PATH_CONST (or variables) which is also okay-ish because those are "harder" to attack.
• PHPCS_SecurityAudit.Security.BadFunctions.EasyRFI.Notice notice that follows some (still possibly insecure) hardening already.

Now a developer is empowered to exclude this severity from his sniffs:

<rule name="PHPCS_SecurityAudit.Security.BadFunctions.EasyRFI">
  <exclude name="PHPCS_SecurityAudit.Security.BadFunctions.EasyRFI.Notice">
</rule>

After the developer (hopefully) understood that this is just hardening because:

$foo = 'http://example.com/bring/me/to/document_root/.git';
echo parse_url( $foo, PHP_URL_PATH ); // "/bring/me/to/document_root/.git"

would shift the RFI problem to an exploit problem.

[jrfnl]

Sniffs could have a third level called "notice" (besides the existing "error" and "alert") to indicate that there is hardening.

That's not an option. PHPCS only has warnings and errors. There is no notice level nor any intention to add one.

The only thing which could be done here is to play with the severity level of messages, but that would need very strict guidelines and documentation to prevent it from becoming a total mess.

If that road would be taken, it could (should) probably replace the paranoiaMode configuration setting as well.

Paranoia-Sniffs should have notices so that devs can exclude them (after they understood the problem)

This is already possible and unrelated to the Security standard.

Every individual sniff errorcode can be excluded for a complete project by using a custom ruleset with an <exclude name="..."/> directive as per your example above.

For individual issues inline ignore comments can be used to whitelist certain code // phpcs:ignore Security.Category.SniffName.ErrorCode -- for reasons.

[ScreamingDev]

This is correct - I am well aware about it:

Every individual sniff errorcode can be excluded for a complete project by using a custom ruleset with an directive as per your example above. For individual issues inline ignore comments can be used to whitelist certain code // phpcs:ignore Security.Category.SniffName.ErrorCode -- for reasons.

But I would not use one of these methods to exclude

PHPCS only has warnings and errors

because I want to be aware that something is missing there.

• Supressing leaks in general <exclude name="..."/> is not an option (in this example) as we both know - I guess
• Supress for file neither is an option because it is like the general exclusion and allows leaks as soon as the file changes and the dev team forgets about the phpcs:ignore .
• Supressing via // phpcs:ignore is okay but devs would need to place this everywhere after investigation and fixing it which would lead to tons of comments

So a third way in between could be a solution. However this may work for you - as I don't know the code of phpcs-security-audit so well. But now I know that a notice-like level is no option so I hope someone comes up with a good and simple solution for the severity . Thanks for talking about this. Fingers crossed :)

[jmarcil]

A little bit of history of how things work in this tool.

First of all, it clearly says on the README that it is a generator of false positive 😅. It's very verbose by default. ParanoiaMode is turned on by default for that reason.

From that point of view, all WARNINGs are essentially NOTICEs that should be quickly dismissed by anyone doing a review. Think of is as an assistant for secure code review that helps you pointing to line of codes you want to see. It's effectively a tool to point to risky functions. If you see an ERROR, then it means that this is most certainty a problem and you should review that first.

If you want to use it so that it's not that verbose, and only return issues that the certainty is high, you have to set ParanoiaMode to off.

Now, fast forward 10 years later.

People are actually running this in continuous environment, and thus I'm seeing a bigger need for suppression and false positive diminution than before. Their goal is not to run it one time to assist a review, but rather get the amount of warning/error to 0 at some point, showing that they considered all risks.

Back to the issue at hand.

I believe that what @ScreamingDev is asking is about reducing false positives in a way that the end user can signify to the tool that this code is actually "safe".

In the tool, a function that serve to add some security control value (either explicitly in the function or by a side effect) is a called a mitigation. Having the tool know about those mitigations facilitate the reduction of false positive.

Right now the tool only supports built-in mitigations as you can see in https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/b47f8a3ef359a78d4f88212dd9dcf9c926496c1e/Security/Sniffs/Utils.php#L236 and is also very limited (there's almost no reduction of false positive yet).

The "EasyRFINotice" and it's exclusion proposed above is effectively a WARNING that disappear when a mitigation function is present.

The proper solution for that would be to have user defined mitigation function per rule. We would also need to extend mitigation to not just a function but also parameters.

In your config it would look like this:

    <rule ref="Security.BadFunctions.EasyRFI">
        <properties>
            <property name="mitigationfunctions" value="parse_url"/>
        </properties>
    </rule>

And of course, if you want to actually have parse_url built in as part of the rule, then ParanoiaMode set to off will allow you to suppress it (and any other mitigation functions). You could also set it per rule like this:

    <rule ref="Security.BadFunctions.EasyRFI">
        <properties>
            <!-- Comment out to follow global ParanoiaMode -->
            <property name="forceParanoia" value="0"/>
        </properties>
    </rule>

Hope this clarify a bit.

[jmarcil]

oh and phpcs-security-audit is a line by line scanner.

require parse_url( $foo, PHP_URL_PATH );

is as bad as any T_VARIABLE passed to require. because you have no clue what is in $foo.

Exclude this at your own risk.

[ScreamingDev]

Thanks for that idea and all the background @jmarcil . I loved it!

My first impression is: For the RFI example here the mitigation functions would be a thing.

On second thought I knew that it is a line-by-line- or toker-by-token-parser which makes it hard to understand the context and react to it. So the mitigation may allow small mistakes like parse_url( $foo, PHP_URL_HOST ) (due to a silly autocomplete) which may not be what the security audit wants to allow.

Anyway I hope to see some workaround for that some day. Until then using

@phpcs:ignore Security.BadFunctions.EasyRFI -- escaping via parse_url is okay

solves it and (in addition) shows that someone actually reviewed this part.