File inclusion weakness in stream wrappers, and StreamWrapperInterface composite constants are not clearly documented or tested for correctness

So to clarify, when I began my stream wrapper work I saw that public was registered with a type 29 and that immediately screamed security weakness because it was LOCAL. I then miscalculated (opsie) 1C to be 29 so I filed on bugcrowd saying "constants are wrong". Well the constants are not wrong but documenting and testing them is good.

However, this didn't change the fact that registering public:// was happening as a local wrapper and so after I saw this test pass I looked into PublicStream a bit late and saw the getType methods. I think the "local stream" class name have caused some confusion here just because we store something on a local disk does not mean it should not be registered as a (remote) URL wrapper. Yay for PHP and its clear, easy to use and understand security facilities.

The original change to use LOCAL was actually in #942690: Security harden stream wrappers by defaulting them as remote.

Nope, even before that issue, public, private, and all other wrappers that didn't explicitly specify a type, defaulted to STREAM_WRAPPERS_NORMAL, which before that issue, was defined as STREAM_WRAPPERS_LOCAL | STREAM_WRAPPERS_WRITE_VISIBLE.

The PHP option allow_url_fopen is true by default which means public:// should work fine with functions like file_get_contents()

What do we do for servers where php.ini defines allow_url_fopen=0?

we should probably consider a backport removal of LOCAL to D7 also

I'm nervous about that. image_gd_save() checks file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL), and so I imagine some contrib modules do as well, especially with file_get_stream_wrappers() documentation having that as an example.

Maybe but that needs a requirement warning because we can't make your site less secure w/o a warning. Also who switches off url fopen...?

+      'title' => t('PHP allows opening URLs as files'),
+      'description' => t('PHP has allow_url_fopen set to 0. This reduces your site\'s functionality and is not the default PHP value. You are advised to edit php.ini and set allow_url_fopen to 1'),

both need better wording: PHP doesn't allow opening URLs as file. And more importantly it reduces the site's security (because we are forced to register public:// as local yadda, yadda).

This is only visible on the install page, correct?

How about...

+++ b/core/modules/system/system.install
@@ -97,6 +97,24 @@ function system_requirements($phase) {
+      'description' => t('PHP has allow_url_include set to 1. This reduces your site\'s security. You are strongly advised to edit php.ini and set allow_url_include to 0'),

Your server's PHP configuration has allow_url_include turned on. This can negatively impact your site's security, and you are strongly encouraged to turn it off. To disable this feature, set allow_url_include to 0 in php.ini.

+++ b/core/modules/system/system.install
@@ -97,6 +97,24 @@ function system_requirements($phase) {
+      'description' => t('PHP has allow_url_fopen set to 0. This reduces your site\'s functionality and is not the default PHP value. You are advised to edit php.ini and set allow_url_fopen to 1'),

Your server's PHP configuration has allow_url_fopen turned off. This disables the ability to retrieve files using a URL, and may reduce your site's functionality. To enable this feature, set allow_url_fopen to 1 in php.ini.

Site builders find a way. Never underestimate what people can come up with.

It is. Security is always good to emphasize.

It looks like the only thing that was changed was quoting the value. Was that a mistake?

I added my text from above and quoted the values. (I'm not too sure the quotes are necessary, though.)

@pwolanin, do you mean add wording to indicate the suggestion of enabling allow_url_fopen has security implications?

I nested the checks since _include requires _fopen to even function. A warning about _include with _fopen off is not needed. Added a sentence to help clarify. (I think) It is a bit tough to make it succinct, which I think is important given how long this text is getting. Lets just make sure the text is reasonable. The code change is more important than the message.

No one seems to have answered @effulgentsia's question from #16

How does this work now?

      // If destination is not local, save image to temporary local file.
      $local_wrappers = file_get_stream_wrappers(StreamWrapperInterface::LOCAL);
      if (!isset($local_wrappers[$scheme])) {
        $permanent_destination = $destination;
        $destination = drupal_tempnam('temporary://', 'gd_');
      }

in core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php

+++ b/core/modules/system/system.install
@@ -97,6 +97,26 @@ function system_requirements($phase) {
+  } else {

else { should be on a new line.

Is this fixed by https://www.drupal.org/SA-CORE-2017-003 ?

#47: No. See http://cgit.drupalcode.org/drupal/commit/?id=c732355412b84a6f7079d92b402....

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.