Last Comment Bug 476594 - [SECURITY] Calling srand() at startup time in mod_perl makes the same token be generated over and over
: [SECURITY] Calling srand() at startup time in mod_perl makes the same token b...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 3.3
: All All
: -- critical (vote)
: Bugzilla 3.0
Assigned To: Max Kanat-Alexander
: default-qa
:
Mentors:
Depends on:
Blocks: bz-release-322 476618 476622
  Show dependency treegraph
 
Reported: 2009-02-02 22:14 PST by Max Kanat-Alexander
Modified: 2009-02-03 02:46 PST (History)
4 users (show)
mkanat: approval+
mkanat: approval3.2+
mkanat: blocking3.2.2+
mkanat: approval3.0+
mkanat: testcase?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (716 bytes, patch)
2009-02-02 22:24 PST, Max Kanat-Alexander
justdave: review+
Details | Diff | Splinter Review

Description Max Kanat-Alexander 2009-02-02 22:14:01 PST
On bmo, we were noticing that attachments always had the same token, over and over.

We talked to the maintainer of mod_perl, and he pointed out that we might be calling srand() at startup time, which we are, in Bugzilla::Install::Localconfig. We can fix this for now by making LOCALCONFIG_VARS into a sub instead of a constant, but the right fix is to call srand() inside of a ChildInitHandler with a good seed.
Comment 1 Max Kanat-Alexander 2009-02-02 22:24:28 PST
Created attachment 360252 [details] [diff] [review]
v1

We can work out a better solution for HEAD, later, but here's a quick patch that fixes this (and works the way that this always should have worked anyway).
Comment 2 Max Kanat-Alexander 2009-02-02 22:30:30 PST
We need some sort of testcase that proves that we're generating sufficiently random tokens even if we ask for them very quickly.
Comment 3 Philippe M. Chiasson (:gozer) 2009-02-02 22:36:43 PST
Just be careful with this for 2 reasons:
 
This now means that srand() will be called implicitely by each httpd child the first time rand() is called. That's going to equate srand(time). This means that if you have 2 httpd processes started within the same second, they will too follow the same random sequence for their lifetime

Anything that re-introduces a call to rand() at startup will cause this to happen again. 3rd-party modules, etc.

The ideal solution is to add to httpd.conf:

PerlChildInitHandler "sub { srand($some_good_randomness) }"

This way, each time a new httpd child is spawned, it will pick itself some good new randomness.

Picking what to use for $some_good_randomness is always the fun part. I like Math::TrulyRandom or /dev/u?random, but it is not a portable solution.
Comment 4 Philippe M. Chiasson (:gozer) 2009-02-02 22:41:01 PST
(In reply to comment #2)
> We need some sort of testcase that proves that we're generating sufficiently
> random tokens even if we ask for them very quickly.

That's indicating that randomness is probably not the best way to go. A form of HMAC() would probably be a better bet, it's fast and doesn't drain the entropy from the server.

So you could use a token like SHA1(db secret + SHA1(db secret + userid + attachmentid))

Doesn't matter, really. If I understand this feature correctly, you want links to have an unguessable part to them, not a random part.
Comment 5 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-02-02 22:58:39 PST
Comment on attachment 360252 [details] [diff] [review]
v1

This is applied on b.m.o now, and appears to work as advertised. All my tokens are unique now.
Comment 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-02-02 23:38:29 PST
I hadn't seen gozer's comments in here yet when I marked that though...  he's got some good points.  Long term we should probably get something in like he's suggesting, because we never know what else we might use that random token generator for, and we might need something really random at some point.

But for now...

(In reply to comment #4)
> Doesn't matter, really. If I understand this feature correctly, you want links
> to have an unguessable part to them, not a random part.

This is correct.
Comment 7 Max Kanat-Alexander 2009-02-02 23:46:13 PST
(In reply to comment #3)
> The ideal solution is to add to httpd.conf:
> 
> PerlChildInitHandler "sub { srand($some_good_randomness) }"

  Yeah. That's what we're going to do for future trunk releases. We just want some time to make sure that our random goodness is actually good and works on all the platforms we support.
Comment 8 Max Kanat-Alexander 2009-02-03 02:00:30 PST
tip:

Checking in Bugzilla/Install/Localconfig.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Localconfig.pm,v  <--  Localconfig.pm
new revision: 1.16; previous revision: 1.15
done

3.2:

Checking in Bugzilla/Install/Localconfig.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Localconfig.pm,v  <--  Localconfig.pm
new revision: 1.12.2.2; previous revision: 1.12.2.1
done

3.0:

Checking in Bugzilla/Install/Localconfig.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Localconfig.pm,v  <--  Localconfig.pm
new revision: 1.8.2.2; previous revision: 1.8.2.1
done
Comment 9 Max Kanat-Alexander 2009-02-03 02:43:06 PST
Security advisory sent in bug 476618, unlocking this bug.

Note You need to log in before you can comment on or make changes to this bug.