250897 - token.cgi sends multiple confirmation emails (possible email bomber)

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Jose Antonio Coret]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

It's very easy to create a simple e-mail bomber that sends 100 e-mails per second.
I create a simple scripts that does this. 

Reproducible: Always
Steps to Reproduce:
1.In your browser if do you want type the following address
http://bugzilla.mozilla.org/token.cgi?a=reqpw&loginname=<registered e-mail address>
2.When done reload (F5 for example)
3.If you reload 100 times, 100 e-mails are sended

Actual Results:  
This works as an e-mail bomber

Expected Results:  
Lock sending e-mail if we are send 3 or more e-mails in the same day, for example...

Comment 1

[Jose Antonio Coret]

Attachment: Sample program to test the "feature"

This simple scripts sends 10 emails in a less time than a second to the same
email address.

Comment 2

[Dave Miller [:justdave]]

Wow, that's fun.  Yeah, there should be a limited number of password reset
attempts per day or something.  (3 per day sounds like plenty to me).  Even a 20
minute delay between each or something.

Comment 3

[Jose Antonio Coret]

I prefer to send a maximun of 3 e-mails per day and, of course, a delay betwen
e-mails of 20 minutes.


(In reply to comment #2)
> Wow, that's fun.  
I known :)

>Yeah, there should be a limited number of password reset
> attempts per day or something.  (3 per day sounds like plenty to me).  

3 per day is the best number for me :)

>Even a 20 minute delay between each or something.

Yes, but, of course, sending a maximun of XXX e-mails per day, for example, 3
e-mails per day.

Comment 4

[Joel Peshkin]

For this particular case, we can protect because we keep track of the tokens in
the DB.  Is this the only such case?

Probably, the best way is to enforce an interval because we don't want to keep a
pile of old tokens around anyway.  Let's just require that an old PW token be 20
minutes old before permitting a new one.  It can be done as a simple check in
IssuePasswordToken().  On the off-chance that someone's SQL server is practicing
time-travel, tokens issued in the future should be considered more than 20
minutes old.


While we're at it, get rid of the call to time() in Token.pm and use SELECT
NOW() from the database.

Comment 5

[Jose Antonio Coret]

I found the same bug in other products and, the developer of one of this wrote me : 

> I was thinking
>we should use one of the human check scripts where the user is prompted
>with some graphical digits that he/she need to read and write in an edit
>box.  If they are entered correctly, then we know it is a human user.
>However, this still doesn't stop humans from abusing the feature.

Is this the better way?

Comment 6

[Joel Peshkin]

Attachment: Patch makes user wait 10 minutes between password resets

This patch makes the database use sql server time for password tokens and
throws an error if the user requests another token before the prior request is
10 minutes old.

Comment 7

[Joel Peshkin]

Attachment: v2 - fixes error message

oops .. this one

Comment 8

[Joel Peshkin]

Attachment: same patch for 2.16 branch

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 152940 [details] [diff] [review]
v2 - fixes error message

>     my $token_ts = time();
>-    my $issuedate = time2str("%Y-%m-%d %H:%M", $token_ts);

Are we still using $token_ts for something or did you just forget to remove it?

Comment 10

[Joel Peshkin]

The template still uses it to tell the user when the token will expire

Comment 11

[Jouni Heikniemi]

(In reply to comment #5)
> >we should use one of the human check scripts where the user is prompted
> >with some graphical digits that he/she need to read and write in an edit
> >box.  If they are entered correctly, then we know it is a human user.
> >However, this still doesn't stop humans from abusing the feature.
> Is this the better way?

Many consider this good, but it's not really that fancy. It mainly works, but
try changing your password on a imageless browser (or if you're visually
impaired/driving a car/jogging - and using a screen reader)! I think 20 minutes
is both simpler and more reliable for this thing we're doing here - of course,
there are other aspects that could benefit more from a graphical interactivity
check.

Comment 12

[Christian Reis]

Comment on attachment 152941 [details] [diff] [review]
same patch for 2.16 branch

(In reply to comment #4)
> IssuePasswordToken().  On the off-chance that someone's SQL server is 
> practicing time-travel, tokens issued in the future should be considered more 
> than 20 minutes old.

You've decided to deal with this using only SQL-based timestamps, right?

I assume you've tested this because I really won't be able to apply it before
this Thursday.

The only thing I can comment on this is that perhaps the 10 minute thing could
be kept in an easier-to-find place than the middle of Token.pm -- considered
using a variable set at the top of the file close to $maxtokenage?

Comment 13

[Dave Miller [:justdave]]

(In reply to comment #10)
> The template still uses it to tell the user when the token will expire

The token expires based on the database time and not the current time, right?
(that's what we're writing into the database by using NOW()).  Should we not
retrieve NOW() from the database for purposes of determining that time, too? 
(since the database might not have the same time as the local machine)

Comment 14

[Christian Reis]

Comment on attachment 152940 [details] [diff] [review]
v2 - fixes error message

Looks okay (and identical).  Though if the user's server is doing the
time-travel thing, then it will be fun when $token_ts is not the same as NOW()
:-)

Comment 15

[Christian Reis]

Do we really want to add code to deal with these corner cases? If the time is
wrong, then the token string printed out will be incorrect; I don't think it's
such a big deal (specially because the database server is the only time provider
being used here which guarantees no harm done even then). I vote let it go as it is.

Comment 16

[Dave Miller [:justdave]]

OK, I'll buy it.  We have enough other inconsistancies between time() and NOW()
elsewhere in the codebase, we can fix those on the trunk as a separate bug.

Next question: Do we have any way to detect how many attempts in the same day? 
If we can't efficiently detect and limit that, then I think we need to make the
delay between attempts longer (like a half hour or so).  Think about the poor
schmuck who's away from his mailbox for a couple days, comes back to find 250
password reset mails in his mailbox.

Granted, that's still not quite a denial of service against his mailbox because
that's probably comparable to the amount of spam the average mailbox gets in
that time period, but still...

Comment 17

[Dave Miller [:justdave]]

any answers to my questions in comment 16?

Comment 18

[Jose Antonio Coret]

You can use a new field/table in the database that stores the last time that an
e-mail was sended. Is an example.

Comment 19

[Joel Peshkin]

You answered your own question.. 24 * 6 = 144 messages per day is a relatively
light load.

Comment 20

[Dave Miller [:justdave]]

I don't have time to mess with doing a 2.16.7 today.  Although this doesn't
necessarily need an advisory, it's still not something I would publicly
advertise prior to having a fix for it easily downloadable.  So lets hold this
until we're ready to do 2.16.7 (maybe in conjunction with 2.18rc3?)

Comment 21

[Dave Miller [:justdave]]

Checked in on trunk:

Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.23; previous revision: 1.22
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.72; previous revision: 1.71
done

and 2.18 branch:

Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.22.2.1; previous revision: 1.22
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.61.2.3; previous revision: 1.61.2.2
done

and 2.16 branch:

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