[New Sniff] Forbidden functions

[jrfnl]

Rule:

A number of functions are forbidden for use in a theme.

Current list:

• eval()
• ini_set()
• popen()
• proc_open()
• exec()
• shell_exec()
• system()
• passthru()
• base64_decode()
• base64_encode()
• uudecode()
• str_rot13()

Ref: https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/#check-for-bad-things

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/badthings.php

Decision needed:

The above list needs proper review - is this list complete ?

Should the WPCS WordPress.PHP.DiscouragedFunctions sniff be turned on as well ?

Should there also be a check against the use of the backtick operator ? Ref: http://php.net/manual/en/language.operators.execution.php

To do:

• [ ] Review the list of forbidden functions.
• [ ] Create unit tests
• [ ] Create sniff
• [ ] If so agreed - add existing WordPress.PHP.DiscouragedFunctions sniff to the ruleset.
• [ ] If needed, update the rule in the Theme Review handbook.

[jrfnl]

I would also suggest having a look at the following (existing) sniffs to see if these can/should be (partially) incorporated: WordPress.VIP.FileSystemWritesDisallow WordPress.VIP.RestrictedFunctions WordPress.VIP.SessionFunctionsUsage

[grappler]

Some of the sniffs in WPCS are not separated correctly so that is why I opened an issue to discuss it https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/582

[justintadlock]

We'd need to go through this one function by function. There's a lot that we'd allow: https://github.com/WPTRT/WordPress-Coding-Standards/blob/feature/theme-review-sniffs/WordPress/Sniffs/VIP/RestrictedFunctionsSniff.php

[grappler]

@justintadlock Totally, that is also why I created a new issue upstream to separate some of these checks so that we can only use which applies to us. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/582

[ginsterbusch]

Does the backtick operator even work out of context, ie. outside a shell_exec?

Update: Yes, it does. Second Update: ... with PHP 7.0.5 / Ubuntu 14.04 LTS / Linux x86_64 Tested the following in my local fiddle (similar to PHPFiddle; wrote it a few years ago, for quicker PHP tests):

$file = `uname -a`;
echo $file;

Which resulted in the expected output Linux coolrunnings 4.2.0-41-generic #48~14.04.1-Ubuntu SMP Fri Jun 24 17:09:15 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Further tests with:

• whoami => (my user name)
• groups => (the groups my user name is in)
• ls ~ => (display the content of the home directory of the current user)

went successfully through as well. One with malicious intent could easily cobble together a rootkit / intrusion kit with that. Also, one with next-to-none knowledge about sanitization and security could accidently tear open a BIG fat security hole for people of the former persuasion.

So yes, there should DEFINITELY be a test for (and against) the backtick operator.

cu, w0lf.

[jrfnl]

Does the backtick operator even work out of context, ie. outside a shell_exec?

Well, only on PHP < 5.4 and only when safe_mode is off, but yes, in that case, it would work.

[ginsterbusch]

Well, only on PHP < 5.4 and only when safe_mode is off, but yes, in that case, it would work.

Sorry, but: not true. My development environment uses PHP 7 (version 7.0.5 to be exact). Guess where I tested it? ;)

Also, there is no more safe_mode after PHP 5.4: http://php.net/manual/en/features.safe-mode.php

cu, w0lf.

[jrfnl]

@ginsterbusch Of course. I'm getting confused, its late here :sleeping:

[ginsterbusch]

@jrfnl No offense taken :) yeah, its late, leaning towards early (dawn is creeping up already).

cu, w0lf.

[jrfnl]

@ginsterbusch Still an hour or so before it gets light... all the same - result: https://github.com/squizlabs/PHP_CodeSniffer/pull/1073

[joyously]

Is there a danger from preg_replace() with the 'e' modifier?

[jrfnl]

@joyously Yes there is, but that would be a rule to itself as the preg_replace() function itself should not be blocked and this sniff is not about checking the function parameters.

Related upstream PR covering preg_replace() with /e (and currently not mergable because of licensing issues): https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/608

[jrfnl]

Regarding the backtick operator for executing shell commands:

The upstream PR at PHPCS has been merged, but is currently not yet contained in a released version. I've pushed a branch which can be used for the PR once PHPCS releases a version containing this sniff.

Branch: https://github.com/WPTRT/WordPress-Coding-Standards/commits/WPTRT/PHPCS2.7/feature/disallow-backtick-shell-execution Relevant commit: https://github.com/WPTRT/WordPress-Coding-Standards/commit/f71fbc8371c0c4b50ce34ec131224c6c990742c7

[joyously]

I am cleaning up a hack on one of my client sites, and the injected code starts with: $l___l_='base'.(32*2).'_de'.'code';

[dingo-d]

Would love to hear if there is some new info about this 🙂