Closed Bug 1003852 Opened 7 years ago Closed 7 years ago

Digest::SHA 5.82 and newer always croak on wide characters, preventing users with Unicode passwords from logging in

Categories

(Bugzilla :: User Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: a3li, Assigned: LpSolit)

References

Details

Attachments

(1 file)

Sorry for the shameless copy-pasting of the summary, but this issue is basically bug 964113 applied to passwords. ;)

Trying to log in or change your password using a string containing wide characters results in an error when using Digest::SHA 5.820.0:
  Wide character in subroutine entry at /…/Bugzilla/Util.pm line 652.\n

The if part of the conditional block at that code location seems to have UTF encoding applied, while the else branch doesn't. The issue seems to be still present in trunk.
Attached patch patch, v1Splinter Review
Assignee: user-accounts → LpSolit
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8415318 - Flags: review?(dkl)
Target Milestone: --- → Bugzilla 4.4
The right solution is to always encode (that is, pass bytes) to Digest (and crypt).
Checking utf8::is_isutf8() is against best practice. 

I believe the correct fix would be:

use Encode 'encode';
$password = encode('UTF-8', $password);

before the password is pased to Digest or crypt()
Assignee: LpSolit → dylan
Status: ASSIGNED → NEW
woops, sorry about changing those metadatas, I only meant to comment,
Assignee: dylan → LpSolit
Status: NEW → ASSIGNED
Comment on attachment 8415318 [details] [diff] [review]
patch, v1

Review of attachment 8415318 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Util.pm
@@ +626,5 @@
>      }
>  
> +    # Wide characters cause crypt and Digest to die.
> +    if (Bugzilla->params->{'utf8'}) {
> +        utf8::encode($password) if utf8::is_utf8($password);

I think calling is_utf8() here is against best practice for unicode in perl. If $password is not utf8 internally encoded, it just means it is values <127, and encode() would be a no-op anyway.
Attachment #8415318 - Flags: review-
Comment on attachment 8415318 [details] [diff] [review]
patch, v1

We can take up the is_utf8() practice in another bug.
Attachment #8415318 - Flags: review- → review+
Flags: blocking4.4.5+
Attachment #8415318 - Flags: review?(glob)
Attachment #8415318 - Flags: review?(dkl)
Attachment #8415318 - Flags: review+
Comment on attachment 8415318 [details] [diff] [review]
patch, v1

r=glob
Attachment #8415318 - Flags: review?(glob) → review+
Flags: approval4.4+
Flags: approval+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   70e8ab7..d694e68  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   2234f68..1d6e06b  4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.