Updated: Comment #0
Problem/Motivation
Part of SA-CORE-2013-003
Drupal core directly used the mt_rand() pseudorandom number generator for generating security related strings used in several core modules. It was found that brute force tools could determine the seeds making these strings predictable under certain circumstances.
Proposed resolution
Forward port fix from SA-CORE-2013-003
Remaining tasks
Reviews
User interface changes
None
API changes
?
Related Issues
None
Original patch by Owen Barton, David Stoline, Heine Deelstra, Damien Tournoud, and Peter Wolanin
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff.txt | 1.88 KB | scor |
#46 | 2140433-46.patch | 30.81 KB | scor |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedComment #2
larowlanComment #3
scor CreditAttribution: scor commentedComment #4
amateescu CreditAttribution: amateescu commentedSA followups are critical, marking as such.
Comment #5
Berdirhttp://drupalcode.org/project/drupal.git/commitdiff/782d1155c62c0a879bf5... is the 7.24 commit, all random changes will need to be ported to 8.x
Comment #6
chx CreditAttribution: chx commentedComment #7
chx CreditAttribution: chx commentedI could use some guidance over whether Crypt::hmacBase64 should be removed or not.
Comment #8
chx CreditAttribution: chx commentedComment #10
chx CreditAttribution: chx commentedSad panda. I fixed that but forgot to reroll before post.
Comment #11
chx CreditAttribution: chx commentedComment #13
pwolanin CreditAttribution: pwolanin commentedTestbot 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"
Comment #14
pwolanin CreditAttribution: pwolanin commentedputs the full function in FileStorage so we can avoid duplicating it around.
Comment #15
pwolanin CreditAttribution: pwolanin commentedplus minor doxygen fixes.
Comment #16
pwolanin CreditAttribution: pwolanin commentedComment #17
dawehnerLet's just use some proper types here, so the patch don't get nitpicked later.
I am a bit confused about the usage of the method. I cannot really see how a drupal hash is related to URLs
+1 for sure to move this into an actual method, as this removes potential bugs.
For a moment I thought this should not be part of a component, though there is not dependency added to other pieces in drupal.
Let's try to use static:: instead to make it reusable/overridable.
This seems to be a method which could be abstracted to the Cryped/some random component/
Comment #21
chx CreditAttribution: chx commented> 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.
Comment #22
pwolanin CreditAttribution: pwolanin commentedThis should fix the problem in system.install, fixes MTimeProtectedFastFileStorageTest and addresses most of dawehner's comments.
Comment #23
chx CreditAttribution: chx commentedComment #26
pwolanin CreditAttribution: pwolanin commented22: 2140433-22.patch queued for re-testing.
Comment #27
pwolanin CreditAttribution: pwolanin commentedI'm not sure we need 55 bytes when we used to hash down to 32?
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #29
pwolanin CreditAttribution: pwolanin commented22: 2140433-22.patch queued for re-testing.
Comment #31
xjmJust a reroll.
Comment #33
catchMarked #2140473: Code execution prevention of Files directory via .htaccess for Apache as duplicate.
Comment #34
pwolanin CreditAttribution: pwolanin commentedNeeds re-roll. Working on it.
Comment #35
pwolanin CreditAttribution: pwolanin commentedre-rolled + fix test + removed code specific to PHP 5.3.4, which is irrelevant to Drupal 8.
Comment #37
pwolanin CreditAttribution: pwolanin commentedMissed something in the patch:
Comment #38
pwolanin CreditAttribution: pwolanin commentedComment #39
pwolanin CreditAttribution: pwolanin commentedoops, empty patch (the increment was right) - here's the real one.
Comment #41
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #42
pwolanin CreditAttribution: pwolanin commentedComment #43
larowlan=directory?
minor nitpick
Comment #44
pwolanin CreditAttribution: pwolanin commentedfixed. Any other problems?
Comment #45
rlmumfordThis all looks good to me.
Comment #46
scor CreditAttribution: scor commenteddocs cleanup to comply more with our coding standards.
Comment #47
pwolanin CreditAttribution: pwolanin commentedonly doc changes.
Comment #48
webchickCommitted and pushed to 8.x. Thanks!
Comment #49
Heine CreditAttribution: Heine commentedLee 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?
This comment should mention openssl.
Is keeping the fallback desirable?
Comment #50
Heine CreditAttribution: Heine commentedSorry, I was reading from after a git pull, but this didn't get all the updates somehow.
Comment #52
klausiThe mt_srand() call does not make sense to me in D8, filed #2617208: Remove mt_srand() seeding because of performance.