[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 WordPress/WordPress-Coding-Standards#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. WordPress/WordPress-Coding-Standards#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 😴

[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: squizlabs/PHP_CodeSniffer#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): WordPress/WordPress-Coding-Standards#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: f71fbc8

[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 🙂