Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite loop analyzing a certain file. #3942

Closed
1 task done
npr-gh opened this issue Mar 6, 2024 · 2 comments
Closed
1 task done

Infinite loop analyzing a certain file. #3942

npr-gh opened this issue Mar 6, 2024 · 2 comments

Comments

@npr-gh
Copy link

npr-gh commented Mar 6, 2024

Describe the bug

PHPSniffer managed to run forever analyzing a certain CSS file. (I discovered later, 'practically forever', as in longer than the heat death of the universe, but still it theoretically would complete in a finite time).

After the verbose mode created a 17-gigabyte log, I cut it off.

The section of the CSS file that causes the problem is this bit of code.

#available-widgets .widget-title:before {
        content: "\f132";
        position: absolute;
        top: -3px;
        left: 100%;
        margin-left: 20px;
        width: 20px;
        height: 20px;
        color: #2c3338;
        font: normal 20px/1 dashicons;
        text-align: center;
        box-sizing: border-box;
        -webkit-font-smoothing: antialiased;
        -moz-osx-font-smoothing: grayscale;
}

/* dashicons-smiley */
#available-widgets [class*="easy"] .widget-title:before { content: "\f328"; top: -4px; }

/* dashicons-star-filled */
#available-widgets [class*="super"] .widget-title:before,
#available-widgets [class*="like"] .widget-title:before { content: "\f155"; top: -4px; }

/* dashicons-wordpress */
#available-widgets [class*="meta"] .widget-title:before { content: "\f120"; }

/* dashicons-archive */
#available-widgets [class*="archives"] .widget-title:before { content: "\f480"; top: -4px; }

/* dashicons-category */
#available-widgets [class*="categor"] .widget-title:before { content: "\f318"; top: -4px; }

/* dashicons-admin-comments */
#available-widgets [class*="comment"] .widget-title:before,
#available-widgets [class*="testimonial"] .widget-title:before,
#available-widgets [class*="chat"] .widget-title:before { content: "\f101"; }

/* dashicons-admin-post */
#available-widgets [class*="post"] .widget-title:before { content: "\f109"; }

/* dashicons-admin-page */
#available-widgets [class*="page"] .widget-title:before { content: "\f105"; }

/* dashicons-text */
#available-widgets [class*="text"] .widget-title:before { content: "\f478"; }

/* dashicons-admin-links */
#available-widgets [class*="link"] .widget-title:before { content: "\f103"; }

/* dashicons-search */
#available-widgets [class*="search"] .widget-title:before { content: "\f179"; }

/* dashicons-menu */
#available-widgets [class*="menu"] .widget-title:before,
#available-widgets [class*="nav"] .widget-title:before { content: "\f333"; }

/* dashicons-tagcloud */
#available-widgets [class*="tag"] .widget-title:before { content: "\f479"; }

/* dashicons-rss */
#available-widgets [class*="rss"] .widget-title:before { content: "\f303"; top: -6px; }

/* dashicons-calendar */
#available-widgets [class*="event"] .widget-title:before,
#available-widgets [class*="calendar"] .widget-title:before { content: "\f145"; top: -4px;}

/* dashicons-format-image */
#available-widgets [class*="image"] .widget-title:before,
#available-widgets [class*="photo"] .widget-title:before,
#available-widgets [class*="slide"] .widget-title:before,
#available-widgets [class*="instagram"] .widget-title:before { content: "\f128"; }

/* dashicons-format-gallery */
#available-widgets [class*="album"] .widget-title:before,
#available-widgets [class*="galler"] .widget-title:before { content: "\f161"; }

/* dashicons-format-video */
#available-widgets [class*="video"] .widget-title:before,
#available-widgets [class*="tube"] .widget-title:before { content: "\f126"; }

/* dashicons-format-audio */
#available-widgets [class*="music"] .widget-title:before,
#available-widgets [class*="radio"] .widget-title:before,
#available-widgets [class*="audio"] .widget-title:before { content: "\f127"; }

/* dashicons-admin-users */
#available-widgets [class*="login"] .widget-title:before,
#available-widgets [class*="user"] .widget-title:before,
#available-widgets [class*="member"] .widget-title:before,
#available-widgets [class*="avatar"] .widget-title:before,
#available-widgets [class*="subscriber"] .widget-title:before,
#available-widgets [class*="profile"] .widget-title:before,
#available-widgets [class*="grofile"] .widget-title:before { content: "\f110"; }

/* dashicons-cart */
#available-widgets [class*="commerce"] .widget-title:before,
#available-widgets [class*="shop"] .widget-title:before,
#available-widgets [class*="cart"] .widget-title:before { content: "\f174"; top: -4px; }

/* dashicons-shield */
#available-widgets [class*="secur"] .widget-title:before,
#available-widgets [class*="firewall"] .widget-title:before { content: "\f332"; }

/* dashicons-chart-bar */
#available-widgets [class*="analytic"] .widget-title:before,
#available-widgets [class*="stat"] .widget-title:before,
#available-widgets [class*="poll"] .widget-title:before { content: "\f185"; }

