Passwords containing wide characters causes system error

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
User Accounts
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Bugzilla 3.2
Bug Flags:
approval +
approval3.2 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
I think we've filed or discussed this before...
OS: Linux → All
Hardware: PC → All

Comment 2

9 years ago
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??
> 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
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.
(Assignee)

Comment 5

9 years ago
Created attachment 337067 [details] [diff] [review]
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
Attachment #337067 - Flags: review?(mkanat)
(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.

Updated

9 years ago
Assignee: user-accounts → dkl
Status: NEW → ASSIGNED
(Assignee)

Comment 7

9 years ago
Created attachment 337069 [details] [diff] [review]
Patch to fix crypt() crash when using wide characters (v2)

Added fix to userprefs.cgi as well.

Dave
Attachment #337067 - Attachment is obsolete: true
Attachment #337069 - Flags: review?(mkanat)
Attachment #337067 - Flags: review?(mkanat)

Comment 8

9 years ago
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

9 years ago
(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

9 years ago
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.
Attachment #337069 - Flags: review?(mkanat) → review-
(Assignee)

Comment 11

9 years ago
Created attachment 338188 [details] [diff] [review]
Patch to fix crypt() crash when using wide characters (v3)

Checking for utf8::is_utf8().

Dave
Attachment #337069 - Attachment is obsolete: true
Attachment #338188 - Flags: review?(mkanat)

Comment 12

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

Great, looks good to me.
Attachment #338188 - Flags: review?(mkanat) → review+

Updated

9 years ago
Flags: approval3.2+
Flags: approval+
Target Milestone: --- → Bugzilla 3.2
(Assignee)

Comment 13

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
I'm late, so this is just for the record... I like the idea of a wrapper around crypt() better than code duplication.
(Assignee)

Comment 15

9 years ago
(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
(Assignee)

Comment 16

9 years ago
Created attachment 338380 [details] [diff] [review]
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
Attachment #338380 - Flags: review?(mkanat)

Comment 17

9 years ago
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?
Attachment #338380 - Flags: review?(mkanat)
(Assignee)

Comment 18

9 years ago
(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
You need to log in before you can comment on or make changes to this bug.