453767 - Passwords containing wide characters causes system error

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[David Lawrence [:dkl]]

When a user attempts to use a password that contains unicode characters (wide), the cgi dies with an internal 500 error. Parsing the error logs reveals:

[Fri Sep 05 01:00:43 2008] [error] Wide character in crypt at /var/www/html/bugzilla/Bugzilla/Auth/Verify/DB.pm line 58

The crypt() function is not able to use those characters it seems.

Comment 1

[Max Kanat-Alexander]

I think we've filed or discussed this before...

Comment 2

[Vitaly Fedrushkov]

Yes we have discussed that on 2008-04-24.

No it was never filed before.  However, it is reproducible on every Bugzilla instance.

QUOTE http://perldoc.perl.org/functions/crypt.html

If using crypt() on a Unicode string (which potentially has characters with codepoints above 255), Perl tries to make sense of the situation by trying to downgrade (a copy of the string) the string back to an eight-bit byte string before calling crypt() (on that copy). If that works, good. If not, crypt() dies with Wide character in crypt .

UNQUOTE

Looks like we have to encode password fields before use.  Quoted-printable??

Comment 3

[A. Shimono [:himorin]]

> If using crypt() on a Unicode string (which potentially has characters with
> codepoints above 255), Perl tries to make sense of the situation by trying to
> downgrade (a copy of the string) the string back to an eight-bit byte string
> before calling crypt() (on that copy). If that works, good. If not, crypt()
> dies with Wide character in crypt .

I think you can do filtering utf8 flag (of perl strings) before crypt()-ing.
Like
    # Crypt the password.
    if (Bugzilla->params->{'utf8'}) {
        utf8::encode($password);
    } 
    my $cryptedpassword = crypt($password, $salt);
@Bugzilla/Util.pm l.478

Comment 4

[A. Shimono [:himorin]]

And,, i could not use IM (to convert into Kanji or even entering Hiragana with Roman-Input) at the password fields, so it might be hard to use multi-byte password at Bugzilla. But, I'm not sure it is desired behavior, b/c I've not checked the CSS.
And it's not common to use multi-byte (Kanji, Hiragana, etc.) passwords in Japanese.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to fix crypt() crash when using wide characters (v1)

The change from comment 3 does in fact fix the crash when passing wide characters to crypt on my test system. Attaching a patch that puts the change in the right places.

Dave

Comment 6

[A. Shimono [:himorin]]

(In reply to comment #5)
> The change from comment 3 does in fact fix the crash when passing wide
> characters to crypt on my test system. Attaching a patch that puts the change
> in the right places.

I think you should patch userprefs.cgi line 88, too.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to fix crypt() crash when using wide characters (v2)

Added fix to userprefs.cgi as well.

Dave

Comment 8

[Vitaly Fedrushkov]

Symptom is clearly reproducible even when 'utf8' is not set.  Perhaps we should encode in any case.

Typing cyrillics on keyboard does not require 'input methods' and is available on any form field in a browser.  Using cyrillic passwords in Windows is common, as it does not require switching from default (Russian) layout to English.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Symptom is clearly reproducible even when 'utf8' is not set.  Perhaps we should
> encode in any case.

  Anybody who doesn't have utf8 set is looking for trouble if they use non-ASCII, We don't fix their problems.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 337069 [details] [diff] [review]
Patch to fix crypt() crash when using wide characters (v2)

Also check utf8::is_utf8($password) just to be safe.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to fix crypt() crash when using wide characters (v3)

Checking for utf8::is_utf8().

Dave

Comment 12

[Max Kanat-Alexander]

Comment on attachment 338188 [details] [diff] [review]
Patch to fix crypt() crash when using wide characters (v3)

Great, looks good to me.

Comment 13

[David Lawrence [:dkl]]

tip:
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.122; previous revision: 1.121
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.75; previous revision: 1.74
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.8; previous revision: 1.7
done

3.2:
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.120.2.1; previous revision: 1.120
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.69.2.4; previous revision: 1.69.2.3
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.7.4.1; previous revision: 1.7
done

Comment 14

[Marc Schumann [:Wurblzap]]

I'm late, so this is just for the record... I like the idea of a wrapper around crypt() better than code duplication.

Comment 15

[David Lawrence [:dkl]]

(In reply to comment #14)
> I'm late, so this is just for the record... I like the idea of a wrapper around
> crypt() better than code duplication.

Should be easy enough to use the current utility function bz_crypt() everywhere that crypt() is used currently. bz_crypt() would just need to take an additional argument $salt that would be used in place of the random one that is generated for you. I can work up a quick patch for this.

Dave

Comment 16

[David Lawrence [:dkl]]

Attachment: Patch to use only bz_crypt() instead of crypt() (v1)

Attaching patch to remove all places where crypt() is used and use bz_crypt() instead. Added optional argument to bz_crypt() to allow a $salt to be passed in instead of using the random one provided.

Dave

Comment 17

[Max Kanat-Alexander]

Comment on attachment 338380 [details] [diff] [review]
Patch to use only bz_crypt() instead of crypt() (v1)

Could you put this on a separate bug targeted at 3.4?

Comment 18

[David Lawrence [:dkl]]

(In reply to comment #17)
> (From update of attachment 338380 [details] [diff] [review])
> Could you put this on a separate bug targeted at 3.4?

Bug 455584