{"action":"created","issue":{"url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/4023","labels_url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/4023/labels{/name}","comments_url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/4023/comments","events_url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/4023/events","html_url":"https://github.com/bcit-ci/CodeIgniter/issues/4023","id":99014886,"number":4023,"title":"ReDoS (Regular Expression Denial of Service) / Code Injection Risk","user":{"login":"Calico90","id":5116641,"avatar_url":"https://avatars.githubusercontent.com/u/5116641?v=3","gravatar_id":"","url":"https://api.github.com/users/Calico90","html_url":"https://github.com/Calico90","followers_url":"https://api.github.com/users/Calico90/followers","following_url":"https://api.github.com/users/Calico90/following{/other_user}","gists_url":"https://api.github.com/users/Calico90/gists{/gist_id}","starred_url":"https://api.github.com/users/Calico90/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Calico90/subscriptions","organizations_url":"https://api.github.com/users/Calico90/orgs","repos_url":"https://api.github.com/users/Calico90/repos","events_url":"https://api.github.com/users/Calico90/events{/privacy}","received_events_url":"https://api.github.com/users/Calico90/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-08-04T17:06:07Z","updated_at":"2015-08-04T20:22:20Z","closed_at":null,"body":"The second parameter `$separator` in function `increment_string` in file `system/helpers/string_helper.php` is not properly escaped for use in regular expressions. \r\n\r\nNevertheless the parameter is injected directly into PHP's `preg_match` function on line 256.\r\n`preg_match('/(.+)'.$separator.'([0-9]+)$/', $str, $match);`\r\n\r\nThis can cause a Denial of Service (ReDoS) and makes it possible to inject code into the regular expression. When the developer used this `increment_string` with a dynamic (user customizable) string and this string is not properly escaped, code injection is possible and a potentially ReDoS can be caused.\r\n\r\nI would recommend to use PHP's `preg_quote` function to mitigate this vulnerability. \r\nLine 256 in file `system/helpers/string_helper.php` should be:\r\n`preg_match('/(.+)' . preg_quote($separator) . '([0-9]+)$/', $str, $match);`\r\n\r\nAlso without the use of `preg_quote` it's not possible to call this `increment_string` function with one of the following delimiters ` . \\ + * ? [ ^ ] $ ( ) { } = ! < > | : -`\r\n\r\n----\r\n\r\nThis vulnerability seem to exists since CodeIgniter versions 2.1.0 and higher and also applies to stable releases 2.2.0 and 3.0.0.\r\n\r\n*When this fix is pushed to the next stable release, I'll request a CVE number for this vulnerability and assign it to this issue on GitHub.* Also I will prepare a pull request for this fix.\r\n\r\n*Generic information about ReDoS can be found here:*\r\n- https://en.wikipedia.org/wiki/ReDoS\r\n- https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS"},"comment":{"url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/comments/127747369","html_url":"https://github.com/bcit-ci/CodeIgniter/issues/4023#issuecomment-127747369","issue_url":"https://api.github.com/repos/bcit-ci/CodeIgniter/issues/4023","id":127747369,"user":{"login":"Calico90","id":5116641,"avatar_url":"https://avatars.githubusercontent.com/u/5116641?v=3","gravatar_id":"","url":"https://api.github.com/users/Calico90","html_url":"https://github.com/Calico90","followers_url":"https://api.github.com/users/Calico90/followers","following_url":"https://api.github.com/users/Calico90/following{/other_user}","gists_url":"https://api.github.com/users/Calico90/gists{/gist_id}","starred_url":"https://api.github.com/users/Calico90/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Calico90/subscriptions","organizations_url":"https://api.github.com/users/Calico90/orgs","repos_url":"https://api.github.com/users/Calico90/repos","events_url":"https://api.github.com/users/Calico90/events{/privacy}","received_events_url":"https://api.github.com/users/Calico90/received_events","type":"User","site_admin":false},"created_at":"2015-08-04T20:22:20Z","updated_at":"2015-08-04T20:22:20Z","body":"@narfbg Thanks for the feedback. I appreciate the direct approach. \"Saying that this is any kind of a vulnerability is like saying that preg_match() itself is vulnerable and needs a CVE ID assigned\" is a not really the right comparison since it does affect the parameter of a CodeIgniter function and the parameter itself is obviously injectable. Also I don't like it myself when everything is labeled as a vulnerability but as i mentioned \"When the developer used this increment_string with a dynamic (user customizable) string and this string is not properly escaped...\" it's applies indeed on a quite custom (maybe unlikely) scenario and that makes it a LOW risk severity in my opinion. \r\n\r\nI'm fine with leaving the CVE on this, altough I think that this actually will classify for a CVE. That the parameter is misused of it's purpose doesn't mean it's not vulnerable. The whole purpose of IT security is doing unexpected stuff to stuff that expects the expected. Anyway, let's forget about the CVE on this one. \r\n\r\nAbout the public disclosure, since this is an open-source project. This would be the right place in my opinion to discuss such stuff. I'm not aware of another channel to discuss this with the development team, is there? \r\n\r\n*A bit off-topic: A Bug Bounty program on HackerOne could be nice and helpful. Researchers often hack for credits not just for money. On that platform are multiple open-source projects without cash bounties.*\r\n\r\n*About the PR, I thought so already just found it out the hard way. Sorry!*\r\n"}}