silverstripe / cwp

Common Web Platform (CWP) features module. We strongly recommend using it for all new CWP projects. Future features will be delivered here.
https://www.cwp.govt.nz
BSD 3-Clause "New" or "Revised" License
10 stars 26 forks source link

No supported modules should use `file_get_contents` to access external URLs #228

Open brynwhyman opened 5 years ago

brynwhyman commented 5 years ago

Background

CWP Ops were about to release PHP ini changes that disabled allow_url_fopen and allow_url_include which would prevent the use of file_get_contents() in particular with URLs.

This release had to be postponed because it was discovered some supported modules are still using this function.

Overview

We need to investigate all supported modules (and 3rd party dependancies) to identify where file_get_contents is being used and update it to something more secure.

The use of Guzzle (or raw curl) is encouraged for security reasons, mainly to prevent accidental remote code execution/remote file inclusion bugs.

Release considerations

This improvement would focus on new stacks so we would only need to fix it in a new CWP 2 release.

Modules identified so far

robbieaverill commented 5 years ago

I have a feeling that the use of file_get_contents() in project code would also be widespread. I assume some research has been done in this space before the decision to disable it was made?

brynwhyman commented 5 years ago

I assume some research has been done in this space before the decision to disable it was made?

@madmatt, @chillu do you have input here?

madmatt commented 5 years ago

As noted on the CWP announcement, this was only planned to affect new stacks for now. Once the change had been rolled out, further work would be done to find any affected stack code and get it resolved prior to disabling those values for existing stacks.

In addition, CWP developer docs were updated to indicate why it shouldn't be used.

sminnee commented 5 years ago

I believe the ticket title should be "No supported modules should use file_get_contents to access URLs" but I'll let @madmatt confirm that.

robbieaverill commented 5 years ago

I'm finding it hard to justify this as a bug to be honest... but since the announcement calls it a "low impact security risk" then I guess we will

robbieaverill commented 5 years ago

Are we doing this for SS3 as well?

brynwhyman commented 5 years ago

Are we doing this for SS3 as well?

I believe this should focus on new stacks, so it would make sense to target the latest CWP 2 release.