Split off from #938614: Downgrade required PHP version to 5.2.4.

In that issue we raised the PHP requirement from 5.2.0 to 5.2.4 so that we can ensure that remote stream wrappers can't be used as a PHP include when the allow_url_include php.ini setting is FALSE.

But currently, new stream wrappers defined by modules default as local stream wrappers, unless the module explicitly adds STREAM_WRAPPERS_REMOTE to the 'type' within hook_stream_wrappers().

This patch changes the default to be REMOTE, so that modules have to explicitly opt-in to STREAM_WRAPPERS_LOCAL in order to have their streams be more trusted.

This patch copies the relevant hunks from #938614-59: Downgrade required PHP version to 5.2.4, and adds a new hunk for documentation in hook_stream_wrappers().

#16 drupal-942690-16.patch1.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 35,617 pass(es), 0 fail(s), and 1 exception(s). View
#9 drupal.stream_wrappers_default_remote.9.patch9.73 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,373 pass(es). View
#6 drupal.stream_wrappers_default_remote.6.patch9.48 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,380 pass(es). View
drupal.stream_wrappers_default_remote.patch6.46 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 26,278 pass(es). View


effulgentsia’s picture

+++ modules/system/system.api.php	15 Oct 2010 15:33:00 -0000
@@ -2310,18 +2315,35 @@ function hook_stream_wrappers() {
+    'youtube' => array(
+      'name' => t('YouTube video'),
+      'class' => 'MyModuleYouTubeStreamWrapper',
+      'description' => t('Video streamed from YouTube.'),
+      // A module implementing YouTube integration may decide to support using
+      // the YouTube API for uploading video, but here, we assume that this
+      // particular module only supports playing YouTube video.
+    ),

Something I don't like in general is that we have separate bits for STEAM_WRAPPERS_LOCAL and STREAM_WRAPPERS_REMOTE. So that means something can be both or neither. Does that make any sense? For example, in the above "youtube" example, neither is specified, since we're saying we want "remote" to be the default. But if something then calls file_get_stream_wrappers(STREAM_WRAPPERS_REMOTE), the "youtube" stream wrapper won't be returned. This is a pre-existing condition of HEAD, so I'm not sure to what extent this patch needs to solve this, but it seems like a WTF to me.

pwolanin’s picture

To clarify: I have some concerns that we could improve the DX, and a little concern about code readability, but probably chx is right that this is the only secure approach at this point for the stream wrappers.

pwolanin’s picture

Good point about the bit mask - makes more sense for a single position to be 0 or 1 for local versus remote.

aaron’s picture

@effulgentsia: Obviously, in the case for the youtube stream, STREAM_WRAPPERS_REMOTE would probably be better to be explicitly defined as best practice for your concern. For that matter, considering that WTF, what would be the disadvantages of simply requiring an explicit definition, rather than a default? Anyone implementing stream wrappers for Drupal really should be reading the friendly manual at this early stage of development, anyway.

aaron’s picture

Component: base system » file system

marking the component.

effulgentsia’s picture

9.48 KB
PASSED: [[SimpleTest]]: [MySQL] 26,380 pass(es). View

Discussed more with chx in IRC. Here's a patch that completely removes the STREAM_WRAPPERS_REMOTE constant. Given that whether something is local or remote has security implications, let's head off the problems that would stem from people using both the STREAM_WRAPPERS_REMOTE and STREAM_WRAPPERS_LOCAL constants, in potentially inconsistent ways. Removing the constant also has the benefit of triggering PHP notices in all modules using that constant, which is a good thing, since given the API change inherent in this patch, we want those module developers evaluating their use of that constant. BC breaks suck, but security holes suck more.

chx’s picture

aaron i find your faith in developers amusing.

pwolanin’s picture

Visual review of the patch looks good.

effulgentsia’s picture

9.73 KB
PASSED: [[SimpleTest]]: [MySQL] 26,373 pass(es). View
+++ includes/file.inc	15 Oct 2010 20:37:21 -0000
@@ -90,12 +90,31 @@ define('FILE_STATUS_PERMANENT', 1);
+ * Examples:
+ * @code
+ *   // Retrieve information about all stream wrappers defined by
+ *   // hook_stream_wrappers() implementations.
+ *   $all_stream_wrappers = file_get_stream_wrappers();
+ *
+ *   // Retrieve information about all stream wrappers that have both
+ *   $readable_and_visible_stream_wrappers = file_get_stream_wrappers(STREAM_WRAPPERS_READ | STREAM_WRAPPERS_VISIBLE);
+ *
+ *   // Retrieve information about all stream wrappers that are visible, but do
+ *   // not use local file storage.
+ *   $remote_stream_wrappers = array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_VISIBLE), file_get_stream_wrappers(STEAM_WRAPPERS_LOCAL));
+ * @endcode

