0

I'm trying to implement SHA1 password hashing with key-stretching combined with a UUID as a salt. The salt is stored as a column in my usersTable inside the database backend, thus each user will be generated their own unique salt.

My problem is that when I attempt to regenerate the password hashes, the hashes are not matching up and I'm not seeing where the issue lies.

registerUser fetches a user object from createUser, and then persists it into the database. I then use validatePassword to regenerate the hash. See below for full code snippets. This is my first attempt at securing passwords, clearly I have goofed somewhere but I can't spot the bug.

public void registerUser() {
    try {

        Usertable newUser = createUser();

        // user constructed at this point, persist it to the database.
        utx.begin();
        em.persist(newUser);
        utx.commit();

        // Register user with Meter
        Meter myMeter = (Meter) em.createNamedQuery("Meter.findByMeterid").setParameter("meterid", this.meterId).getSingleResult();
        myMeter.setUsername(newUser);

        utx.begin();
        em.merge(myMeter);
        utx.commit();

    } catch (NoSuchAlgorithmException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (NotSupportedException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (SystemException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (RollbackException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (HeuristicMixedException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (HeuristicRollbackException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (SecurityException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (IllegalStateException ex) {
        Logger.getLogger(RegisterBean.class.getName()).log(Level.SEVERE, null, ex);
    }

}

private Usertable createUser() throws NoSuchAlgorithmException {
    Security securityLevel = (Security) em.createNamedQuery("Security.findBySecurityid").setParameter("securityid", SECURITY_LEVEL_USER).getSingleResult();
    Usertable newUser = new Usertable();

    // generate UUID to be used as a salt.
    UUID salt = UUID.randomUUID();

    // generate hash
    MessageDigest msgDigest = MessageDigest.getInstance("SHA-1");
    String inputText = new String(salt.toString() + this.password);
    for (int i = 0; i < ITERATIONS; i++) {
        msgDigest.update(inputText.getBytes());
        byte rawByte[] = msgDigest.digest();
        inputText = (new BASE64Encoder()).encode(rawByte);
    }

    String hashValue = inputText;

    newUser.setUsername(this.userName);
    newUser.setSecurityid(securityLevel);
    newUser.setSalt(salt.toString());
    newUser.setPassword(hashValue);

    return newUser;
}

public void validatePassword(FacesContext context, UIComponent ui, Object passwordField) {
    try {
        UIInput userNameInput = (UIInput) context.getViewRoot().findComponent("regform:userName");
        String userName = (String) userNameInput.getValue();

        Usertable myUser = (Usertable) em.createNamedQuery("Usertable.findByUsername").setParameter("username", userName).getSingleResult();

        // generate hash
        MessageDigest msgDigest = MessageDigest.getInstance("SHA-1");
        String inputText = new String(myUser.getSalt() + this.password);
        for (int i = 0; i < ITERATIONS; i++) {
            msgDigest.update(inputText.getBytes());
            byte rawByte[] = msgDigest.digest();
            inputText = (new BASE64Encoder()).encode(rawByte);
        }

        if (!inputText.equals(myUser.getPassword())) {
            String message = "Username or password incorrect";
            throw new ValidatorException(new FacesMessage(message));
        } else {
            // password is valid, store user into session and mark logged in.
            this.myUser = myUser;
        }

    } catch (NoSuchAlgorithmException ex) {
        Logger.getLogger(LoginBean.class.getName()).log(Level.SEVERE, null, ex);
    } catch (NoResultException ex) {
        String message = "Username or password incorrect";
        throw new ValidatorException(new FacesMessage(message));
    }

}
Nebri
  • 773
  • 2
  • 8
  • 21
  • I don't think that what I write here is the solution. I am just wondering if your loop shouldn't contain a "msgDigest.reset()" before the msgDigest.update() ? I guess you want to start a new SHA-1 calculation for each loop... OK forget it; digest(...) should already reset... (didn't know that) – Ingo Blackman Nov 27 '13 at 00:05
  • Side note: you know that SHA-1 is being [deprecated](http://blogs.technet.com/b/pki/archive/2013/11/12/sha1-deprecation-policy.aspx), right? – ajb Nov 27 '13 at 00:05
  • 1
    yes I know SHA-1 is being obsoleted and removed, this is part of an assignment for academic purposes and SHA-1 was one of my suggestions from the faculty not due to it's strength but due to it being implemented in a wide array of languages. – Nebri Nov 27 '13 at 00:07

1 Answers1

1

Are you sure that this.password is set correctly before validatePassword() is called? Have you inspected the initial value of inputText in both createUser() and validatePassword() to ensure that they match?

This method of hashing passwords is not secure. Because of collisions in the hash function output, the more iterations you perform, the less entropy the resulting hash will contain. To preserve the same level of unpredictability found in the original password, you need to add the password to each round of hashing.

The best way to do this is to use an existing password hashing library, like scrypt or bcrypt, or at least a key derivation function like PBKDF2, which is built into most Java runtimes.

erickson
  • 265,237
  • 58
  • 395
  • 493
  • I am positive that this.password has been set correctly. I've been going over with my debugger for the last couple hours inspecting all the variables and inputs. – Nebri Nov 27 '13 at 00:09
  • @Nebri Well, the hashing itself is not the problem. I would guess the problem is with your persistence mechanism failing to store or retrieve the correct information. Other things to check could be different base-64 encoding details (Apache commons has multiple encoders that include whitespace differently), or running the hash generation and validation methods in different processes that use different character encodings. – erickson Nov 27 '13 at 00:39
  • I say the hashing is not a problem, because I got rid of the persistence stuff and just ran your hashing/validation code myself. This wasn't easy, because of the way you've mingled everything together, but the unit test did validate correctly for me. – erickson Nov 27 '13 at 00:43
  • Well that is very interesting because as soon as I remove the hashing from the application, it works beautifully. Not sure where to move on from this point aside from trying to implement a different hash. Persistence doesn't appear to be the problem on my end. – Nebri Nov 27 '13 at 01:05
  • erickson, I apologize friend. Your initial answer was indeed correct. inputText was not correct, that line should read: String inputText = securePassword((String) passwordField); – Nebri Nov 27 '13 at 01:30