Explanation of issues

[janmasarik]

Hey,

I was wondering if there is any plan to add explanation of found issues to this project.

For example, we don't really understand why Function array_filter() that supports callback detected is detected as vulnerability. (Identifier:PHPCS_SecurityAudit.BadFunctions.CallbackFunctions.WarnFringestuff)

Is similar feature planned somewhere in the future?

[pavarnos]

It would be really helpful...
eg why does PHPCS_SecurityAudit.BadFunctions.EasyRFI report a warning if there is no '.' in the path...

[jmarcil]

Hey!

Thanks for your interest in this project. I am a big fan of documentation so please don't take this as rebuttal for the suggestion. I also think I could update those really old warnings with a few words to make them better.

That said, it would be somewhat impossible for the tool to document properly what you actually need in a one liner. This would require external documentation to be effective. So far the main goal of the tool has been to point out people to line of code that are at risk without really including the underlying explanation.

Let me answer you both and we'll see how hard it could be as one rule can actually means many things as well.

For array_filter(), it's somewhat "easy" once you know attack examples:

Textpattern tag system allows for code execution
https://nealpoole.com/blog/2011/05/multiple-major-security-vulnerabilities-in-textpattern/

Callback Function Execution Code
https://totalwebsecurity.net/php-security/methods-code-execution/

So simply put, when you can write the callback, you can execute code, so it's bad to have it from a user input because an attacker can then choose what to call and execute.

For EasyRFI, I'm not sure what you mean by no '.' in the path. Is the path a dynamic parameter? This tool only check the static values, on a line per line basis, so it will never know that the path doesn't have something risky (it could be an empty variable, it will still return the warning). If you're thinking of a remote URL that doesn't require a dot in it, I can think of two hacks that would be possible: ipv6 url and using a local network hostname that has an open redirect that you URL encode the params of.

Note that after reading the rule code itself, I don't see why it would be only RFI because it could also mean LFI.. so path traversal could also be betting wording. But you would still need to do some research to figure out what path traversal is!

I think the bottom line here is that interpreting those results need some kind of expertise. The good news is that if you want to save time on figuring out why.. just simply make your code so it will not trigger the rules in the first place. Don't use a variable with user input in it for the callback of array_filter and same for any includes.

So again, the main goal of that tool is to either point someone who know what's going on quickly into the risky lines of codes, or have people ask questions like you did until they figure out themselves what is going on. And I hope you just did! 😄

[jmarcil]

Oh and if you guys want to help, just list here all the warnings that are unclear to you and I'll try to see if I can change the wording a bit so it becomes more palpable in 2018.

Thanks!

[pavarnos]

Thanks for the quick reply. This package has the potential to be a really useful and popular tool, especially with a bit of extra documentation / explanation (even if it is just comments in the code)

When I run phpcs_security_audit on my index.php

// snip
require_once dirname(__DIR__) . '/src/Constant.php';
// snip

i get the following log

----------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 1 LINES
----------------------------------------------------------------------------------------------------------------------------------------------------
 28 | WARNING | Possible RFI detected with dirname on require_once (PHPCS_SecurityAudit.BadFunctions.EasyRFI.WarnEasyRFI)
 28 | WARNING | Filesystem function dirname() detected with dynamic parameter
    |         | (PHPCS_SecurityAudit.BadFunctions.FilesystemFunctions.WarnFilesystem)
 28 | WARNING | Possible RFI detected with __DIR__ on require_once (PHPCS_SecurityAudit.BadFunctions.EasyRFI.WarnEasyRFI)
----------------------------------------------------------------------------------------------------------------------------------------------------

In Paranoia mode

• https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/master/Security/Sniffs/BadFunctions/EasyRFISniff.php#L45 appears to be looking for a '.' character?
• and __DIR__ is not dynamic? Or what is the RFI problem with __DIR__? (some documentation would help us learn!)

thanks!

[pretzlaw]

We also tried this and have similar issues.
First of all there are easy things like // phpcs:ignore PHPCS_SecurityAudit.BadFunctions.CryptoFunctions.WarnCryptoFunc which marks the problem as ok-ish.

