From: Max Kanat-Alexander <mkanat@bugzilla.org>
Date: Mon, 24 Jan 2011 21:48:17 +0000 (-0800)
Subject: Bug 619594: (CVE-2010-4568) [SECURITY] Improve the randomness of
X-Git-Tag: bugzilla-3.6.4~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c8d6c3679fe89a0123f78f8796c26643ab1a171c;p=thirdparty%2Fbugzilla.git

Bug 619594: (CVE-2010-4568) [SECURITY] Improve the randomness of
generate_random_password, to protect against an account compromise issue
and other critical vulnerabilities.
r=LpSolit, a=LpSolit

https://bugzilla.mozilla.org/show_bug.cgi?id=621591
---

diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm
index d5d76cb799..e15e235070 100644
--- a/Bugzilla/Install/Localconfig.pm
+++ b/Bugzilla/Install/Localconfig.pm
@@ -205,7 +205,9 @@ EOT
     },
     {
         name    => 'site_wide_secret',
-        default => sub { generate_random_password(256) },
+        # 64 characters is roughly the equivalent of a 384-bit key, which
+        # is larger than anybody would ever be able to brute-force.
+        default => sub { generate_random_password(64) },
         desc    => <<EOT
 # This secret key is used by your installation for the creation and
 # validation of encrypted tokens to prevent unsolicited changes,
@@ -323,7 +325,14 @@ sub update_localconfig {
     my @new_vars;
     foreach my $var (LOCALCONFIG_VARS) {
         my $name = $var->{name};
-        if (!defined $localconfig->{$name}) {
+        my $value = $localconfig->{$name};
+        # Regenerate site_wide_secret if it was made by our old, weak
+        # generate_random_password. Previously we used to generate
+        # a 256-character string for site_wide_secret.
+        $value = undef if ($name eq 'site_wide_secret' and defined $value
+                           and length($value) == 256);
+        
+        if (!defined $value) {
             push(@new_vars, $name);
             $var->{default} = &{$var->{default}} if ref($var->{default}) eq 'CODE';
             if (exists $answer->{$name}) {
diff --git a/Bugzilla/Install/Requirements.pm b/Bugzilla/Install/Requirements.pm
index 4002b34303..729f1e2c33 100644
--- a/Bugzilla/Install/Requirements.pm
+++ b/Bugzilla/Install/Requirements.pm
@@ -288,6 +288,12 @@ sub OPTIONAL_MODULES {
         version => '1.999022',
         feature => ['mod_perl'],
     },
+    {
+        package => 'Math-Random-Secure',
+        module  => 'Math::Random::Secure',
+        version => '0.05',
+        feature => ['rand_security'],
+    },
     );
 
     my $extra_modules = _get_extension_requirements('OPTIONAL_MODULES');
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 8442db7da9..074b8fefdc 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -551,9 +551,56 @@ sub bz_crypt {
     return $crypted_password;
 }
 
+# If you want to understand the security of strings generated by this
+# function, here's a quick formula that will help you estimate:
+# We pick from 62 characters, which is close to 64, which is 2^6.
+# So 8 characters is (2^6)^8 == 2^48 combinations. Just multiply 6
+# by the number of characters you generate, and that gets you the equivalent
+# strength of the string in bits.
 sub generate_random_password {
     my $size = shift || 10; # default to 10 chars if nothing specified
-    return join("", map{ ('0'..'9','a'..'z','A'..'Z')[rand 62] } (1..$size));
+    my $rand;
+    if (Bugzilla->feature('rand_security')) {
+        $rand = \&Math::Random::Secure::irand;
+    }
+    else {
+        # For details on why this block works the way it does, see bug 619594.
+        # (Note that we don't do this if Math::Random::Secure is installed,
+        # because we don't need to.)
+        my $counter = 0;
+        $rand = sub {
+            # If we regenerate the seed every 5 characters, our seed is roughly
+            # as strong (in terms of bit size) as our randomly-generated
+            # string itself.
+            _do_srand() if ($counter % 5) == 0;
+            $counter++;
+            return int(rand $_[0]);
+        };
+    }
+    return join("", map{ ('0'..'9','a'..'z','A'..'Z')[$rand->(62)] } 
+                       (1..$size));
+}
+
+sub _do_srand {
+    # On Windows, calling srand over and over in the same process produces
+    # very bad results. We need a stronger seed.
+    if (ON_WINDOWS) {
+        require Win32;
+        # GuidGen generates random data via Windows's CryptGenRandom
+        # interface, which is documented as being cryptographically secure.
+        my $guid = Win32::GuidGen();
+        # GUIDs look like:
+        # {09531CF1-D0C7-4860-840C-1C8C8735E2AD}
+        $guid =~ s/[-{}]+//g;
+        # Get a 32-bit integer using the first eight hex digits.
+        my $seed = hex(substr($guid, 0, 8));
+        srand($seed);
+        return;
+    }
+
+    # On *nix-like platforms, this uses /dev/urandom, so the seed changes
+    # enough on every invocation.
+    srand();
 }
 
 sub validate_email_syntax {
diff --git a/mod_perl.pl b/mod_perl.pl
index a21d5d725b..2de5ca9462 100644
--- a/mod_perl.pl
+++ b/mod_perl.pl
@@ -46,6 +46,9 @@ use Bugzilla::Mailer ();
 use Bugzilla::Template ();
 use Bugzilla::Util ();
 
+# For PerlChildInitHandler
+eval { require Math::Random::Secure };
+
 my ($sizelimit, $maxrequests) = ('', '');
 if (Bugzilla::Constants::ON_WINDOWS) {
     $maxrequests = "MaxRequestsPerChild 25";
@@ -64,8 +67,14 @@ my $cgi_path = Bugzilla::Constants::bz_locations()->{'cgi_path'};
 my $server = Apache2::ServerUtil->server;
 my $conf = <<EOT;
 $maxrequests
-# Make sure each httpd child receives a different random seed (bug 476622)
-PerlChildInitHandler "sub { srand(); }"
+# Make sure each httpd child receives a different random seed (bug 476622).
+# Math::Random::Secure has one srand that needs to be called for
+# every process, and Perl has another. (Various Perl modules still use
+# the built-in rand(), even though we only use Math::Random::Secure in
+# Bugzilla itself, so we need to srand() both of them.) However, 
+# Math::Random::Secure may not be installed, so we call its srand in an
+# eval.
+PerlChildInitHandler "sub { eval { Math::Random::Secure::srand() }; srand(); }"
 <Directory "$cgi_path">
     AddHandler perl-script .cgi
     # No need to PerlModule these because they're already defined in mod_perl.pl
diff --git a/template/en/default/setup/strings.txt.pl b/template/en/default/setup/strings.txt.pl
index 9f8744ec46..d4f49f1021 100644
--- a/template/en/default/setup/strings.txt.pl
+++ b/template/en/default/setup/strings.txt.pl
@@ -62,6 +62,7 @@ END
     feature_mod_perl          => 'mod_perl',
     feature_moving            => 'Move Bugs Between Installations',
     feature_patch_viewer      => 'Patch Viewer',
+    feature_rand_security     => 'Improve cookie and token security',
     feature_smtp_auth         => 'SMTP Authentication',
     feature_updates           => 'Automatic Update Notifications',
     feature_xmlrpc            => 'XML-RPC Interface',