Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the gensalt function for the bcrypt x variant ($2x$). #60

Merged
merged 1 commit into from
Nov 11, 2018

Conversation

besser82
Copy link
Owner

This prefix was never intended for use when hashing new passphrases.
The only use case originally intended was to manually edit '$2a$' to '$2x$' in passphrase hashes to knowingly take the risk yet enable users to continue to log in when upgrading systems with buggy bcrypt implementations to fixed versions.

There was never an intent to be able to generate new setting strings with that prefix. The original implementation of the bcrypt gensalt function doesn't allow the use of the prefix '$2x'.
Thus libxcrypt must not, either.

@besser82 besser82 requested a review from zackw November 11, 2018 12:46
besser82 referenced this pull request Nov 11, 2018
The “default” hash, used by crypt_gensalt when its prefix argument is
NULL, was formerly hardcoded in hashes.lst, which meant that if you
disabled that specific hash, crypt_gensalt would go back to failing
when its prefix argument is NULL.  Now, the default will be the
strongest of the hashes that’s enabled, unless all of the STRONG
hashes are disabled, in which case crypt_gensalt still fails when its
prefix argument is NULL.  We assume you know what you’re doing if you
configure the library with nothing but weak hashes, but you must also
acknowledge at runtime that you want to use them for new passphrases.
@codecov-io
Copy link

codecov-io commented Nov 11, 2018

Codecov Report

Merging #60 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #60   +/-   ##
========================================
  Coverage    91.96%   91.96%           
========================================
  Files           32       32           
  Lines         2988     2988           
========================================
  Hits          2748     2748           
  Misses         240      240
Impacted Files Coverage Δ
crypt-bcrypt.c 98.6% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b45474...06e8d9f. Read the comment docs.

Copy link
Collaborator

@solardiz solardiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (subtype != 'a' && subtype != 'b' && subtype != 'x' && subtype != 'y') check in BF_gensalt should have the subtype != 'x' portion dropped. That would revert this code back to what it is in crypt_blowfish.

crypt-bcrypt.c Show resolved Hide resolved
@solardiz solardiz changed the title Remove the gensalt fuction for the bcrypt x variant ($2x$). Remove the gensalt function for the bcrypt x variant ($2x$). Nov 11, 2018
@besser82 besser82 force-pushed the besser82/no_bcrypt_x_gensalt branch from c5fdd66 to c847b54 Compare November 11, 2018 14:58
@besser82
Copy link
Owner Author

The (subtype != 'a' && subtype != 'b' && subtype != 'x' && subtype != 'y') check in BF_gensalt should have the subtype != 'x' portion dropped. That would revert this code back to what it is in crypt_blowfish.

@solardiz Fixed in rebased commit. I've also added a comment in gensalt_bcrypt_x_rn() stating that this prefix must not be used for computing new hashes.

@besser82
Copy link
Owner Author

@zackw Are you fine with this change?

test-gensalt-extradata.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@zackw zackw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the apparently-unrelated changes to the main loop in test-gensalt-extradata, this looks good to me. This is undoubtedly my fault, back when I started hacking up libxcrypt for glibc binary compatibility I did not understand the differences among the bcrypt variants properly and I must have thought lack of gensalt support for $2x$ was an oversight.

test-gensalt-extradata.c Outdated Show resolved Hide resolved
test-gensalt-extradata.c Outdated Show resolved Hide resolved
@besser82
Copy link
Owner Author

Apart from the apparently-unrelated changes to the main loop in test-gensalt-extradata, this looks good to me.

@zackw Do you agree with my explaination?

@besser82 besser82 force-pushed the besser82/no_bcrypt_x_gensalt branch from c847b54 to 1ffdc1a Compare November 11, 2018 15:26
@besser82
Copy link
Owner Author

@zackw Waiting for Travis to finish. I will merge then and push v4.3.2 bugfix release.

This prefix was never intended for use when hashing new passphrases.
The only use case originally intended was to manually edit '$2a$' to
'$2x$' in passphrase hashes to knowingly take the risk yet enable
users to continue to log in when upgrading systems with buggy bcrypt
implementations to fixed versions.

There was never an intent to be able to generate new setting strings
with that prefix.  The original implementation of the bcrypt gensalt
function doesn't allow the use of the prefix '$2x'.
Thus libxcrypt must not, either.
@besser82 besser82 force-pushed the besser82/no_bcrypt_x_gensalt branch from 1ffdc1a to 06e8d9f Compare November 11, 2018 15:51
@besser82 besser82 merged commit 06e8d9f into develop Nov 11, 2018
@besser82 besser82 deleted the besser82/no_bcrypt_x_gensalt branch November 11, 2018 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants