Comments (9)
Hello @codylerum @david-hawley,
this fix is now present in 1.7.1
from password4j.
actually the maximum allowed is 30 and not 31.
password4j/src/main/java/com/password4j/BcryptFunction.java
Lines 766 to 767 in bf3a601
When
logRounds
is 31 (the max allowed value), this shift results in a negative number. Effectively the rounds is set to 0.
from password4j.
@firaja that BadParametersException check is greater than 31, not greater or equals.
But just try it. Set the hash.bcrypt.rounds=31 and measure how long it takes. Or use a debugger. rounds
is set to -2147483648
.
from password4j.
@david-hawley sorry I completely misunderstood your point.
I think that converting rounds
from int
to long
should do the trick.
from password4j.
Converting to long would fix it. I have a draft PR for that. Given the possible issues below I wonder if changing the limit to 30 would be better.
One of the tests in BcryptFunctionTest.genSaltGeneratesCorrectSaltPrefix
is now impractical to run (unless you want the test to take 2 days).
If anyone were currently using 31 rounds this fix is going to cause issues.
- Checking existing passwords is going to take a very long time (days). As will making new passwords.
- When that time passes the check will fail against passwords encoded with the previous version.
I am not sure how/if the PR should have any unit tests for this 31 case.
from password4j.
Converting to long would fix it. I have a draft PR for that. Given the possible issues below I wonder if changing the limit to 30 would be better.
One of the tests in
BcryptFunctionTest.genSaltGeneratesCorrectSaltPrefix
is now impractical to run (unless you want the test to take 2 days).If anyone were currently using 31 rounds this fix is going to cause issues.
- Checking existing passwords is going to take a very long time (days). As will making new passwords.
- When that time passes the check will fail against passwords encoded with the previous version.
I am not sure how/if the PR should have any unit tests for this 31 case.
In this case a unit test is not doable and just checking the parameters encoded in the hash wouldn't add any additional level of correctness that the other tests are already giving.
Even if there may be cases of projects using 31, their hashes would be not correct. A migration approach would be to use version 1.7.0 and update the hash to 30 log rounds. After that install 1.7.1 and migrate back to 31 log rounds. But I think that using 31 log rounds is not usable for any system.
from password4j.
Agreed that no one would legit be using 31 or even 30 rounds. The hypothetical situation would be they tried the max allowed and found it to be fast enough (because it was actually 0) and just went with that.
Ignoring any conversion hints/help I think my PR fixes the main issue. I updated one unit test as well.
from password4j.
Should this be closed?
from password4j.
@codylerum The fix was merged. I don't know the close policy for this project. It hasn't been released yet.
I hope no one is unintentionally using 31 rounds. Mitigation is going to be tricky without something to assist.
from password4j.
Related Issues (20)
- needRehash function to check if password parameters are up to date HOT 4
- Library cannot be loaded on Java8 JVMs HOT 3
- JDK17: java.security.AccessController is deprecated HOT 2
- static block in Password class does not initialize due to NPE HOT 3
- Password4J Module Support HOT 1
- Wrong hashes when characters outside of ISO 8859-1 are used HOT 7
- Support for Balloon Hashing HOT 10
- There is no option to disable console printBanner. HOT 4
- stdout polluted with friendly message HOT 2
- Argon2: fix addRandomSalt
- Additional Exception Handling for Restricted Environments HOT 2
- Align default values to OWASP recommended
- Remove logging functionalities HOT 2
- Add banner HOT 2
- Remove the remaining dependencies
- Argon2 not working as expected HOT 11
- Inconsistency between public and internal APIs HOT 1
- Configurable salt length HOT 8
- Please provide byte array based hashing HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from password4j.