Closed Bug 250897 Opened 19 years ago Closed 19 years ago

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

Categories

(Bugzilla :: Email Notifications, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: joxeankoret, Assigned: bugreport)

Details

(Whiteboard: [fixed in 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1])

Attachments

(3 files, 1 obsolete file)

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...
This simple scripts sends 10 emails in a less time than a second to the same
email address.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.18+
Flags: blocking2.16.7+
Target Milestone: --- → Bugzilla 2.16
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.

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.




Summary: Possible email bomber → token.cgi sends multiple confirmation emails (possible email bomber)
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?
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.
oops .. this one
Attachment #152939 - Attachment is obsolete: true
Attachment #152940 - Flags: review?
Attachment #152941 - Flags: review?
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?
The template still uses it to tell the user when the token will expire
(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 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?
Attachment #152941 - Flags: review? → review+
(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 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()
:-)
Attachment #152940 - Flags: review? → review+
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.
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...
Flags: approval?
Flags: approval2.18?
any answers to my questions in comment 16?
You can use a new field/table in the database that stores the last time that an
e-mail was sended. Is an example.
You answered your own question.. 24 * 6 = 144 messages per day is a relatively
light load.
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?)
Flags: approval2.16?
Whiteboard: patch awaiting checkin right before release
Whiteboard: patch awaiting checkin right before release → [ready for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1]
Assignee: justdave → bugreport
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
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval2.16?
Flags: approval2.16+
Flags: approval+
Resolution: --- → FIXED
Whiteboard: [ready for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1] → [ready for 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1]
Whiteboard: [ready for 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1] → [fixed in 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1]
Group: webtools-security
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.