476594 - [SECURITY] Calling srand() at startup time in mod_perl makes the same token be generated over and over

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

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]

Attachment: 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]

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)]

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)]

(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]]

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]]

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]

(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]

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]

Security advisory sent in bug 476618, unlocking this bug.