All users were logged out of Bugzilla on October 13th, 2018

Use bz_crypt everywhere instead of the crypt() function

RESOLVED FIXED in Bugzilla 3.4

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Bugzilla 3.4
Bug Flags:
approval +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Updated

10 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Version: unspecified → 3.2

Comment 2

10 years ago
Created attachment 343839 [details]
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.
(Assignee)

Comment 4

10 years ago
Created attachment 344189 [details] [diff] [review]
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
Attachment #338932 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #344189 - Flags: review?(LpSolit)

Comment 5

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

Comment 6

10 years ago
Created attachment 344192 [details] [diff] [review]
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
Attachment #344189 - Attachment is obsolete: true
Attachment #344192 - Flags: review?(LpSolit)
Attachment #344189 - Flags: review?(LpSolit)

Comment 7

10 years ago
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
Attachment #344192 - Flags: review?(LpSolit) → review+

Updated

10 years ago
Flags: approval+

Comment 8

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

Comment 9

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

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

Comment 11

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.