Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix mcrypt_create_iv(..., MCRYPT_RAND) to auto-seed RNG
Summary: Without seeding the random number generator,
we'll always get the same IV, and that reduces the security
of this function.

Fortunately, f_rand() has all of that logic for auto-seeding
and selection of a suitable initial seed built-in.

Realistically, using MCRYPT_RAND should be deprecated.
I'm going to wait on PHP Internals to make a decision on
https://wiki.php.net/rfc/deprecate_mcrypt_rand
before adding that warning however, so that our test suite
remains consistent.

Credit: Theodore R. Smith of PHP Experts, Inc. <theodorephpexperts.pro>

Closes #3496

Reviewed By: @ptarjan

Differential Revision: D1502435
  • Loading branch information
sgolemon authored and hhvm-bot committed Aug 17, 2014
1 parent d139b46 commit ab6fdeb
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion hphp/runtime/ext/mcrypt/ext_mcrypt.cpp
Expand Up @@ -17,6 +17,7 @@

#include "hphp/runtime/base/base-includes.h"
#include "hphp/runtime/base/runtime-error.h"
#include "hphp/runtime/ext/ext_math.h"

#include <sys/types.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -376,7 +377,8 @@ Variant HHVM_FUNCTION(mcrypt_create_iv, int size, int source /* = 0 */) {
} else {
n = size;
while (size) {
iv[--size] = (char)(255.0 * rand() / RAND_MAX);
// Use userspace rand() function because it handles auto-seeding
iv[--size] = (char)f_rand(0, 255);
}
}
return String(iv, n, AttachString);
Expand Down

2 comments on commit ab6fdeb

@hopeseekr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yeah. Looks good. Your response time on the weekend is incredibly impressive! A++

@FBNeal
Copy link
Contributor

@FBNeal FBNeal commented on ab6fdeb Aug 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this fix has been assigned CVE-2014-5386

Please sign in to comment.