455584 - Use bz_crypt everywhere instead of the crypt() function

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[David Lawrence [:dkl]]

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

From Bugzilla 453767 comment 15:
> 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.

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 1

[Frédéric Buclin]

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

>Index: Bugzilla/Util.pm

>+    if (!defined $salt) {

>+        my $salt = '';

You have to remove |my|, else the scope of the generated salt is limited to the IF block.

There is a problem with your patch, and I cannot understand why. Without the patch, changing your password from userprefs.cgi works fine, but with your patch applied, I get:

Insecure dependency in parameter 3 of DBI::db=HASH(0xa428000)->do method call while running with -T switch at /var/www/html/bugzilla/userprefs.cgi line 108.


I checked, and $password is not tainted in bz_crypt(), even after the call to utf8::encode(). Also, the generated $salt is not tainted either, but the generated $cryptedpassword is tainted.

Comment 2

[Frédéric Buclin]

Attachment: reduced testcase

Run the test script as:

  perl -T test.pl foo bar

The output is:

# perl -T test.pl foo bar
crypted is tainted? yes (expected: yes)
crypted2 is tainted? no (expected: no)
crypted3 is tainted? yes (expected: no)

Note the third line?! I have no explanation for this behavior. Note: I'm using Perl 5.10.0.

Comment 3

[Frédéric Buclin]

I filed http://rt.perl.org/rt3/Public/Bug/Display.html?id=59998 about this problem.

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to use bz_crypt() everywhere instead of crypt() (v2)

Fixed incorrect user of my in Bugzilla::Util::bz_crypt().

I suppose we are now waiting for upstream perl to address the taint issue.

Dave

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> I suppose we are now waiting for upstream perl to address the taint issue.

No way. Even if they fix it today, the fix would only be available in Perl 5.8.9 and 5.10.1. All you have to do is to detaint the crypted password by using trick_taint().

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to use bz_crypt() everywhere instead of crypt() (v3)

Added line to trick_taint($cryptpassword) if is_tainted($password) or is_tainted($salt) as a workaround of the Perl crypt() issue. Tests and works as expected.

Please review
Dave

Comment 7

[Frédéric Buclin]

Comment on attachment 344192 [details] [diff] [review]
Patch to use bz_crypt() everywhere instead of crypt() (v3)

Yes, now it's working fine. r=LpSolit

Comment 8

[Frédéric Buclin]

mkanat: note that I'm in favor of taking it for 3.2 too as we would now have one central place where the 'wide character' bug is fixed instead of the current two places (with duplicated code). This would make our live easier for maintenance.

Comment 9

[David Lawrence [:dkl]]

(In reply to comment #7)
> (From update of attachment 344192 [details] [diff] [review])
> Yes, now it's working fine. r=LpSolit

Frederic, should I go ahead and check this in to tip while we wait to see if this should be added to 3.2 also? or wait and do both at the same time. I ask because normally we close the bug when it is checked in, but I would need to leave it open if it potentially would go into 3.2.

Dave

Comment 10

[Max Kanat-Alexander]

Just check it in to tip. I don't want to take changes to the password-crypting code on a branch so close to its release.

Comment 11

[David Lawrence [:dkl]]

tip:
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.123; previous revision: 1.122
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.77; previous revision: 1.76
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.9; previous revision: 1.8
done