From b914ff32034d1bd0fa43d8fa539a725464b0c55c Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Mon, 24 Jan 2011 13:50:03 -0800 Subject: [PATCH] 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 --- Bugzilla/Install/Localconfig.pm | 25 ++++++++++++++-- Bugzilla/Install/Requirements.pm | 6 ++++ Bugzilla/Util.pm | 49 +++++++++++++++++++++++++++++++- mod_perl.pl | 13 +++++++-- 4 files changed, 88 insertions(+), 5 deletions(-) diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm index 8857b7521c..97d93aee9d 100644 --- a/Bugzilla/Install/Localconfig.pm +++ b/Bugzilla/Install/Localconfig.pm @@ -188,7 +188,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 => <{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'; $localconfig->{$name} = $answer->{$name} || $var->{default}; @@ -342,6 +351,18 @@ EOT ["*$var->{name}"]); } } + # When updating site_wide_secret to the new value, don't + # leave the old value behind. + if (grep { $_ eq 'site_wide_secret' } @new_vars) { + my $read = new IO::File($filename, '<') || die "$filename: $!"; + my $text; + { local $/; $text = <$read> } + $read->close; + $text =~ s/^\$site_wide_secret = '\w{256}';$//ms; + my $write = new IO::File($filename, '>') || die "$filename: $!"; + print $write $text; + $write->close; + } my $newstuff = join(', ', @new_vars); print < '3.51', feature => 'Recommended important security fix' }, + { + package => 'Math-Random-Secure', + module => 'Math::Random::Secure', + version => '0.05', + feature => 'Improve cookie and token security', + }, ); my $all_modules = _get_extension_requirements( diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index dbeaaa5d4b..2c4400f416 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -504,9 +504,56 @@ sub bz_crypt { return $cryptedpassword; } +# 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 (eval { require Math::Random::Secure; 1; }) { + $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 1e5c7fc633..f4192ec7f7 100644 --- a/mod_perl.pl +++ b/mod_perl.pl @@ -44,6 +44,9 @@ use Bugzilla::Mailer (); use Bugzilla::Template (); use Bugzilla::Util (); +# For PerlChildInitHandler +eval { require Math::Random::Secure }; + # This means that every httpd child will die after processing # a CGI if it is taking up more than 70MB of RAM all by itself. $Apache2::SizeLimit::MAX_UNSHARED_SIZE = 70000; @@ -53,8 +56,14 @@ my $cgi_path = Bugzilla::Constants::bz_locations()->{'cgi_path'}; # Set up the configuration for the web server my $server = Apache2::ServerUtil->server; my $conf = < AddHandler perl-script .cgi # No need to PerlModule these because they're already defined in mod_perl.pl -- 2.47.2