But we don't see any testing here. If you start unit-testing then have a look at https://github.com/pretzlaw/phpunit-docgen which takes the UnitTest doc comments and merges them into one big documentation.

Are the tests somewhere else or is it just "test.php" ?

[vv12131415]

why I should not use hash function?
I mean I'm warned with Crypto function hash used.

[jmarcil]

You guys are asking the right questions 😄!

I'll reiterate on what I said as the goal of this project is not education nor to replace security consulting. The main reason is that since this tool and its documentation have no context, it could falsely give advice where you think you are safe while you are not.

One of its main goal is to point out code that you should ask questions about, and by asking those here, looks like the tool is fulfilling that goal 😃.

So far I don't see any questions or call out about a certain finding message that can be made better following the discussion here. So I'll close this issue. Maybe in the future I'll explain the tool philosophy in the README, so that people understand why it doesn't say more.

That said, I'll still answer your questions here, but I'm haven't worked in PHP myself since a while now, so my thinking might even been outdated. That's why the tool is still relevant btw, because the assumptions are simple and the details are low, so they have less chances of changing over time.

@pavarnos the '.' character indicate a string concatenation in PHP. that just means that the following won't proc : include('aaa' . 'aa.php')'; since it's just a static call, but any other thing (such a function or object call) will proc the rule. Don't get that '.' mistaken by the actual RFI payload 😉.

@pavarnos ___DIR___ is not recognized by this tool. so for it it's an unknown token that can be anything, so it's reporting on it. In some crazy code, maybe that __DIR__ can be overridden somewhere else, and then leads to an actual problem? The tool leaves that reflection to the user as it can be complicated. If your answer is "it's like hard-coded for us and there's no variable around it", then that's a false positive for you!

@pretzlaw all the tests are in tests.php. I don't know if you can really unit test, since the tests file is not correct PHP, but rather a collection of PHP tokens that the phpcs parser looks at. BTW I do like your way of using phpcs:ignore for removing bad finding results! That's a feature of phpcs I never knew.

@vladyslavstartsev A warning doesn't mean that you should not use it, it means that it's a security sensible function that you should think about.

Hope this help you all! Normally I would have sent you to further reading on OWASP, but most of the OWASP stuff I've found was abandoned. There's still an awesome list about it https://github.com/guardrailsio/awesome-php-security that could probably point you to good resources in PHP security.

[ScreamingDev]

Please write down your thoughts somewhere when you implement a new rule or so.
We like to know the things that are currently only inside your head.

[jmarcil]

@ScreamingDev You're bringing a valid point that was reflected in most of the comments of this thread but wasn't addressed directly by myself, or at least was set as an initial non-goal when I first made that tool (now a couple years ago).

That said, I edited your comment to remove an unnecessary sentence as I won't tolerate any non constructive behavior. Note that this would deter some people from accepting your point of view, even if it's a well justified one.

Normally security scanning tools provide a documentation that help the user into understanding what are the findings. Now that this tools is more popular, it gets in the hand of people that don't have a deep understanding of security vulnerabilities and probably need to guide them to understanding what is going on, and even fixing it.

There's a model I really like that I think should be the goal for this project:
https://find-sec-bugs.github.io/bugs.htm
https://security-code-scan.github.io/#Rules

I'm creating this issue #44 and still keep this one closed to just go with something clearer in the future.

The current thread actually requested in-results or in-code documentation, but I think the outside mini-docs format would be the best to solve the current issue. They can explain more, link to outside docs, and be updated without needed to re-release or update the tool.

I'll need help from the community, as like I said in a comment above, I'm not in touch with the PHP world anymore, so I actually don't know if my knowledge is still valid nowadays. Probably for most of it, but I'll definitely need knowledgeable people to review details because they are important for security.

Bad advice is worse than no advice, and I'll do my best to have this project be provide correct guidance, because clearly right now it does the exact opposite.

Thanks again to everyone who commented in this thread, it's great to see interest and traction around this project!