As discussed in #1668892: Enable secure compiled information on disk . I separated out loader.inc because that will not be upgradable (and index.php), the rest of core will be -- eventually I will want to move the inclusion of loader.inc into index.php to make bootstrap.inc easy upgradeable. Most of the patch is, in fact, just this moving around. drupal_include on line 147 down to line 209 in loader.inc is all that is new. Easy as pie... two new settings.php $conf vars, php_path specifying where we can write to and php_prefix is the fixed random. These eventually will be written by the installer.

CommentFileSizeAuthor
#129 1675260-129.patch33.59 KBchx
#125 1675260-121-124-interdiff.txt12.02 KBpwolanin
#124 1675260-124.patch30.84 KBpwolanin
#121 1675260-121.patch30.55 KBchx
#121 interdiff.txt10.81 KBchx
#118 1675260-117.patch29.73 KBpwolanin
#118 1675260-105-117.interdiff.txt10.69 KBpwolanin
#105 1675260_105.patch24.06 KBchx
#105 interdiff.txt547 byteschx
#103 1675260_103.patch24.06 KBchx
#103 interdiff.txt888 byteschx
#87 1675260_87.patch23.97 KBeffulgentsia
#87 interdiff.txt6.01 KBeffulgentsia
#74 1675260_68.patch23.88 KBeffulgentsia
#73 1675260_69.patch30.34 KBeffulgentsia
#69 1675260_69.patch30.34 KBeffulgentsia
#68 1675260_68.patch23.88 KBeffulgentsia
#65 1675260_65.patch24.4 KBeffulgentsia
#58 1675260_58.patch20.97 KBeffulgentsia
#48 1675260_47.patch16.79 KBchx
#48 interdiff.txt1.1 KBchx
#47 1675260_46.patch16.8 KBchx
#47 interdiff.txt3.96 KBchx
#45 1675260_45.patch14.73 KBchx
#45 interdiff.txt3.25 KBchx
#44 1675260_44.patch14.6 KBchx
#44 interdiff.txt1.52 KBchx
#43 1675260_43.patch14.25 KBchx
#43 interdiff.txt6 KBchx
#39 1675260_39.patch13.11 KBchx
#39 interdiff.txt5.38 KBchx
#37 1675260_37.patch14.15 KBchx
#37 interdiff.txt1.91 KBchx
#36 1675260_36.patch13.44 KBchx
#34 1675260_34.patch13.51 KBchx
#32 1675260_32.patch13.51 KBchx
#31 1675260_31.patch11.84 KBchx
#27 1675260_27.patch12.06 KBchx
#21 1675260_21.patch5.15 KBchx
#17 1675260_17.patch3.5 KBchx
#14 1675260_14_new.patch2.61 KBchx
#9 1675260_9.patch14.24 KBneclimdul
#8 1675260_8.patch13.41 KBchx
#7 1675260_7.patch13.37 KBchx
#6 1675260_5.patch13.72 KBchx
#5 1675260_4.patch13.78 KBchx
#4 1675260_4.patch13.54 KBchx
#3 1675260_3.patch13.42 KBchx
php_loader.patch13.27 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Obviously, testing, more doxygen, installer support is necessary still but first let's agree whether we want this and let's see whether it passes like this.

Status: Needs review » Needs work

The last submitted patch, php_loader.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.42 KB

Now this tries to load the override first then the original. Edit: the test failures were due to missing globals.

chx’s picture

FileSize
13.54 KB

I rewrote the flilter logic to use less memory and only 81 new LoC instead of 83. Yes, it takes less than 100 LoC to implement this. I promised a very small API didn't I :) ?

chx’s picture

FileSize
13.78 KB

This is in line with #133 and has tests (woot!) which I have tested to pass. I have not tested the core installer , though. Also some doxygen is missing. But this is big progress.

Edit: I have not separated out the loader functionality into a separate file. That's premature optimization.

chx’s picture

FileSize
13.72 KB

Some simplification in the writer.

chx’s picture

FileSize
13.37 KB

This one here unifies the reader and the writer into one filter. Took me a bit to figure this one out. But the code reuse in the filter is so high that it worths it. Previously we had an optional read_callback and an optional write_callback instead we now have an optional path_prefix_callback. For the filter, this is responsible for creating the directories.

chx’s picture

FileSize
13.41 KB

Oh, and if read_callback and write_callback is gone then we can clean this up further returning drupal_php_read and drupal_php_write to their intended size: one line of code each. I have noticed that while stream_filter_register is not fussy about registering the same thing twice, stream_wrapper_register is and this might cause a problem with the database wrapper (or anything else people might want to add). A drupal_stream_wrapper_register() has been added to avoid this.

neclimdul’s picture

FileSize
14.24 KB

Some rough docs I wrote while reading the code and a fix for the shortcut in _drupal_php_helper().

chx’s picture

neclimdul raised the question whether we need multiple loaders, the $type argument. However, the DIC can not be stored in the database and so if there is only one loader that means we can not ever use the database. We can say we only want the filesystem and the only configuration is allowed is whether you want native or through a user stream filter and throw the whole config and pluggable thing out. I do not feel good about that approach :)

Edit: we want to inline the variable get as $GLOBALS['conf']['php_loader'] in the next patch.

chx’s picture

Priority: Normal » Major

The original issue was a major feature request so I guess this is major too.

effulgentsia’s picture

Priority: Major » Normal

I haven't reviewed the code in detail yet, but my main concern with using a stream filter is that, per #1668892-127: Enable secure compiled information on disk, it's not compatible with APC caching. The issue summary of #1673162: Analyze performance of uncompiled and compiled DI containers contains some benchmarks for apc and no-apc loading of compiled/uncompiled DIC. The no-apc numbers still show an improvement relative to uncompiled, so a compiled DIC with #9 is still an improvement compared to HEAD. But my hunch is that if we proceed with this for now, we'll still need a follow-up to optimize further for APC. OTOH, the idea in #1668892-144: Enable secure compiled information on disk sounds awesome if it can fly with the security team. Thoughts?

effulgentsia’s picture

chx’s picture

Title: Implement the user filter, prefixed PHP loading » Implement PHP reading/writing secured against "leaky" script
Priority: Normal » Major
FileSize
2.61 KB

A completely new direction which does not involve streams. It involves hash-named directories, and a file name in depending on the mtime of the directory. Comments explain why it's secure. No doxygen or tests yet.