In IRC, chx suggested reworking this documentation to not have inline code comments inside a PHP docblock, so this patch does that.

+++ includes/stream_wrappers.inc	15 Oct 2010 20:37:21 -0000
@@ -65,9 +63,9 @@ define('STREAM_WRAPPERS_VISIBLE', 0x0010
- * Stream wrapper type flag -- visible, readable and writeable.
+ * Stream wrapper type flag -- hidden, readable and writeable using local files.
  * Stream wrapper type flag -- visible and read-only.
@@ -75,9 +73,23 @@ define('STREAM_WRAPPERS_WRITE_VISIBLE', 

@@ -75,9 +73,23 @@ define('STREAM_WRAPPERS_WRITE_VISIBLE', 
+ * Stream wrapper type flag -- visible, readable and writeable.
+ */

#6 makes it look like STREAM_WRAPPERS_WRITE_VISIBLE is being changed, but it's not, so this patch cleans that up.

Powered by Dreditor.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

This is originally chx's patch, so he's not RTBC'ing it. But, he's approved it, pwolanin has approved it (#2, #3, #8), and in IRC, heyrocker didn't object to it, so I think it's worth RTBC'ing and putting in front of Dries and webchick.

aaron’s picture

i like this approach. thanks for the work!

chx’s picture

Status: Reviewed & tested by the community » Needs work

webchick asks for a test. Write a test wrapper (or just steal it from http://drupal.org/files/issues/928104-1-implement-dswi.patch :D ) register it without a type, ini_set allow_url_fopen to false if it's not false yet (do not do anything if it's false, it's neither necessary nor feasible) try to open a test:// file and then detect the error you are rewarded with. Simpletest tests catch errors so it must be possible (grep banana modules/simpletest/tests/error.test).

Dries’s picture

Status: Needs work » Fixed

Committed to CVS HEAD. Thanks.

chx’s picture

Priority: Critical » Normal
Status: Fixed » Needs work

Well, the test is still necessary

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7

Okay, let's write a test!

tim.plunkett’s picture

1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 35,617 pass(es), 0 fail(s), and 1 exception(s). View

Well, I didn't get too far. I found it very hard to write a test for code that already works.
Interesting note, the following patch gave the following error: stream_wrapper_unregister() [function.stream-wrapper-unregister]: Unable to unregister protocol dummy://

(That was me trying to recreate the code before the fix.)

xjm’s picture

Issue tags: +Needs tests


aaron’s picture

Status: Needs work » Needs review
aaron’s picture

Status: Needs review » Needs work

Whoops, I just realized that you reverted back from this patch. That was confusing.

tim.plunkett’s picture

Yeah, I wasn't kidding when I said I didn't get very far :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed 2a0e326 on 8.3.x
    - Patch #942690 by effulgentsia: security harden stream wrappers by...

  • Dries committed 2a0e326 on 8.3.x
    - Patch #942690 by effulgentsia: security harden stream wrappers by...