Harden default php settings

[juju4]

Following puppetlabs/puppetlabs-apache#1749 (comment), I adapted to puppet-php.
For now it is just part of settings, not sure if it should be differentiated.
Risk: users silently overwriting it vs potential post-adaption needed

Potentially more settings to add.

[c33s]

this pr is a bc break, so if this module should follow semver, it would require a major version number increase.

of course i am for good defaults, but it is more than annoying if a minor or patch update break things.

for example expose_php is quite harmless but settings like allow_url_open changed in a minor or patch version are fatal.

also the settings which are set to a hardened default should be discussed

[c33s]

my suggestion would be to add a boolean flag named harden and only if this is set to true the harden settings will be applied (and can still by overriden by providing settings in the settings hash)

edit: so there would be no BC break if the default setting of harden is false

[bastelfreak]

I think we need at least one acceptance test to verify that those settings actually work.

[bastelfreak]

I agree with @c33s here. I'm fine with breaking changes as long as we make it in a major release (which is also fine). We always try to ship working default settings that are as close as the upstream settings. Hardened settings are good, but they are often opinionated. I'm happy to merge this but it needs to be optional and we need tests. We can't ship settings when we don't know if they work.

[juju4]

I'm fine with current remarks. can have it in tree with harden false and at next major release, set to true.

few questions

• if I differentiate those settings (eg harden_enable, harden_settings), is there a puppet function to merge settings and harden_settings with priority to the former?
• as for test, adding a harden_spec.rb in spec/classes seems the way to go. correct?

[bastelfreak]

@juju4 unit tests go into spec/classes/, acceptance tests into spec/acceptance/. There are multiple solutions for merging hashes, for example https://forge.puppet.com/puppetlabs/stdlib#deep_merge

[juju4]

Sorry, it's on my todo list but had no time for it for now. maybe in a few weeks.

[c33s]

i had another look at the settings you added. i fear that this settings are too strict and sometimes i don't understand why they should be a security hole. with some of this settings we will create a bad DX (developer experience) because php programs won't work any more.

'Date/date.timezone' => 'UTC',
is a nice default fallback but is this really critical?

'PHP/expose_php' => 'Off',
this is a very good setting, which increases security and has no side effects for php applications.

'PHP/allow_url_fopen' => 'Off',
i had this in my default settings settings and reverted it soon because too many php applications use this fucntion in their code.

'PHP/disable_functions' => 'pcntl_alarm,pcntl_fork,pcntl_waitpid,pcntl_wait,pcntl_wifexited,pcntl_wifstopped,pcntl_wifsignaled,pcntl_wexitstatus,pcntl_wtermsig,pcntl_wstopsig,pcntl_signal,pcntl_signal_dispatch,pcntl_get_last_error,pcntl_strerror,pcntl_sigprocmask,pcntl_sigwaitinfo,pcntl_sigtimedwait,pcntl_exec,pcntl_getpriority,pcntl_setpriority,chown,diskfreespace,disk_free_space,disk_total_space,dl,exec,escapeshellarg,escapeshellcmd,fileinode,highlight_file,max_execution_time,passthru,pclose,phpinfo,popen,proc_close,proc_open,proc_get_status,proc_nice,proc_open,proc_terminate,set_time_limit,shell_exec,show_source,system,serialize,unserialize,__construct, __destruct, __call,__wakeup',

this is the php default

pcntl_alarm,pcntl_fork,pcntl_waitpid,pcntl_wait,pcntl_wifexited,pcntl_wifstopped,pcntl_wifsignaled,pcntl_wifcontinued,pcntl_wexitstatus,pcntl_wtermsig,pcntl_wstopsig,pcntl_signal,pcntl_signal_dispatch,pcntl_get_last_error,pcntl_strerror,pcntl_sigprocmask,pcntl_sigwaitinfo,pcntl_sigtimedwait,pcntl_exec,pcntl_getpriority,pcntl_setpriority,

so in the PR settings the pcntl_wifcontinued is missing,

i think there are too many functions which are used in normal php installations. this settings look more like masshosting php

• chown: will be executed with the php user, see no real risk here.
• diskfreespace: there is an attack vector (DOS) here but also UX/DX for uploads
• disk_free_space: there is an attack vector (DOS) here but also UX/DX for uploads
• disk_total_space: there is an attack vector (DOS) here but also UX/DX for uploads
• dl: yes should be disabled
• exec: using php cli tools like robo requires you to exec things.
• escapeshellarg: why shouldn't i allow a string manipulating function?
• escapeshellcmd: ^^
• fileinode: info attack vector. but if i use openbasedir, the users can only access their files.
• highlight_file: ^^
• max_execution_time: info attack vector
• passthru: isn't executing on the shell is often required for many applications?
• pclose: see popen
• phpinfo: too often too helpful
• popen: maybe the wrong step in the background process / muli thread / multi process world
• proc_close: ^^
• proc_open: ^^
• proc_get_status: ^^
• proc_nice: ^^
• proc_open: ^^
• proc_terminate: ^^
• set_time_limit: this can be a good setting but maybe the better place would be as an admin value
• shell_exec: isn't executing on the shell is often required for many applications?
• show_source: info attack vector. but if i use openbasedir, the users can only access their files.
• system: isn't executing on the shell is often required for many applications?
• serialize: why that? this is to much used for consuming and providing apis
• unserialize: why that? this is to much used for consuming and providing apis
• __construct: i have not found an extra method so i assume this is for the constructor of an object. why should i want to not execute the constructor, destructor, call or wakeup?
• __destruct: ^^
• __call: ^^
• __wakeup: ^^

'PHP/memory_limit' => '128M',
as far as i remember this is the default from php itself.

'PHP/include_path' => '/usr/share/pear:/usr/share/php',
. is missing, which will prevent include to work for files in the same directory, also you add the pear path, which is not always there. my default setting .:/usr/share/php (comes from php, i haven't set this manually)

'PHP/file_uploads' => 'Off',
required most of the time. no wordpress installation would work without file uploads

'Assertion/assert.active' => 'Off',
not sure if we should go for this one. yes the assert function should be disabled in prod. also zend.assertions is the new setting for this in php7

'Session/session.use_strict_mode' => 1,
+1

'Session/session.cookie_secure' => true,
will break sessions on http sites

'Session/session.cookie_httponly' => true,
will prevent js access to the cookie. +1 from my side.

one extra thing we have to consider, shiping default values in this module overwrites the default settings from php. so we have to support this default settings in this module because if there is a security hole or attach vector which php blocks by changing their default ini file will not change our defaults. so we will still ship the wrong outdated settings.

[c33s]

is the timezone really a critical setting?

[vox-pupuli-tasks]

Dear @juju4, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[vox-pupuli-tasks]

Dear @juju4, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[vox-pupuli-tasks]

Dear @juju4, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[vox-pupuli-tasks]

Dear @juju4, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[juju4]

On default values, there are the ones I used and would recommend.
There are obviously very context dependent but IMO, I prefer a secure default state with known documentation and easy to adapt that the often unsecure unknown default.

Some are from audit tools or other.
Feel free to adapt

[igalic]

making PHP secure is … hard.

[igalic]

aaaand, before 7.0.0, you may as well add preg_replace

[vox-pupuli-tasks]

Dear @juju4, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks