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

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +
approval3.2 +
blocking3.2.2 +
approval3.0 +
testcase ?

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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.
Summary: Calling rand() at startup time in mod_perl makes the same token be generated over and over → Calling srand() at startup time in mod_perl makes the same token be generated over and over
(Assignee)

Comment 1

8 years ago
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).
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #360252 - Flags: review?(justdave)
Attachment #360252 - Flags: review?(LpSolit)
(Assignee)

Comment 2

8 years ago
We need some sort of testcase that proves that we're generating sufficiently random tokens even if we ask for them very quickly.
Flags: testcase?
(Assignee)

Updated

8 years ago
Blocks: 476597
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.
(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 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.
Attachment #360252 - Flags: review?(justdave) → review+
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.
(Assignee)

Comment 7

8 years ago
(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.
Flags: blocking3.2.2?
(Assignee)

Updated

8 years ago
Flags: blocking3.2.2? → blocking3.2.2+
(Assignee)

Updated

8 years ago
Blocks: 476618
(Assignee)

Comment 8

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: approval3.2+
Flags: approval3.0+
Flags: approval+
Resolution: --- → FIXED
Summary: 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 be generated over and over
(Assignee)

Comment 9

8 years ago
Security advisory sent in bug 476618, unlocking this bug.
Group: bugzilla-security
(Assignee)

Updated

8 years ago
Blocks: 476622
(Assignee)

Updated

8 years ago
Attachment #360252 - Flags: review?(LpSolit)
You need to log in before you can comment on or make changes to this bug.