Security Fix: Argument of a function used in an include/require, could lead to File Inclusion #491
Conversation
… File Inclusion Error from semgrep audit.php.lang.security.file.inclusion-arg
|
I'm like @derrabus. I has several guesses and concerns about this PR, let me share them with you in case this helps in any way: I feel like this is something spotted by an automated security scanner. Here, the tool is missing that the method is private and is never called with arbitrary data. A quick look will indeed show you that we pass it only static data. The report is thus invalid. About the patch, supposing there would be a security issue, it doesn't fix anything since it reuses the argument without doing any filtering nor validation of it. If the tool doesn't spot this as a security anymore, then you'd better trash that tool. Then, such tools are just stupid automata. Please perform the analyses I just did before submitting issues in the future, that'll save time for everybody. And last but not least, you should know that sharing possible security issues publicly is a very much discouraged practice. Pretty much all serious open-source projects have a security policy in place that you can read to know how to report security issues. With all that said, I hope you understand why I'm closing. |
|
@nicolas-grekas, you're right. I appreciate your approach and explanation. I will remember and keep it in my mind for the future; I have learned quite a lot from it. I'm really sorry for the inconvenience. Unfortunately, this is not my tool; I was part of a quality check for WooCoommerce official plugins that takes the analysis from Semver as linked: https://qit.woo.com/docs/managed-tests/security/. I will report it as a false positive. |
Error from semgrep
audit.php.lang.security.file.inclusion-arg