Status: Needs review » Needs work

The last submitted patch, 1675260_14_new.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Bah, that doesn't change the design. Also note that the next patch will need to deal with existing dirs -- which is easy, just delete the file from the directory that will update the directory mtime to be current (same time as mktime IMO).

chx’s picture

FileSize
3.5 KB

I have checked on Windows that file creation and deletion affects the holding directory mtime. So that's settled. Performance note: the operating system needs to read the inodes of every directory en-route anyways and so there is no additional I/O because of the filemtime calls, the only additional time is that information needs to up to PHP. In general, this is as close as we can get to just native include. Works with apc. Even apc.stat=1 which no other solution we came up with so far could claim.

There was a complaint this will be harder to handle this with git. Let me introduce you to git add -A sites/default/files/codegen which adds new ones and removes old ones. Another complaint will be that git checking out will change the directory mtime. While this is true, the attached version (still "bare", no docs, no tests but LOTS of comments) actually stores the filenames (lightly encoded) and it is trivial to write a small and superb fast drush script that iterates over the codegen dirs, decodes the filenames (by replacing the # with slash) and redo the hashing of the filenames (realizing the need for it, I have moved this into a separate function).

effulgentsia’s picture

I think this is a good approach, but:
- We should still make it swappable, following the cache system pattern ($conf variable mapping a 'bin' (e.g., twig, dic) to a class, where #17 is a default class we ship with).
- I'm not sure it's a good idea to use the public files dir. Perhaps another $conf variable that defaults to 'sites/default/codegen'.
- Should _drupal_php_rename_to_salted() also chmod the file to 400?

Before any of the above though, let's get some of the active participants from #1668892: Enable secure compiled information on disk to +1 this approach or poke holes in it.

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: +Security, +WSCCI, +symfony, +Dependency Injection (DI), +wscci-hitlist, +Twig

A few more people weighed in on #1668892: Enable secure compiled information on disk that the approach in #17 is sufficiently secure. Marked that issue a duplicate. Bringing over its tags. Setting to "needs work" at least for #18.1.

effulgentsia’s picture

Issue tags: +Performance

Adding Performance tag to clarify why this is a "task", not a "feature request". We have good reason to expect from #1673162: Analyze performance of uncompiled and compiled DI containers that this will be necessary to avoid/minimize performance regressions from D7 to D8.

chx’s picture

Status: Needs work » Needs review
FileSize
5.15 KB

Here's a classed version. It's a component -- there's nothing Drupal-ish about this, after all. The only problem was that we had a drupal_hmac_64 call but I changed even that to an injected thing. Non-Drupal people can pass in a closure wrapped around hash_hmac or whatever. Right now it's not plugged into Drupal either. I am not deciding on public files or not in this patch, that'll be the next but I would like to note that making users to create and chmod another directory is a pita. Added chmod 0400.

Is @var Callable a thing?

Edit: it seems it is, just with a lowercase c.

pounard’s picture

@var callable
Without caps. "callable" is a valid type hint in PHP 5.4, accepting all of what is callable (function name, closure, array with an object and a function name, etc...).

Nice code, thanks chx, the interface segregation here is exactly what we need at this point.

Crell’s picture

Status: Needs review » Needs work

This is looking much much better than the earlier versions! No detailed review of the algorithm itself yet, but some general comments:

+++ b/core/lib/Drupal/Component/PhpLoader/ProtectedLoader.php
@@ -0,0 +1,111 @@
+  /**
+   * var $hmac Callable
+   */
+  protected $hmac;

This should be @var

+++ b/core/lib/Drupal/Component/PhpLoader/ProtectedLoader.php
@@ -0,0 +1,111 @@
+  function __construct($config) {
+    $this->prefix = $config['prefix'];
+    $this->salt = $config['salt'];
+    $this->hmac = $config['hmac'];
+  }

Since the constructor is not part of the interface, these should be broken out to separate variables rather than a config array. It makes the class more self-documenting. It also means we can provide a docblock for the constructor with very clear documentation about what to pass in and what it does.

Also, should ProtectedLoader be named something more descriptive, like TimeProtectedLoader, or some such? (To differentiate from, I presume, someone implementing some other protection mechanism.)

chx’s picture

We have discussed the $config thing and agreed we keep it just document it very well.

pounard’s picture

This is super low level stuff, and it not be magically instanciated by some DIC, but using pure hardcoded code in some bootstrap phase I guess, so I'm pro keeping the options array here that will be the one we get from the settings.php file.

I don't want to force the contructor to be in the interface, an intermediate solution might be a setOptions() method or just a really good interface PHP documentation.

neclimdul’s picture

re the constructor, I agree. Because its not part of the interface its up to the implementation to clearly document the argument and the use polymorphism/good DI practices to abstract the rest away.

chx’s picture

Status: Needs work » Needs review
FileSize
12.06 KB

Tested, documented, binned, whatnot. Added chmod() calls to really batten down the hatches.

pounard’s picture

Is there a need to allow multiple loaders with the $bin parameter? I tend to think no, any opinion?

Status: Needs review » Needs work

The last submitted patch, 1675260_27.patch, failed testing.

chx’s picture

#28, effulgentsia points out that we have three use cases for the system: Twig, DIC and module upgrades. Twig and DIC can be regenerated but module upgrade can't and so they deserve separate handling. I am puzzled by the 12 exceptions, how can if (is_dir($path)) { chmod($path, 0700); say chmod(): No such file or directory?

chx’s picture

Status: Needs work » Needs review
FileSize
11.84 KB

Ah ha. Stream wrappers without stream_metadata. OK, moved that chmod into my test.

chx’s picture

FileSize
13.51 KB

Rerolled against HEAD plus solved the test cleanup issue once and for all -- a crashing test might have left unreadable dirs behind. Not any more.

Status: Needs review » Needs work

The last submitted patch, 1675260_32.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.51 KB

Meh.

xjm’s picture

#34: 1675260_34.patch queued for re-testing.

chx’s picture

FileSize
13.44 KB

I found two debug() statements and removed them.

chx’s picture

FileSize
1.91 KB
14.15 KB

Instead of copying the whole of file_unmanaged_delete_recursive() I made it slightly more reusable. I do not want to add a possibility of chmod'ing into that function, it's dangerous enough.

pounard’s picture

I don't see the point of functions drupal_php_include() and drupal_php_write() etc... We are slowly switching a lot of core code to OOP code, and I think that people are smart enough to use drupal_php_get_loader()->write(), drupal_php_get_loader()->phpInclude(), etc... Just as cache system is also slowly evolvoving to.

Plus it avoid to have too many functions in global namespace and leverage the OOP architecture below (also the IDE autocompletion if you have one), it encapsulate a bit more what is really behind the procedural wrapper and avoid this indirection layer.

Why is the test a WebTestCase? It would be faster and cleaner to be able to have a UnitTest instead if we can.

That's only an opinion, aside of that I like this patch.

EDIT: Typo.
Re-EDIT: Some other typo.

chx’s picture

FileSize
5.38 KB
13.11 KB

Implemented #38, renamed phpInclude to includePhp (thanks pounard for the name). The test now is so, so very fast, thanks for the idea.

pounard’s picture

Really nice, it makes the change even smaller.

pounard’s picture

If tests pass, it's RTBC for me.

aspilicious’s picture

Status: Needs review » Needs work

I would like to see some doc improvements before this gets in.

+++ b/core/includes/bootstrap.incundefined
@@ -3541,3 +3541,42 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+ * By default, this returns an instance of the Drupal\Component\PhpLoader\MTimeProtectedLoader

This is longer than 80 chars

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.phpundefined
@@ -0,0 +1,130 @@
+<?php
+
+namespace Drupal\Component\PhpLoader;

Missing @file

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.phpundefined
@@ -0,0 +1,130 @@
+class MTimeProtectedLoader implements PhpLoaderInterface {
+  /**

Always add a newline after a "class xxx" line

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.phpundefined
@@ -0,0 +1,130 @@
+   * Removes everything in a directory, leaving it empty.

This needs an @param

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.phpundefined
@@ -0,0 +1,130 @@
+   * @param string $dir
+   * @param string $filename

Needs some extra explanation

+++ b/core/lib/Drupal/Component/PhpLoader/NativeLoader.phpundefined
@@ -0,0 +1,48 @@
+ * Definition of Drupal\Component\PhpLoader\NativeLoader

End with a period

+++ b/core/lib/Drupal/Component/PhpLoader/NativeLoader.phpundefined
@@ -0,0 +1,48 @@
+  protected $prefix;

Add a newline above and create a docblock for this

+++ b/core/lib/Drupal/Component/PhpLoader/PhpLoaderInterface.phpundefined
@@ -0,0 +1,39 @@
+<?php
+
+namespace Drupal\Component\PhpLoader;

Missing @file

+++ b/core/lib/Drupal/Component/PhpLoader/PhpLoaderInterface.phpundefined
@@ -0,0 +1,39 @@
+interface PhpLoaderInterface {

Newline after this

+++ b/core/lib/Drupal/Component/PhpLoader/PhpLoaderInterface.phpundefined
@@ -0,0 +1,39 @@
+   * Include a PHP file.

IncludeS (same for the other functions in this interface)

+++ b/core/lib/Drupal/Component/PhpLoader/PhpLoaderInterface.phpundefined
@@ -0,0 +1,39 @@
+   *   FALSE if the write failed.

We always add both cases in the return documentation. So there can't be any confusion. Even if i's kinda duplicating.

+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -522,6 +522,19 @@ function simpletest_clean_temporary_directories() {
+ * Recursively delete a directory regardless of permissions (if possible).

Deletes a directory recursivly ...

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpLoader/MTimeProtectedLoaderTest.phpundefined
@@ -0,0 +1,41 @@
+class MTimeProtectedLoaderTest extends UnitTestBase {

Can use a docblock header

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpLoader/MTimeProtectedLoaderTest.phpundefined
@@ -0,0 +1,41 @@
+  public function testMTimeProtectedLoader() {

With new files I would like to document test classes better.

21 days to next Drupal core point release.

chx’s picture

Status: Needs work » Needs review
FileSize
6 KB
14.25 KB

Thanks for guarding our code quality. Really appreciated.

chx’s picture

FileSize
1.52 KB
14.6 KB

More!

chx’s picture

FileSize
3.25 KB
14.73 KB

Even more :)

pounard’s picture

Status: Needs review » Reviewed & tested by the community

I think it's ready now.

chx’s picture

FileSize
3.96 KB
16.8 KB

Added a test for the native loader. This makes for a framework for any other loader test btw.

chx’s picture

FileSize
1.1 KB
16.79 KB

Hopefully last one: very small doxygen changes.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.inc
@@ -3541,3 +3541,42 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+function drupal_php_loader($bin = 'default') {

Any particular reason for why this isn't a factory; i.e., Drupal\Component\PhpLoader\PhpLoaderFactory?

If there is a good reason, then let's document it in the phpDoc.

+++ b/core/includes/bootstrap.inc
@@ -3541,3 +3541,42 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+    $loader_backends = variable_get('loader_classes', array('default' =>

Given that the whole purpose of this is to load static, low-level stuff that has been dumped to disk, and that this stuff holds the definitions for services and their parameters, the usage of variable_get() here doesn't make much sense to me. Let's replace this with a simple global $conf.

+++ b/core/includes/bootstrap.inc
@@ -3541,3 +3541,42 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+        'prefix' => variable_get('file_public_path', conf_path() . '/files') . '/codegen',

'prefix' seems to be a directory in all cases, so why not name it 'directory'?

+++ b/core/includes/file.inc
@@ -1332,7 +1334,7 @@ function file_unmanaged_delete($path) {
+function file_unmanaged_delete_recursive($path, $function = 'file_unmanaged_delete_recursive') {

The only thing that _simpletest_delete_recursive() is additionally doing is to call chmod() on $entry_path before recursing. Wouldn't it make more sense to replace $function with $chmod_dir = NULL?

+++ b/core/includes/file.inc
@@ -1340,7 +1342,7 @@ function file_unmanaged_delete_recursive($path) {
+      $function($entry_path, $function);

Why do we pass the called $function name itself as parameter?

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+  public function __construct(array $config) {

Can we rename this to $options?

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+  public function includePhp($filename) {
...
+    if (file_exists($filename)) {
+      include_once $filename;
+      return TRUE;
+    }
+    return FALSE;

1) The call to file_exists() seems unnecessary. It would be better to have a separate ::exists() method that can be called when desired. I don't see why the ::includePhp() method shouldn't unconditionally call into include_once when asked to do so.

2) Why does includePhp() use include_once? If there is a need for include_once, then that should be a separate includeOncePhp() method.

3) Given how expensive it is to calculate the hashed file path, I wonder whether the includeOncePhp() method should use an internal private $includedFiles property to quickly look up whether a given $filename was included already.

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+    if (!@file_put_contents($original_path, $data)) {

Why do we try to hide write errors? They are not suppressed anywhere else.

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+      touch($dir);

Can we add docs that explain why $dir is touched? (outside of the loop)

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+    // clearstatcache() returns NULL so it does not affect the loop condition.
+    while (clearstatcache() || (($mtime = filemtime($dir)) && $previous_mtime != $mtime)) {

That's a nice trick, but it makes the logic unnecessarily hard(er) to follow. It would be cleaner to put one clearstatcache() before the while() and another one at the end of the inner loop.

+++ b/core/lib/Drupal/Component/PhpLoader/MTimeProtectedLoader.php
@@ -0,0 +1,155 @@
+    foreach (new DirectoryIterator($dir, FilesystemIterator::SKIP_DOTS) as $file) {

Shouldn't those two class names at least be rooted to the top-level namespace?

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpLoader/MTimeProtectedLoaderTest.php
@@ -0,0 +1,39 @@
+  public function testMTimeProtectedLoader() {
+    $this->doTest(array('default' =>

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpLoader/PhpLoaderTestBase.php
@@ -0,0 +1,37 @@
+  public function doTest($loader) {

Let's rename the actual test method into testWhatever() on the base class and make the setUp() method for the individual implementation classes provide the implementation to test in $this->loader.

That said, the tests do not seem to be complete in terms of expected functionality/behavior. Let's add some more tests for our expectations there, especially regarding security.

chx’s picture

Assigned: chx » Unassigned
chx’s picture

Any particular reason for why this isn't a factory; i.e., Drupal\Component\PhpLoader\PhpLoaderFactory?

Because that's 500x times as many characters to type. Unlike others I am not drunk on OOP for OOP's ske.

'prefix' seems to be a directory in all cases, so why not name it 'directory'?

It's a path prefix, after all. I admit it comes from the time when it was a filter though. I do not care much what it is called.

The only thing that _simpletest_delete_recursive() is additionally doing is to call chmod() on $entry_path before recursing. Wouldn't it make more sense to replace $function with $chmod_dir = NULL?

Nope. file_recursive_unmanaged_delete() has a stated purpose to not delete things and I want to honor it. It also works with stream wrapper and chmod() doesn't (pre-PHP 5.4 it's always a warning, 5.4 needs stream_metadata implemented which we obviously don't have)

Why do we pass the called $function name itself as parameter?

Why not?

Can we rename $config to $options?

Why would I when it's the configuration for the clsas?

The call to file_exists() seems unnecessary. It would be better to have a separate ::exists() method that can be called when desired. I don't see why the ::includePhp() method shouldn't unconditionally call into include_once when asked to do so.

Because otherwise you can't return TRUE / FALSE.

2) Why does includePhp() use include_once? If there is a need for include_once, then that should be a separate includeOncePhp() method.

It can be include. Core everywhere uses _once even in places which have guards against running twice. I do not see a reason to bikeshed it here.

3) Given how expensive it is to calculate the hashed file path, I wonder whether the includeOncePhp() method should use an internal private $includedFiles property to quickly look up whether a given $filename was included already.

That sounds a bad idea to me memory-wise given how this is going to be used the re-loading is an unlikely problem.

Why do we try to hide write errors? They are not suppressed anywhere else.

Once again, I do not care much. We have a FALSE return value I felt it's the role of the caller to handle the error case.

Can we add docs that explain why $dir is touched? (outside of the loop)

It might actually be obsoleted by now, actually.

That's a nice trick, but it makes the logic unnecessarily hard(er) to follow. It would be cleaner to put one clearstatcache() before the while() and another one at the end of the inner loop.

It's documented and I rather keep the code shorter but -- I do not care.

Shouldn't those two class names at least be rooted to the top-level namespace?

Likely they should.

Let's rename the actual test method into testWhatever() on the base class and make the setUp() method for the individual implementation classes provide the implementation to test in $this->loader.

That said, the tests do not seem to be complete in terms of expected functionality/behavior. Let's add some more tests for our expectations there, especially regarding security.

I have unassigned and unfollowed the issue. We have committed much larger (kernel) patches with practically zero testing. At least this has a test. I am not keen on writing tests for chmod().

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

Thanks for driving it this far, chx. I'll take a turn.

effulgentsia’s picture

I'm almost done implementing sun's feedback and my own, but not quite. Will finish tomorrow and post an updated patch.

chx’s picture

Note: I thought that touch is necessary to make sure the dir mtime / filename changes but then again if there was a file in there we delete it and that changes the dir mtime so it is, indeed, seems unnecessary. On the other hand, the cost of one touch to make extra sure the dir mtime changes even if it was empty is so minuscule that I would keep it.

pounard’s picture

Status: Needs work » Reviewed & tested by the community

@effulgentsia Please don't implement sun's suggestions blindly, I don't agree with all of those, and they need some talking first. Ok chx's answers are pretty good IMHO.

EDIT: About the includeOnce name, I'm against. This is a compiled code class loader more than a php include file helper and I don't see where this could change, I don't think bikeshedding about *_once(), require() or include() has any sense in this issue. Once a file is loaded, it shouldn't be loaded twice because all its compiled code (mostly classes) are up, and it is very likely to be context agnostic code (so include makes no sense in the end. I'd prefer to call the method loadCode() or something instead of keeping the includePhp() bikeshedding name (even if we agreed that load was a bad name with chx in the first place).

@chx Tell me if I'm wrong about the context agnostic side, especially regarding Twig of which I don't much about.

pounard’s picture

Sorry, browser cache problem.

chx’s picture

Status: Reviewed & tested by the community » Needs work

What definitely needs implementing is a test for delete and fixing the delete using classes not use'd because that's a fatal error what sun found.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
20.97 KB

I still want to add some more tests after lunch, but meanwhile, here's a patch that can be reviewed. I renamed a lot, so not attaching an interdiff.

Status: Needs review » Needs work

The last submitted patch, 1675260_58.patch, failed testing.

pounard’s picture

Assigned: effulgentsia » Unassigned

I think that the "generated php" is wrong. It can store PHP code, in general.

effulgentsia’s picture

Status: Needs work » Needs review

#58: 1675260_58.patch queued for re-testing.

effulgentsia’s picture

But the purpose is to store PHP generated by the Drupal process. Even if it's a "download code from d.o. and save it locally", isn't it still "generated" in that sense?

Status: Needs review » Needs work

The last submitted patch, 1675260_58.patch, failed testing.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia
Status: Needs work » Needs review

#60 was x-post I think. Reassigning to myself.

effulgentsia’s picture

FileSize
24.4 KB

Added more tests. I haven't been able to replicate the #58 failures locally yet, but I wonder if the changes here happen to fix them.

Status: Needs review » Needs work

The last submitted patch, 1675260_65.patch, failed testing.

effulgentsia’s picture

Ok, now I can replicate the "Resettable static variables test" failure locally. Working on it. Also, upon thinking about it, I agree with pounard (#60) to remove "generated": it's an extra word that's unnecessary. WIll do so in the next patch.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
23.88 KB
effulgentsia’s picture

FileSize
30.34 KB

Adding the DIC use case into here.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

I'm done for the day. Will check in on this again tomorrow morning.

Status: Needs review » Needs work

The last submitted patch, 1675260_69.patch, failed testing.

katbailey’s picture

Please let's keep the scope of this issue to the mechanism for writing and reading the files. The container use case had originally been included in the bundles patch and was removed pending a secure way to do it, which was this issue here. A follow-up issue was then created for the container compilation, which is here: #1706064: Compile the Injection Container to disk, postponed until this one gets in. As you noted on that issue today, @effulgentsia, I had already submitted a patch which puts back the code that was removed, adjusted to work off chx's solution in #48. Adjusting it to account for renames of the various functions involved will be trivial.

Adding in the container compilation use case will only serve to slow down this issue - the exact way that we do the container compilation needs a whole separate discussion.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
30.34 KB

The #69 failures are interesting. Wondering if they're random or will happen again in the same locations, so uploading again to get a separate bot run.

effulgentsia’s picture

FileSize
23.88 KB

Re #72, fair enough. Reuploading #68 which is just the system without a use case. Looking forward to some reviews.

chx’s picture

Status: Needs review » Needs work

Thanks for the patch.

In the security test I would mention that the security problem we work against here usually does not include access to chmod only an arbitrary setting of upload file names so even that 1 second window is, in fact, much, much shorter: the window between chmod 0300 and 0100.

Also, there is no delete test. This might be related to the single exception we are getting: opendir(/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/files/simpletest/593341/php/dependency_injection/container.php): failed to open dir: Permission denied" If I will have time to look into today I will reassign to myself, not yet sure.

effulgentsia’s picture

Status: Needs work » Needs review

PhpStorageTestBase::doTest() tests delete(). Good suggestion on docs improvement for the security test. Back to "needs review" though to encourage more feedback on the rest of the patch.

Status: Needs review » Needs work

The last submitted patch, 1675260_68.patch, failed testing.

pounard’s picture

The tests are not easily extendable, I think sun raised that point.

The abstract test method should define a getPhpLoader() (no name bikeshedding here) function, and carry all the tests (except the implementation very specific related tests), and the implementors would only have to extend it and implement this method.

chx’s picture

Assigned: Unassigned » chx

This issue is an absolute textbook example of bikeshedding -- the kernel patches are the atomic plants noone understands so they fly in without (much needed) scrutiny this issue is the bikeshed which gets (totally unneeded) lots of nitpicking. I will fix the failing tests and that's it.

Edit: doesn't fail for me locally. Re-testing.

Edit2: holding up the patch for alleged limitation of tests is bikeshedding, plain and simple. They are extensible enough and what if they aren't? There won't be many contrib alternatives and if they find the core test limited they can write their own just fine. The API needs to be unlimited, not the tests.

chx’s picture

Status: Needs work » Needs review

#74: 1675260_68.patch queued for re-testing.

pounard’s picture

@chx sorry for the occasional bikeshed.

Dries’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.phpundefined
@@ -0,0 +1,77 @@
+  public function load($name) {
+    $path = $this->getFullPath($name);
+    if (file_exists($path)) {
+      include_once($path);
+      return TRUE;
+    }
+    return FALSE;

@msonnabaum will beg you to remove the file_exists() call in load() because it could be a performance killer on network file systems. We should ask ourselves if it is really necessary to do that file_exists() call. I can't really tell because there are not any use cases yet in the code, it seems.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -939,4 +939,16 @@ abstract class TestBase {
+    if (file_exists($path)) {
+      chmod($path, 0700);

It is odd that we need to do a file_exists() check. Why would this file get called by file_unmanaged_delete_recursive() on a non-existing file?

moshe weitzman’s picture

Since the generated PHP is basically a cache, I think it would be fine to default to writes to temporary:// instead of public://. The issue is that all requests need to access the generated php file and that read will be slow when you have a shared filesystem. This also avoids a WTF with private files within the public:// directory.

chx’s picture

Assigned: chx » Unassigned

If we want to use this for the module upgrade scenario it needs to be permanent.

The only reason include it has a file_exists because otherwise we can't have a boolean return. Feel free to remove and replace with documentation. I won't. I promised to get this green again and that's it. It wasn't actually failing so that was easy.

I have no idea why effulgentsia added a file_exists before the chmod.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'll work on this some more this afternoon to incorporate recent feedback from pounard, Dries, and Moshe.

If we want to use this for the module upgrade scenario it needs to be permanent.

I think we can go with temporary as the default, and in the issue that we do module upgrade as a use case, have that one use a special bin. Thoughts?

I have no idea why effulgentsia added a file_exists before the chmod.

Something failed for me at one point when I didn't. I'll look into it and if it fails again, see if there's a more proper fix.

chx’s picture

Hrm, yeah, we can go with another bin, after all temporary is a variable too, isn't it?

If by pounard's feedback you mean the tests, I am very much against the idea of adding a test method to the abstract class and then have the real test classes with a getinfo and a setup. It'd work. That's what Doctrine does for the AnnotationReader test. It took me about an hour to comprehend that one. It borders deliberate obfuscating tests. The tests are fine as they are. They are trivial to extend if what you want to do is the same -- if not, write your own test, I already said so. (I find it amusing how the OOP people are suddenly against injecting just because we didn't pick the sacred "implement and override" bullshit they pollute everything with)

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
6.01 KB
23.97 KB

Removed the file_exists(). Renamed doTest() to assertCRUD(), but per #86, didn't change the structure.

@moshe: would it be okay to deal with a non public:// default in a follow up? I think we may need to introduce something different than temporary:// (perhaps local://). Also, I want to avoid calling file_directory_temp() from drupal_php_storage(), so that drupal_php_storage() can be invoked prior to database bootstrap.

@chx, re #75: I didn't change the docs, because testSecurity() is testing the mtime protection in addition to the permissions. So, you're right that permissions alone provide a lot of the security, but since we also have mtime protection, that only kicks in if someone manages to break permissions. Maybe there's a way to capture that better in the docs, but I might not work on this again for a bit, so putting this up as an rtbc candidate in the meantime.

Status: Needs review » Needs work

The last submitted patch, 1675260_87.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

I've seen that @chx posted a question on http://stackoverflow.com/questions/11518486/ctime-mtime-holding-director...

But did we actually verify the changing mtime behavior? It looks like we're adding a dependency on a rather "magic" behavior of certain filesystems here - which builds the basis for an essential security mechanism.

This is what Windows 7 shows me:

$ mkdir core#bootstrap

$ dir core#*
11.08.2012  00:53    <DIR>          core#bootstrap

$ touch core#bootstrap\somegarbage.php

$ dir core#*
11.08.2012  00:56    <DIR>          core#bootstrap

$ dir /T:A core#*
11.08.2012  00:56    <DIR>          core#bootstrap

$ time /T
00:57

$ echo hack > core#bootstrap\somegarbage.php

$ cat core#bootstrap\somegarbage.php
hack

$ dir /T:A core#*
11.08.2012  00:56    <DIR>          core#bootstrap

$ dir /T:W core#*
11.08.2012  00:56    <DIR>          core#bootstrap

$ dir core#*
11.08.2012  00:56    <DIR>          core#bootstrap

In essence, in line with the answer on SO, the mtime is only updated when a file within the directory is created or deleted, but not when a file's content is updated.

In turn, we seem to have the expectation and assume that no one will be able to execute a script along the lines of:

in 'public://php' as $root:
  # We know the directory name to attack.
  # There is only one file in each directory.
  for find '*.php' in "$root/core#bootstrap" as $file
    echo $mystuff > $file

It is possible that you need to have lower-level access in order to perform such a script in the first place. OTOH, relying on this magic filesystem(-specific) behavior is a bit concerning. Just wanted to point out my findings.

effulgentsia’s picture

#87: 1675260_87.patch queued for re-testing.

moshe weitzman’s picture

I would be ok if temp:// were handled on a follow-up.

chx’s picture

#89 this is not what we are protecting against. Also note that your find will abort with permission denied. The dirs are not listable.

Edit: if that script is PHP and it has sufficient rights to chmod a dir created by the web server, it already has the same privileges as you would gain by owning Drupal. If that was bash, ie you gained a shell running under web server rights then you can create a PHP, file in the public dir, write SetHandler application/x-httpd-php to .htaccess (perhaps in a subdir if you happen to not be able to override the one Drupal dumped) and run whatever PHP you wanted. Breaking this is again unnecessary. No, what we want to stop is truly crappy scripts which allow arbitrary file uploads because just including a bare PHP file from an Apache writeable dir is a new vulnerability to those scripts and so we protect against that.

chx’s picture

So let's recap the mtime reliance.

Putting a file in a chmod 0100 directory already protects against one off usage of a leaky upload script.

Making the file a secret hash protects against an infinite repetition of a leaky upload script to write a known filename in the (very short) chmod 0300-0100 window because simply the attacker can't know what filename to write.

The attacker could learn of the filename if the error display setting would be switched on (which is wrong for production anyways) PLUS an error in the generated PHP script would reveal that secret to the browser. See automation above for exploiting this. Making the hash relying on directory mtime protects against this step but this step alone. Should directory mtime not change upon file write/delete then we are still quite secure: the attacker needs this superb weird filesystem, a leaky upload script, some bogus PHP code deployed, error display on to launch the automated attack and even then it needs luck to avoid being detected by hammering a script for catch the short chmod window.

effulgentsia’s picture

Well, we actually have multiple layers of protection. If chmod were truly enough, we wouldn't need the mtime business at all, because both the directory and file are made not writable.

We have another layer of protection that the directory is made not readable either, making disclosure of the hashed file name also not easy.

So we're assuming here that we want another layer of protection in case the chmod isn't enough (e.g., Windows?, or if a crappy upload script is so crappy that it makes the destination writable before copying an uploaded file to it?) and the file name gets disclosed anyway (e.g., if the code inside throws uncaught errors and the site has errors reporting to the screen?). In this case, the directory mtime provides protection against a file move (i.e., what PHP's move_uploaded_file() does). #89 is correct that on all systems (not just Windows), writing to the file directly (rather than moving another file over it) doesn't touch the directory's mtime, so would be a vulnerability. So we're relying that a crappy upload script isn't so crappy that it fails to validate extension, makes the destination file writable, *and* doesn't use move_uploaded_file() or similar.

How much of the above should be documented in the code? Do we want detailed comments in there about "ok, here's what you need to do to break this"?

[Edit: this was an xpost with #93]

chx’s picture

I think a link to this issue is adequate.

pounard’s picture

@#82,#84,

The only reason include it has a file_exists because otherwise we can't have a boolean return. Feel free to remove and replace with documentation. I won't. I promised to get this green again and that's it. It wasn't actually failing so that was easy.

Actually, include(_once|)() will return a strict FALSE if the include failed. So in that case we could return FALSE === @include_once $filename instead thus avoiding both fatal errors and double stat() on the filesystem.

@#83

Since the generated PHP is basically a cache, I think it would be fine to default to writes to temporary:// instead of public://. The issue is that all requests need to access the generated php file and that read will be slow when you have a shared filesystem. This also avoids a WTF with private files within the public:// directory.

We actually already discussed this on IRC, and using temporary here is a really bad idea. Yes it some kind of cache, but it is a persistent cache. Temporary file system will be prone to the operating system recurrent clean cycles purge, while when the site will be a in aggressive production mode it won't attempt rebuild by itself (I hope). Plus using temporary means potentially shared data with other daemons or sites on the operating system level, which makes it even less secure.

@#86

If by pounard's feedback you mean the tests, I am very much against the idea of adding a test method to the abstract class and then have the real test classes with a getinfo and a setup

Even if I do not agree completely, I'm OK with keeping chx's tests as-is in order to avoid bikeshed, and because it works, so we don't need to refactor for the sake of refactoring here.

EDIT: Minor typo.

pounard’s picture

-    return file_exists($this->getFullPath($name));
+    $path = $this->getFullPath($name);
+    return file_exists($path);

Why assigning a temporary variable here? I don't see the need of those changes, at all, and considering that PHP will not have such an efficient compile phase because of its highly dynamic nature, avoiding those seems like a good idea.
In the unlink case, we can afford not putting the @ in front or testing with a file_exists() around first if we want to get rid of errors, because the delete will not be done on performance critical code pathes.

chx’s picture

Hrm, I missed

. If the file can't be included, FALSE is returned and E_WARNING is issued.

on the include manual page. Thanks. So the next patch needs to remove the variable in #97 and implement the include-return.

Edit: I fixed the PHP manual page so it won't be so easy to be missed now.

Crell’s picture

Please be sure to include a comment for why we're using an @ then, which is otherwise a big no-no. (I can see the valid use here, because of the E_WARNING, but we should document why we're making an exception.)

chx’s picture

Note: I am not rerolling this to protest against the bullying-review in #49. This sort of thing must stop and I have no other ways but to strike. I haven't rolled a patch since in this issue and will not roll again nor will I in any other similar issue.

chx’s picture

Assigned: Unassigned » chx

Alas, our process contains no protection against such. If you go on strike , the bully won and the issue died / stalled. If you don't then you waste a ton of time defending non existing problems and again the bully won in stalling the issue as he wanted.

For the sake of this issue I will continue but -- how is the conflict resolution process coming :) ?

pounard’s picture

Just ignore conflicts and continue with the patch. Once all fatal errors and dangerous code path will be eliminated, and all needed features will be here, the names/whitespace/etc bikeshed can go on.

chx’s picture

FileSize
888 bytes
24.06 KB

This implements #98 and #99 as agreed. Sensible requests I can work with.

Status: Needs review » Needs work

The last submitted patch, 1675260_103.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
547 bytes
24.06 KB

D'oh, include is a statement, so...

catch’s picture

We can drop the @$function() call stuff when we've done #1247666: Replace @function calls with try/catch ErrorException blocks, just spamming that here.

pounard’s picture

A try/catch block doesn't turn off the PHP notices, @function does: in this case this is the only goal we want to achieve, avoid a PHP file not found notice (and not Exception) because our layer does its own error handling. Try/catch blocks in this code would solve nothing.

EDIT: I read the other topic after writing that, I'm not sure this is a good idea to switch all PHP notices to exceptions, we are in a low level function here and spawing an exception object might be a bad idea for performances.

chx’s picture

Stop. That's the other issue. Let's stay focused on this one.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Let's mark this RTBC and see the bikeshed happen. I don't care for the names to change again, they are explicit enough for me.

EDIT: I did a quick eye review, and followed all the interdiff since the begining, I didn't see anything chocking happen. I'm happy with the interface and the names and the tests passes, so I'm totally fine with this being RTBC. As I said before, we will find bugs as soon as we will really use it, so let's not bikeshed it without really using it first. I won't do a whitespace review, so if anyone else wants.

webchick’s picture

This is way above my "paygrade" :) so I would prefer to defer final sign-off on this patch to catch or Dries.

webchick’s picture

Assigned: chx » catch

Moving to catch for now to reflect this.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

This patch should include a read-only implementation of PhpStorageInterface

I'll add a simple implementation, but I think the more serious work on that should be in the follow-ups.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Given what this issue already went through, I would rather not add that here, to avoid further debates. (Should write do nothing? Throw exceptions? Etc.) It is entirely possible that we need higher level changes to react etc.

Let's do that in a folllowup.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

Patch also needs work since $GLOBALS['drupal_hash_salt'] may not be set - should use drupal_get_hash_salt()

chx’s picture

Status: Needs work » Closed (won't fix)

As I said before, while "atomic plant" patches get in without any review #1599108-147: Allow modules to register services and subscriber services (events) because noone understands them (I was reviewing and discussing that one with Kat at the very moment it went in) this has no chance of getting in ever because everyone has their opinion. Let's admit it and stop wasting everyone's time.

webchick’s picture

Status: Closed (won't fix) » Needs work

If you no longer wish to work on this due to your disagreements with the feedback you're receiving, that's your perogative, but don't abuse the issue status flag.

Hint: A handy way to keep people from ever reviewing your patches is to treat them like this when they do.

pwolanin’s picture

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
29.73 KB

Here's a new patch + interdiff that adds some more code comments, adds a read-only class, and adds some more tests including a demonstration that the Mtime class cannot protect against e.g. broken code that does a fiel_put_contents(). We use this, for example, in javascript_libraries module for local caching of files, so a logic error in such a module is a possible attack vectot.

chx’s picture

Trying to write a calm review: such a logic would need to chmod the parent directory first and then find out the secret filename. Neither is likely.

Also, the sleep is completely unnecessary, we have dropped the filemtime directory mtime comparison when chmods were added. That can be debated whether it makes sense to drop it. In my opinion it's much better to have but then people have said that the additional mtime can be significant on a networked FS or something. I very seriously doubt there is a system where the mtimeprotected loader will be used and a single mtime is significant and also there are filesystem level caches but -- did I mention I am sick of people bikeshedding this death? If we add it back then of course the test won't work. Not that it does now:

    $hacked = FALSE;
    $untrusted_code = "<?php\n" . '$hacked = TRUE;';
    file_put_contents($expected_filename, $untrusted_code);
    $php->load($name);
    // If the untrusted code was included, $hacked would be re-assigned to FALSE.
    $this->assertIdentical($hacked, FALSE);

(edited for brevity) color me very confused on what this intends to do. hacked starts as FALSE. Then it asserts it stayed FALSE...

chx’s picture

Also, I am totally not sure why would we want to add such a test to core. The test it not about any functionality we do not want to regress but about a known limitation. We know what would need to change to immediately make it obsolete. What purpose it serves? Proof of concept -- there's no need to prove we know already.

chx’s picture

FileSize
10.81 KB
30.55 KB

Summary of changes

  1. Added back the filemtime check with an option to switch it off.
  2. Rolled the security and vulnerability tests into one.
  3. Changed all tests to use a global. The security and vulnerability tests were using a variable and because the included didn't happen in scope those tests were not able to actually to test anything. The old tests were using functions which fatal'd on fail. Not nice. Globals are nicer for this.

The interdiff is big because of the massive test changes.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I'm more confortable with the system defaulting to a mode that protects against the full range of mechanisms to update files.

possibly we should add 'bin' as a param that's used by class FileStorage?

However, that can be a trivial follow-up once we start to use this code and see what's needed to support compiling the DIC.

catch’s picture

+++ b/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.phpundefined
@@ -0,0 +1,76 @@
+  /**
+   * Constructs this FileStorage object.
+   *
+   * @param $configuration
+   *   An associative array, containing at least one key (the rest are ignored):
+   *   - directory: The directory where the files should be stored.
+   *   - bin: overrides the $bin variable passed in directly.
+   * @param $bin
+   *   The storage bin. Multiple storage objects can be instantiated with the
+   *   same configuration, but for different bins.
+   */
+  public function __construct(array $configuration, $bin) {
+    if (isset($configuration['bin'])) {
+      $bin = $configuration['bin'];
+    }
+    $this->directory = $configuration['directory'] . '/' . $bin;

Why do we need both $configuration['bin'] and $bin?

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.phpundefined
@@ -0,0 +1,240 @@
+    // While file_exists() and filemtim() uses the same PHP stat() cache,

file_mtim().

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.phpundefined
@@ -0,0 +1,240 @@
+      file_put_contents($htaccess_file, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006\nDeny from all\nOptions None\nOptions +FollowSymLinks");

Why can't we use file_save_htaccess() here? If there's a reason, why can't we make file_save_htaccess() capable of being used? It's not great that the text is copy/pasted in this function - makes it harder to update tc.

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/MTimeProtectedFileStorageTest.phpundefined
@@ -0,0 +1,114 @@
+    // Ensure that if the file is replaced with an untrusted one (due to another
+    // script's file upload vulnerability), it does not get loaded. Since mtime
+    // granularity is 1 second, we cannot prevent an attack that happens within
+    // a second of the initial save(). However, it is very unlikely for an
+    // attacker exploiting a mere upload vulnerability to also know when a
+    // legitimate file is being saved, discover its hash, undo its file
+    // permissions, and override the file with an upload all within a single
+    // second. Being able to accomplish that would indicate a site very likely

This comment contains a lot of the underlying discussion in this issue, but it's not put as well in the code itself as it is here - if possible it'd be nice to move it into the actual storage class.

Generally I think this looks fine, the security implications have been discussed at length in this issue and I'm happy with the balance that's in the patch.

Would be great to see performance numbers for how expensive this is compared to raw include_once() just to have an idea - presumably we'll recommend that sites that care about performance will check in any generated PHP code to version control and use a raw PHP reader but lots of sites won't. However I can't believe it'll be more overhead than using un-compiled containers or uncompiled Twig templates and whatever other horrible things we'd do without it so that doesn't block a commit at all.

pwolanin’s picture

FileSize
30.84 KB

Here's a revised patch taking those comments into account, and making sure to set all standard configuration keys.

pwolanin’s picture

here's the interdiff too

catch’s picture

OK the interdiff is really a diff of the two patches which is hard to read, but I think it's OK if I'm not going too cross-eyed reading it.

One more question - should the $configuration['fast'] stuff be a separate implementation that extends from the secure version then instead of configuring it as fast you just switch to that storage backend? We don't necessarily need to do that to get this in but it feels like it might be simpler.

pwolanin’s picture

It could be a separate class, sure. it would just change the load method. Given that there is no use of it now except in the test, we could also do it in a follow-up.

pounard’s picture

I agree with catch here, but getting the patch commited is really important to unblock WSCCI/kernel stuff, so IMO pwolanin's suggestion to do a follow-up sounds good to me.

chx’s picture

FileSize
33.59 KB

Class split, test split, fast removed. Whether MTimeProtectedFileStorageTest extends MTimeProtectedFastFileStorageTest or vica versa or both extend the same abstract base class is most definitely a followup. Such ambiguity does not exist for the actual storage classes, there MTimeProtectedFileStorage must extend MTimeProtectedFastFileStorage because while MTimeProtectedFastFileStorage uses FileStorage::load() and FileStorage::exists(), MTimeProtectedFileStorage has its own load and exists methods (and not much else). I ran the PHP Storage tests, they pass, I checked git, the new files are here, all looks good.

pwolanin’s picture

Ha, I was writing almost exactly the same change yesterday, but was having a test fail so I hadn't posted it yet.

However, instead of "MTimeProtectedFastFileStorage" I used "MTimeLessProtectedFileStorage". Which aspect is more important to emphasize?

chx’s picture

MTimeLessProtectedFasterFileStorage :D ?

pwolanin’s picture

Ha, that would be fine with me.

catch’s picture

Title: Implement PHP reading/writing secured against "leaky" script » Change notice for: Implement PHP reading/writing secured against "leaky" script
Assigned: catch » Unassigned
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

OK I discussed this a bit in person with chx and pwolanin (separately), and have looked through a few times now. The two classes seem much happier to me, I like mmmmm timeless as a name but we can do that in a follow-up.

This will need a change notice.

chx’s picture

Status: Active » Needs review
mikeytown2’s picture

I saw that the code that went in uses clearstatcache() without specifying the filename; thus clearing the whole stat cache. I created an issue that addresses this in core #1748880: Only clear the stat cache for the files we care about

pwolanin’s picture

altered the change notice to use drupal_php_storage('mymodule')

chx’s picture

Status: Needs review » Fixed

Sure.

chx’s picture

Title: Change notice for: Implement PHP reading/writing secured against "leaky" script » Implement PHP reading/writing secured against "leaky" script

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

clemens.tolboom’s picture

I'm not sure I understand the 'leaky script' solution at full.

But is it still possible to fix #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination as that only umask stuff?