From f3de2a509a87048feff20de7c2eddfe716d736fc Mon Sep 17 00:00:00 2001 From: "Kristof, Fooman" Date: Tue, 3 Sep 2019 10:35:53 +1200 Subject: [PATCH] Remove ineffective rule I believe this rule is not achieving its intended result. If I understand the intention of the rule md5 should be avoided where collisions can be generated to cause a lapse in security. However in those cases the suggested approach to replace md5 with sha256 I would still consider bad advice. If you are implementing security one should not roll your own. To judge the effectiveness I ran the following on the code base: ``` git checkout 2.2.0 grep -r --exclude-dir='Test' 'md5(' app/code lib/internal | wc -l 52 git checkout 2.3-develop grep -r --exclude-dir='Test' 'md5(' app/code lib/internal | wc -l 53 grep -r --exclude-dir='Test' 'phpcs:ignore Magento2.Security.InsecureFunction' app/code lib/internal | wc -l 4 ``` In summary we have added more exceptions to the rule than made changes after the rule was introduced. I do not believe that removing md5 is warranted in a lot of cases (the exceptions allowed confirm this) and that just removing md5 improves security by itself (in other words `hash('string', 'sha256')` is only marginally better than `md5('string') where really sensitive data is involved, as no one should be handling this directly in the first place. --- Magento2/Sniffs/Security/InsecureFunctionSniff.php | 1 - 1 file changed, 1 deletion(-) diff --git a/Magento2/Sniffs/Security/InsecureFunctionSniff.php b/Magento2/Sniffs/Security/InsecureFunctionSniff.php index 16c69030..1513b22d 100644 --- a/Magento2/Sniffs/Security/InsecureFunctionSniff.php +++ b/Magento2/Sniffs/Security/InsecureFunctionSniff.php @@ -21,7 +21,6 @@ class InsecureFunctionSniff extends ForbiddenFunctionsSniff 'assert' => null, 'create_function' => null, 'exec' => null, - 'md5' => 'improved hash functions (SHA-256, SHA-512 etc.)', 'passthru' => null, 'pcntl_exec' => null, 'popen' => null,