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

Wrong hashes when characters outside of ISO 8859-1 are used #126

Closed
GisoBartels opened this issue Sep 5, 2023 · 7 comments
Closed

Wrong hashes when characters outside of ISO 8859-1 are used #126

GisoBartels opened this issue Sep 5, 2023 · 7 comments

Comments

@GisoBartels
Copy link

Describe the bug
Since v1.7 the default encoding was changed from UTF-8 to ISO 8859-1.
When hashing strings with characters not supported by ISO 8859-1, they get replaced by the ? character, leading to incorrect hashes.

To Reproduce
Hash a string with unsupported characters (e.g. ) with both v1.6.x and v1.7.x with different results.

Expected behavior
Hashes don't differ for different versions.

@firaja
Copy link
Member

firaja commented Sep 6, 2023

Hi @GisoBartels,

can you provide a working example for this issue?
Thank you

@GisoBartels
Copy link
Author

Sure. I noticed the problem through a failing test in an update PR. See here: GisoBartels/kaster#8
The test went green again, when I compiled the latest version with UTF-8 as default charset.

@firaja
Copy link
Member

firaja commented Sep 7, 2023

From what I see the PR is failing because of a test about password generation not password hashing.

org.junit.ComparisonFailure: expected:<[g9*RlE3CilUmDL$#tyhX]> but was:<[PRqOXQNjODPVVuz6Ol5&]>
at org.junit.Assert.assertEquals(Assert.java:117)
at kotlin.test.junit.JUnitAsserter.assertEquals(JUnitSupport.kt:32)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals(Assertions.kt:63)
at kotlin.test.AssertionsKt.assertEquals(Unknown Source)
at kotlin.test.AssertionsKt__AssertionsKt.assertEquals$default(Assertions.kt:62)
at kotlin.test.AssertionsKt.assertEquals$default(Unknown Source)
at app.passwordkaster.core.PasswordGenerationTest.testPassword(PasswordGenerationTest.kt:110)

I don't see Password4j involved in that test. Please report here a piece of code where Password4j is involved, with input, output and expected output (from external tools if needed). I'm not going to analyze and debug third party libraries 😞

In 1.6.3 (and 1.7.x as well) we have a test with multi-byte unicodes and the test passed in all versions.
The string used was (っ^▿^)۶\uD83C\uDF78\uD83C\uDF1F\uD83C\uDF7A٩(˘◡˘ ) ❌❌ ❌❌❌. So it's weird that a backtick is breaking the hashing process.

Have you tried to convert strings to bytes with a different encoding? Password4j accepts also byte[] as well.
In java:

Password4j.hash("my password".getBytes(StandardCharsets.UTF_8)).with(...);

@firaja firaja moved this to Blocked in Core development Sep 12, 2023
@GisoBartels
Copy link
Author

I created a test, so you can debug for yourself. The test will go green, when the default charset is set to UTF-8
#127

@firaja
Copy link
Member

firaja commented Sep 13, 2023

Hi @GisoBartels thank you for providing a working example.

In the next days I will publish the fix for the issue. This might impact other projects but I think that very few people uses non-ASCII characters for their passwords.

firaja added a commit that referenced this issue Sep 14, 2023
#126: back to UTF-8 encoding for byte[]
@firaja
Copy link
Member

firaja commented Sep 15, 2023

Hi @GisoBartels 1.7.3 is released now!
Thank you for your support 🚀

@firaja firaja closed this as completed Sep 15, 2023
@github-project-automation github-project-automation bot moved this from Blocked to Done in Core development Sep 15, 2023
@GisoBartels
Copy link
Author

Thanks for fixing quickly 😊
I can confirm, that with the new version my tests are green again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

2 participants