search

dreezey / argon2-password-hash-provider

Argon2 Password Hash Provider for Keycloak
Apache License 2.0
25 stars 16 forks source link

Salt from CredentialModel is wrongly decoded #5

Open jhomola79 opened 3 years ago

jhomola79 commented 3 years ago

Hi, I have found a failing test. This way, salt is wrongly base64 decoded to only 15 bytes instead of 16 and verification fails. Probably more Keycloak bug, but maybe can interest you. 

@Test
public void testArgon2iVerifyPredefinedHashFromCredentialModel() {
    String rawPassword = "tekvica";
    CredentialModel credentialModel = new CredentialModel();
    credentialModel.setSecretData("{\"value\":\"$argon2i$v=19$m=16,t=2,p=1$a3I5aE5QR3N0U3VGU05peg$fHjVdRNXcS+bS2D4Vu+IHw\",\"salt\":\"a3I5aE5QR3N0U3VGU05peg\"}");
    credentialModel.setCredentialData("{\"hashIterations\":1,\"algorithm\":\"argon2\"}");
    PasswordCredentialModel passwordCredentialModel = PasswordCredentialModel.createFromCredentialModel(credentialModel);
    boolean verified = Argon2Helper.verifyPassword(rawPassword, passwordCredentialModel);
    Assert.assertTrue(verified);
}
dreezey commented 3 years ago

Hi @jhomola79 , thanks for the report.

I have looked into this and indeed when the JSON is serialized to PasswordSecretData, the last element of the byte array seems to get lost, see this line, not entirely sure why but it may be worth reporting this to Keycloak team itself?

I have used java.util.Base64 Utility class in your function to verify with the debugger, and this one seems to properly decode it; byte[16] instead of byte[15] produced by Keycloak's Base64 utility class:

@Test
    public void testArgon2iVerifyPredefinedHashFromCredentialModel() {
        String rawPassword = "tekvica";
        String encodedSalt = "a3I5aE5QR3N0U3VGU05peg";
        byte[] salt = Base64.getDecoder().decode(encodedSalt.getBytes(StandardCharsets.UTF_8));
[X1]    CredentialModel credentialModel = new CredentialModel();
        credentialModel.setSecretData("{\"value\":\"$argon2i$v=19$m=16,t=2,p=1$" + encodedSalt + "$fHjVdRNXcS+bS2D4Vu+IHw\",\"salt\":\"" + encodedSalt + "\"}");
        credentialModel.setCredentialData("{\"hashIterations\":1,\"algorithm\":\"argon2\"}");
        PasswordCredentialModel passwordCredentialModel = PasswordCredentialModel.createFromCredentialModel(credentialModel);
[X2]    boolean verified = Argon2Helper.verifyPassword(rawPassword, passwordCredentialModel);
        Assert.assertTrue(verified);
    }

[X] = breakpoint. [X1] => salt = byte[16] [X2] => passwordCredentialModel.secretData.salt = byte[15]

I'll look into bypassing the Keycloak stored salt property, and instead use the one embedded in the encoded Argon2 string: $argon2i$v=19$m=16,t=2,p=1$a3I5aE5QR3N0U3VGU05peg$fHjVdRNXcS+bS2D4Vu+IHw.

jhomola79 commented 3 years ago

Hi, yes I did something similar, fully ignore that last, doubled salt data. I also used that 'underscoreunderscoreSALTunderscoreunderscore' from PasswordSecretData to not even need to send salt value there. Im a bit worried to report to Keycloak, that they have wrongly implemented base64 decoding. : ) If not missed something.

waweber commented 2 years ago

@jhomola79 @dreezey this issue on the Keycloak jira might be related and might benefit from your input: https://issues.redhat.com/projects/KEYCLOAK/issues/KEYCLOAK-18914