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().
Comment | File | Size | Author |
---|---|---|---|
#16 | drupal-942690-16.patch | 1.62 KB | tim.plunkett |
#9 | drupal.stream_wrappers_default_remote.9.patch | 9.73 KB | effulgentsia |
#6 | drupal.stream_wrappers_default_remote.6.patch | 9.48 KB | effulgentsia |
drupal.stream_wrappers_default_remote.patch | 6.46 KB | effulgentsia | |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedSomething 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.Comment #2
pwolanin CreditAttribution: pwolanin commentedTo 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedGood point about the bit mask - makes more sense for a single position to be 0 or 1 for local versus remote.
Comment #4
aaron CreditAttribution: aaron commented@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.
Comment #5
aaron CreditAttribution: aaron commentedmarking the component.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedDiscussed 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.
Comment #7
chx CreditAttribution: chx commentedaaron i find your faith in developers amusing.
Comment #8
pwolanin CreditAttribution: pwolanin commentedVisual review of the patch looks good.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedIn IRC, chx suggested reworking this documentation to not have inline code comments inside a PHP docblock, so this patch does that.
#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.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #11
aaron CreditAttribution: aaron commentedi like this approach. thanks for the work!
Comment #12
chx CreditAttribution: chx commentedwebchick 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).
Comment #13
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #14
chx CreditAttribution: chx commentedWell, the test is still necessary
Comment #15
tim.plunkettOkay, let's write a test!
Comment #16
tim.plunkettWell, 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.)
Comment #17
xjmNeat!
Comment #18
aaron CreditAttribution: aaron commentedComment #19
aaron CreditAttribution: aaron commentedWhoops, I just realized that you reverted back from this patch. That was confusing.
Comment #20
tim.plunkettYeah, I wasn't kidding when I said I didn't get very far :)
Comment #28
Wim LeersDo we still want this test or not?
Also: this is being referenced from #942690: Security harden stream wrappers by defaulting them as remote.
Comment #29
cilefen CreditAttribution: cilefen commentedWim: those are this issue.
Comment #30
Wim Leers🤦♂️
Comment #39
quietone CreditAttribution: quietone at PreviousNext commentedThis issue was committed 12 years ago to Drupal 7 and re-opened to write a test. I took a brief look at the existing StreamWrapper tests in Drupal 10.0 but I don't know if they cover what the test here is expected to be testing.
Instead of leaving this open, a new issue (Task) dedicated to writing the test should be made which obviously refers to this issue. Adding tag for needs followup.
Then this issue can be marked fixed in Drupal 7.
Comment #40
Amber Himes MatzCreated follow-up #3283724: Add test coverage for stream wrappers to ensure defaults are remote for writing the test.
Closing this as it was fixed/committed 12 years ago in Drupal 7.
Comment #41
quietone CreditAttribution: quietone at PreviousNext commentedAdding tag