CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

Bug: [DebugBar] regex vulnerable to super-linear runtime due to backtracking

Open fliodhais opened this issue 2 years ago • 3 comments

PHP Version

7.4

CodeIgniter4 Version

4.1.1

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

apache

Database

No response

What happened?

Was scanning my project with data analysis tool, Sonarcloud and it highlighted that system/Debug/Toolbar/Views/toolbar.js ln631 "The regex used here is vulnerable to super-linear runtime due to backtracking. It can lead to denial of service."

ln631

var patt   = /\((?:[^)(]+|\((?:[^)(]+|\([^)(]*\))*\))*\)/;

// recommended solutio

Steps to Reproduce

system/Debug/Toolbar/Views/toolbar.js ln615

Expected Output

ln631

var patt   = /\((?:[^)(]+|\((?:[^)(]+|\([^)(]*\))*\))*\)/;

// recommended solution
var patt   = /\((?>(?:[^)(]+|\((?>(?:[^)(]+|\([^)(]*\))*+)\))*+)\)/;

Anything else?

I am not sure how to test if the regex actually will prevent backtracking or not. Supposedly the inputs prior are sanitized to prevent backtracking but it would still be good practice to have a secure regex I would imagine.

I will have a PR with the change but I am not sure how to prove that it works.

Edit: line where the regex is found.

fliodhais avatar Mar 02 '23 10:03 fliodhais

https://makenowjust-labs.github.io/recheck/playground/ Screenshot 2023-03-22 11 08 54

kenjis avatar Mar 22 '23 02:03 kenjis

The regex comes from #3070 But it does not work now if there are two (...).

$routes->get('test', 'Home::index');
$routes->get('test/(:num)', 'Home::index');
$routes->get('test/(:num)/(:any)', 'Home::index');
$routes->get('test/page-(:num)/(:any)', 'Home::index');
$routes->get('test/page-(:num)/(:any).html', 'Home::index');

Screenshot 2023-03-22 11 31 31

kenjis avatar Mar 22 '23 02:03 kenjis

@codeigniter4/core-team Why do we need the complicated regex? Something wrong with this?

--- a/system/Debug/Toolbar/Views/toolbar.js
+++ b/system/Debug/Toolbar/Views/toolbar.js
@@ -628,7 +628,7 @@ var ciDebugBar = {
     routerLink: function () {
         var row, _location;
         var rowGet = this.toolbar.querySelectorAll('td[data-debugbar-route="GET"]');
-        var patt   = /\((?:[^)(]+|\((?:[^)(]+|\([^)(]*\))*\))*\)/;
+        var patt   = /\(.+?\)/g;
 
         for (var i = 0; i < rowGet.length; i++)
         {

kenjis avatar Mar 22 '23 02:03 kenjis