arePasswordsEqualAndValid() is vulnerable to timing attacks

[EdOverflow]

The equals() method performs a character-by-character comparison, which terminates as soon as two characters do not match. This form of comparison is therefore vulnerable to timing attacks:

return stringProperty1.getValueSafe().equals(stringProperty2.getValueSafe()) && isPasswordValid(stringProperty1.getValueSafe());

Link to source code: https://github.com/republique-et-canton-de-geneve/chvote-1-0/blob/master/admin-offline/src/main/java/ch/ge/ve/offlineadmin/controller/PasswordDialogController.java#L195

This comparison is then performed here:

private BooleanBinding bindForValidity(boolean withConfirmation, TextField electionOfficer1Password, TextField electionOfficer2Password, Label errorMessage, Node confirmButton) {
        BooleanBinding passwordsValid = Bindings.createBooleanBinding(
                () -> withConfirmation ? arePasswordsEqualAndValid(electionOfficer1Password.textProperty(), electionOfficer2Password.textProperty()) : isPasswordValid(electionOfficer1Password.getText()),
                electionOfficer1Password.textProperty(),
                electionOfficer2Password.textProperty());
        passwordsValid.addListener((observable, werePasswordsValid, arePasswordsValid) -> {
            confirmButton.setDisable(!arePasswordsValid);
            errorMessage.setVisible(!arePasswordsValid && withConfirmation);
        });
        return passwordsValid;
    }

Link to source code: https://github.com/republique-et-canton-de-geneve/chvote-1-0/blob/master/admin-offline/src/main/java/ch/ge/ve/offlineadmin/controller/PasswordDialogController.java#L171

Unfortunately, I do not know how significant this issue is since you do not have a clear threat model (#17).

[chvote-etat-de-geneve]

The comparison is used only to validate two user inputs made through the application GUI (confirm password pattern): it is a private method of the GUI controller class. Therefore no secret value that the comparison would check against can be guessed using a timing attack.

I understand the need to have a detailed threat model to allow to assess these kinds of issues, however I think that in this current issue, whatever the threat model is, there is no way to exploit the non time constant comparison to gain some knowledge. Please let me know if you see a scenario demonstrating i'm wrong.

[EdOverflow]

Sorry about that. I had an inkling that might be the case.

[chvote-etat-de-geneve]

Don't have to be sorry about that, opensource code is made to be challenged, and we appreciate every request !

[kaworu]

@EdOverflow both password input fields (i.e. password and confirmation) are shown in plaintext through the GUI, so there is no point in trying to brute-force as they are not secret at this stage:

On the other hand I see no penalty to compare them in a constant time. A conservative approach should apply by default the same security measures to "primitive functions" regardless of the context where they are used (which may change in the future). At least, it warrant a comment so that the next person having the same concern can understand at first sight, and to avoid this code to be copied in a context where timing can be an issue.

just my two cents.

[EdOverflow]

@kAworu You raised some very good points.

Just as a matter of interest, why is the JPasswordField class not being used for the password fields?

[chvote-etat-de-geneve]

We have a business requirement to leave the text in clear in these fields:

• the two fields (Commissaire 1 and Commaissaire 2) are completed by two different people
• those users (political people) are not very used to computing and having stars instead of the actual typed characters would lead to repeatedly input errors...

Even with this lax requirement, input errors are often encountered.