Port SA-CORE-2013-003 to Drupal 8

SA followups are critical, marking as such.

http://drupalcode.org/project/drupal.git/commitdiff/782d1155c62c0a879bf5... is the 7.24 commit, all random changes will need to be ported to 8.x

I could use some guidance over whether Crypt::hmacBase64 should be removed or not.

Sad panda. I fixed that but forgot to reroll before post.

Testbot says:

"PHP Fatal error: Undefined class constant 'HTACCESS' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php on line 71"

puts the full function in FileStorage so we can avoid duplicating it around.

plus minor doxygen fixes.

1. +++ b/core/includes/file.inc
   @@ -607,6 +602,23 @@ function file_save_htaccess($directory, $private = TRUE) {
   + * @param $private
   ...
   + * @return
   

   Let's just use some proper types here, so the patch don't get nitpicked later.

2. +++ b/core/includes/install.core.inc
   @@ -1273,7 +1273,7 @@ function install_settings_form_submit($form, &$form_state) {
   +      'value'    => Crypt::randomUrlSafeString(55),
   
   +++ b/core/includes/install.inc
   @@ -450,7 +450,7 @@ function drupal_install_config_directories($mode = NULL) {
   +    $config_directories_hash = Crypt::randomUrlSafeString(55);
   

   I am a bit confused about the usage of the method. I cannot really see how a drupal hash is related to URLs

3. +++ b/core/includes/session.inc
   @@ -366,7 +366,7 @@ function drupal_session_regenerate() {
   -    $session_id = Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomBytes(55));
   +    $session_id = Crypt::randomUrlSafeString();
   

   +1 for sure to move this into an actual method, as this removes potential bugs.

4. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -2,7 +2,7 @@
   + * Contains \Drupal\Component\PhpStorage\FileStorage.
   
   @@ -58,6 +58,46 @@ public function save($name, $code) {
   +  public static function htaccessLines($private = TRUE) {
   

   For a moment I thought this should not be part of a component, though there is not dependency added to other pieces in drupal.

5. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -83,6 +123,10 @@ protected function ensureDirectory($directory, $mode = 0777) {
   +    if (!file_exists($htaccess_path) && file_put_contents($htaccess_path, self::htaccessLines())) {
   

   Let's try to use static:: instead to make it reusable/overridable.

6. +++ b/core/modules/user/user.module
   @@ -363,10 +363,14 @@ function user_password($length = 10) {
   +    do {
   +      // Find a secure random number within the range needed.
   +      $index = ord(Crypt::randomBytes(1));
   +    } while ($index > $len);
   

   This seems to be a method which could be abstracted to the Cryped/some random component/

> 2. I am a bit confused about the usage of the method. I cannot really see how a drupal hash is related to URLs

See, the variable name is wrong. You don't need a hash. Maybe rename the method to something else? It returns something that is adequate for URLs, file names and pretty much everything machine names used for.

> For a moment I thought this should not be part of a component, though there is not dependency added to other pieces in drupal.

Was already in a constant.

> This seems to be a method which could be abstracted to the Crypt

Sure.

This should fix the problem in system.install, fixes MTimeProtectedFastFileStorageTest and addresses most of dawehner's comments.

22: 2140433-22.patch queued for re-testing.

I'm not sure we need 55 bytes when we used to hash down to 32?

-    $config_directories_hash = Crypt::randomStringHashed(55);
+    $config_directories_hash = Crypt::randomBytesBase64(55);

This issue was originally just about the mt_rand() fix; there's a patch for the htaccess stuff being worked on at #2140473: Code execution prevention of Files directory via .htaccess for Apache.

The patch here looks a little further along, but should probably coordinate with that other issue in some way.

22: 2140433-22.patch queued for re-testing.

Just a reroll.

Marked #2140473: Code execution prevention of Files directory via .htaccess for Apache as duplicate.

Needs re-roll. Working on it.

re-rolled + fix test + removed code specific to PHP 5.3.4, which is irrelevant to Drupal 8.

Missed something in the patch:

Fatal error: Call to undefined method Drupal\Component\Utility\Crypt::randomStringHashed() in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2028

oops, empty patch (the increment was right) - here's the real one.

I think there is some kind of test bug - PhpStorage\MTimeProtectedFastFileStorageTest runs with run-tests.sh, but MTimeProtectedFileStorageTest does not.

There should have be 2 fails!

Anyhow - this patch shoudl fix the test fail.

fixed. Any other problems?

This all looks good to me.

docs cleanup to comply more with our coding standards.

only doc changes.

Committed and pushed to 8.x. Thanks!

Lee Rowlands ask me to take a look at this.

The /dev/urandom stat on Windows is unnecessary and can add up to 38 usecs to each use of drupal_random_bytes(). If a /dev/urandom file DOES exist, non random data may be returned by drupal_random_bytes.

$bytes .= fread($fh, max(4096, $count)); may read more bytes than needed, when count is near 4096 and bytes is not empty. Shouldn't this be max(4096, $count - strlen($bytes), just like in the call to openssl?

// If /dev/urandom is not available or returns no bytes, this loop will
// generate a good set of pseudo-random bytes on any system.

This comment should mention openssl.

Is keeping the fallback desirable?

Sorry, I was reading from after a git pull, but this didn't get all the updates somehow.

The mt_srand() call does not make sense to me in D8, filed #2617208: Remove mt_srand() seeding because of performance.