/* dashicons-feedback */
#available-widgets [class*="form"] .widget-title:before { content: "\f175"; }

/* dashicons-email-alt */
#available-widgets [class*="subscribe"] .widget-title:before,
#available-widgets [class*="news"] .widget-title:before,
#available-widgets [class*="contact"] .widget-title:before,
#available-widgets [class*="mail"] .widget-title:before { content: "\f466"; }

/* dashicons-share */
#available-widgets [class*="share"] .widget-title:before,
#available-widgets [class*="socia"] .widget-title:before { content: "\f237"; }

/* dashicons-translation */
#available-widgets [class*="lang"] .widget-title:before,
#available-widgets [class*="translat"] .widget-title:before { content: "\f326"; }

/* dashicons-location-alt */
#available-widgets [class*="locat"] .widget-title:before,
#available-widgets [class*="map"] .widget-title:before { content: "\f231"; }

/* dashicons-download */
#available-widgets [class*="download"] .widget-title:before { content: "\f316"; }

/* dashicons-cloud */
#available-widgets [class*="weather"] .widget-title:before { content: "\f176"; top: -4px;}

/* dashicons-facebook */
#available-widgets [class*="facebook"] .widget-title:before { content: "\f304"; }

/* dashicons-twitter */
#available-widgets [class*="tweet"] .widget-title:before,
#available-widgets [class*="twitter"] .widget-title:before { content: "\f301"; }

The issue is a very gross inefficiency on the part of PHPSniffer, it seems to scale as O(2^n).

For each line of "#available_widgets", it appears that the entire file is scanned again, recursively. This creates an exponential amount of work.

To reproduce

Run the following on the command line:

php /usr/src/phpSniffer/phpcs.phar -p test.css --standard=PHPCompatibilityWP -vvv | tee test.txt

Expected behaviour

  • The analysis completes in a reasonable amount of time
  • If a bug in a sniff causes the analysis to take inordinately long, cut off the output at a reasonable point. For example, a restriction of 100M iterations could at least force the execution to skip te file after ~30 seconds, instead of stalling until the server's disk/memory limit is reached and the OOMkiller crashes it / PHP memory limit causes an exception.

Actual behaviour:

A little interesting experiment that really exposes the behaviour is to take the initial block of CSS only, then add the #available-widgets lines block by block. I started at a random position in the middle /* dashicons-admin-users */, and added blocks one by one, then looked at the execution time. I didn't do a lot of sampling so there's quite a bit of statistical error here. The exponential behaviour is pretty clear after a few iterations of adding more lines to the file.

+-------+-------+-------+
| lines | done  | time  |
+-------+-------+-------+
|     7 | 316ms | 425ms |
|    10 | 710ms | 820ms |
|    15 | 1.9s  | 2.0s  |
|    16 | 2.44s | 2.65s |
|    20 | 17s   | 17.2s |
|    22 | 36.5s | 36.7s |
|    24 | 71.2s | 72.0s |
+-------+-------+-------+

t ~ 23.383e(0.325L) where t = time in ms, L = number of '#available-widgets' lines is a least-squares exponential fit to this data.

Versions (please complete the following information)

Operating System: Debian 12.
PHP version 8.3
PHP_CodeSniffer version master
Standard PHPCompatibilityWP
Install type PHAR / Git clone.

Additional context

This can cause the viability of Denial-of-service attacks if PHPCodeSniffer / PHPCompatibilityWP is provided as a service and may be a security issue in this context.

Please confirm:

  • I have searched the issue list and am not opening a duplicate issue.
  • [o] I do not confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards. It can be argued either way and depends on if PHP_Codesniffer takes responsibility for run-time length, it also depends on whether PHP_CodeSniffer does iteration or if external standards can do iteration over symbols (what does the looping?) or can run code. I merely observed the behaviour and didn't step through each line of code.
  • [x ] I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@martinjoiner
Copy link

Based on the information you provide this seems like a very contrived example. You suggest it is a potential vector for a denial-of-service attack but only in the situation where:

  1. Someone is providing PHPCodeSniffer as a service in production
  2. There are no limits on the request to protect
  3. They are allowing CSS to be fed into a static analysis tool for PHP

This seems like a totally unrealistic example and nowhere near the normal use case for PHPCodeSniffer. Why are you feeding CSS into a tool for PHP?

@jrfnl
Copy link
Contributor

jrfnl commented Mar 14, 2024

@npr-gh @martinjoiner Well, PHPCS does currently still handle CSS and JS (up to a point), but the PHPCompatibility rulesets never have, nor will.

As per the readme on those projects: use --extensions=php when running the PHPCompatibility rulesets.

Or if combined with other rulesets, use the below in a custom ruleset to limit PHPCompatibility to PHP files:

	<rule ref="PHPCompatibilityWP">
		<include-pattern>*\.php$</include-pattern>
	</rule>

(though the problem is likely to be in the CSS tokenizer, but as that will be removed in v4, this is unlikely to be fixed)

On another note: don't open issues in this repo anymore, use the new repo instead. See #3932.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants