240004 - User forgotten password - system sends email with token - token has "bad" characters

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Paul Wallingford]

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

I am using Mozilla 1.6 amd its email client.  When a user forgets their password
they can be sent an email with a session token to allow them to change their
password.  This token is made up of characaters other than letters and numbers
(in addition to letters and numbers).

The mozilla mail client scans text emails and highlights urls so they are
clickable.  It chokes on some of these extra characters prematurely ternimating
the url click link.  The link brings up a page from bugzilla that states "bad
token".  For a technical user, this is annoying, but mots can figure out what is
happening.  For general users, this can be maddening as it seems the system
doesn't work.



Reproducible: Always
Steps to Reproduce:
Pretend you have forgotten your password and have the system send "it" to you on
the login screen.  If the url contains certain characters, the url link in the
email will not work.

This is an actual example of a link that was sent from our bugzilla system:

http://bugzilla.worldenergylabs.com/token.cgi?a=cxlpw&t=lL9G%2ARB-

The mozilla email client  only highlighted as a clickable link up to the ARB. 
The "-" at the end was not included.

The session token algorithm should be changed to include only A-Z, a-z and 0-9.
 To maintain the same keyspace, the number of characters in the encoding will
have to be increased a bit.




The system gets the token from the password routine (sub GenerateRandomPassword
in globals.pl).  It generates 8 characters randomly from the list 0-9, A-Z, a-z,
-, _, !, @, #, $, %, ^ and *.  This is 71 characters.  The total keyspace of the
generated password or token is 8 * (log 71 / log 2) = 49.20 bits of information.

Option 1: Getting rid of the non alphanumeric characters from the list gives a
keyspace of 47.63 bits.  Pretty damn close and without the problems.  Add a 9th
character instead of only 8 and your keyspace jumps to 53.59 bits which exceeds
the original password routine's keyspace.  (NOTE that all of these are
brute-forceable - you need probably close to 14 characters of strictly
alphanumeric characters, which is 83 bits, before you start seeing actual security.)

Option 2: Another alternative is to leave the password routine alone, but add a
second one for just generating sessions tokens.  These are very short routines
and would not impact the program negatively.

Example:
sub gentoken {
  my $size = (shift or 9);
  return join "", map{ (0..9,a..z,A..Z)[rand 62] } (1..$size);
}

Option 3: A third alternative is to post process the password token, converting
it to a bitstring then back out to a strictly alphanumeric string which would be
longer but have the same keyspace.

The second option above is the best

Comment 1

[Vlad Dascalu]

Hi Paul, thanks for your insight.

Could you write a patch for either option 1 or option 2? I'd prefer option 1
because it implies a smaller patch to review and approve, but option 2 is fine
as well.

We've had problems in the past due to this. I've supported in IRC to keep tokens
only to alphanumeric chars due to encoding issues, but never got around to do it.

I'd like this one for BZ 2.18. Thanks!

Comment 2

[Dave Miller [:justdave]]

This is fixed already in cvs in the last couple days...

*** This bug has been marked as a duplicate of 192775 ***

Comment 3

[Vlad Dascalu]

There are clients that break the URL if they find in the middle "-" or other
special chars?

I don't know, but fixing this would have been a nice way to be on the safe side.

Comment 4

[Paul Wallingford]

Actually, Vlad is right.  Bug 192775 should be changed from resolved.  It is
not.  The characters that are sent, regardless of whether they are at the end of
the URL or not violate the spec.  They need to be escaped.  For instance, once
of the characters used in the token or password is @.  This can only appear in
the domain part to specify a login as in ftp://joe:[e-mail]/ 
Otherwise, it needs to be escaped.  The fix I sent to Vlad, I will post here,
solves all these problems.

=== The solution ===


Rip out the subroutine GenerateRandomPassword in the globals.pl file.

Replace it with the following code:


sub GenerateRandomPassword {
    my $size = (shift or 10); # default to 10 chars if nothing specified

    return join "", map{ (0..9,a..z,A..Z)[rand 62] } (1..$size);
}


That's it.  If there are any routines that call this subroutine with an
argument, the returned password will be slightly weaker than before.  I did a
search throughout the code in 2.16.5 and didn't find any examples of this, so it
should not be an issue.

Comment 5

[Gervase Markham [:gerv]]

Looks good.

Gerv

Comment 6

[Vlad Dascalu]

Putting me as a owner to add it to my to-do list as a responsability to write
the patch and see that it's commited into CVS.

Note to self: add back the rightful contributor as a owner after patching. :)

Comment 7

[Vlad Dascalu]

Feeling like asking Dave about this again, after those comments.

Comment 8

[Dave Miller [:justdave]]

In general this sounds good to me.  CCing a few other folks who might care to
get their opinions.

Comment 9

[Bradley Baetz (:bbaetz)]

Yeah, that looks fine to me.

Comment 10

[Joel Peshkin]

Attachment: Paul's fix in the form of a patch

Comment 11

[Vlad Dascalu]

Comment on attachment 146687 [details] [diff] [review]
Paul's fix in the form of a patch

Tested the new sequence from the command line, it works. Looks good.

Comment 12

[Vlad Dascalu]

Nit: we are trimming a lot of comments and we add none instead.

I do believe that the code is pretty self-explaining, but if anybody else gets a
cool idea of what to add here next to the # signs :-), I can checkin with that
comment added if that's desireable.

Comment 13

[Joel Peshkin]

I think it is a clean enough implementation that the one comment it has is
sufficient.

Comment 14

[Vlad Dascalu]

FIXED

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.264; previous revision: 1.263
done

Comment 15

[Frédéric Buclin]

*** Bug 126373 has been marked as a duplicate of this bug. ***