4

I'm having a problem with some Go code I've written for a password authentication library. The general idea is to provide 2 functions, Check() and New(), which are both provided a password and a 256 bit HMAC key. The Check() function is also provided a 256 bit salt and a 256 bit hash, and returns a boolean. The New() function returns a new, random salt, and it's corresponding hash. Both functions rely on a helper function, hash(), that uses scrypt for key lengthening, and does the real work of generating an output hash.

This was working when I originally wrote it (as evidenced by the fact that I have working test data generated by an earlier, lost revision of the code).

The problem I'm now having is that the Check() function seems to work perfectly when provided with data generated by the old version of the code, but now seems to fail with any data generated by the code's own New() function (which both use the underlying hash() function).

I know, I should have had git version controlling the code from the start! I've learnt my lesson now.

I've grouped the functions, and a quick demo of the problem into one .go file, as below, and added some output for debugging:

package main

import (
    "code.google.com/p/go.crypto/scrypt"
    "crypto/hmac"
    "crypto/rand"
    "crypto/sha256"
    "crypto/subtle"
    "errors"
    "fmt"
    "io"
)

// Constants for scrypt. See code.google.com/p/go.crypto/scrypt
const (
    KEYLENGTH = 32
    N         = 16384
    R         = 8
    P         = 1
)

// hash takes an HMAC key, a password and a salt (as byte slices)
// scrypt transforms the password and salt, and then HMAC transforms the result.
// Returns the resulting 256 bit hash.
func hash(hmk, pw, s []byte) (h []byte, err error) {
    sch, err := scrypt.Key(pw, s, N, R, P, KEYLENGTH)
    if err != nil {
        return nil, err
    }
    hmh := hmac.New(sha256.New, hmk)
    hmh.Write(sch)
    h = hmh.Sum(nil)
    hmh.Reset() // Probably not necessary
    return h, nil
}

// Check takes an HMAC key, a hash to check, a password and a salt (as byte slices)
// Calls hash().
// Compares the resulting 256 bit hash against the check hash and returns a boolean.
func Check(hmk, h, pw, s []byte) (chk bool, err error) {
    // Print the input hash
    fmt.Printf("Hash: %x\nHMAC: %x\nSalt: %x\nPass: %x\n", h, hmk, s, []byte(pw))
    hchk, err := hash(hmk, pw, s)
    if err != nil {
        return false, err
    }
    // Print the hash to compare against
    fmt.Printf("Hchk: %x\n", hchk)
    if subtle.ConstantTimeCompare(h, hchk) != 1 {
        return false, errors.New("Error: Hash verification failed")
    }
    return true, nil
}

// New takes an HMAC key and a password (as byte slices)
// Generates a new salt using "crypto/rand"
// Calls hash().
// Returns the resulting 256 bit hash and salt.
func New(hmk, pw []byte) (h, s []byte, err error) {
    s = make([]byte, KEYLENGTH)
    _, err = io.ReadFull(rand.Reader, s)
    if err != nil {
        return nil, nil, err
    }
    h, err = hash(pw, hmk, s)
    if err != nil {
        return nil, nil, err
    }
    fmt.Printf("Hash: %x\nSalt: %x\nPass: %x\n", h, s, []byte(pw))
    return h, s, nil
}

func main() {

    // Known values that work
    pass := "pleaseletmein"

    hash := []byte{
        0x6f, 0x38, 0x7b, 0x9c, 0xe3, 0x9d, 0x9, 0xff,
        0x6b, 0x1c, 0xc, 0xb5, 0x1, 0x67, 0x1d, 0x11,
        0x8f, 0x72, 0x78, 0x85, 0xca, 0x6, 0x50, 0xd0,
        0xe6, 0x8b, 0x12, 0x9c, 0x9d, 0xf4, 0xcb, 0x29,
    }

    salt := []byte{
        0x77, 0xd6, 0x57, 0x62, 0x38, 0x65, 0x7b, 0x20,
        0x3b, 0x19, 0xca, 0x42, 0xc1, 0x8a, 0x4, 0x97,
        0x48, 0x44, 0xe3, 0x7, 0x4a, 0xe8, 0xdf, 0xdf,
        0xfa, 0x3f, 0xed, 0xe2, 0x14, 0x42, 0xfc, 0xd0,
    }

    hmac := []byte{
        0x70, 0x23, 0xbd, 0xcb, 0x3a, 0xfd, 0x73, 0x48,
        0x46, 0x1c, 0x6, 0xcd, 0x81, 0xfd, 0x38, 0xeb,
        0xfd, 0xa8, 0xfb, 0xba, 0x90, 0x4f, 0x8e, 0x3e,
        0xa9, 0xb5, 0x43, 0xf6, 0x54, 0x5d, 0xa1, 0xf2,
    }

    // Check the known values. This Works.
    fmt.Println("Checking known values...")
    chk, err := Check(hmac, hash, []byte(pass), salt)
    if err != nil {
        fmt.Printf("%s\n", err)
    }
    fmt.Printf("%t\n", chk)

    fmt.Println()

    // Create new hash and salt from the known HMAC and Salt
    fmt.Println("Creating new hash and salt values...")
    h, s, err := New(hmac, []byte(pass))
    if err != nil {
        fmt.Printf("%s\n", err)
    }

    // Check the new values. This Fails!
    fmt.Println("Checking new hash and salt values...")
    chk, err = Check(hmac, h, []byte(pass), s)
    if err != nil {
        fmt.Printf("%s\n", err)
    }
    fmt.Printf("%t\n", chk)
}

I've tried this on both Linux 64bit and Windows8 64bit and it fails on both.

Any help would be much appreciated! As I said, I did have this working at some point, but I seem to have broken it somewhere along the way. I typically only discovered it wasn't working when writing unit tests... I suppose that's what they're for!

Thanks,

Mike.

Intermernet
  • 18,604
  • 4
  • 49
  • 61
  • Why do you use HMAC and SHA-256 when scrypt already uses both? – Luke May 21 '13 at 06:46
  • I directly call functions from both of those packages in the line `hmh := hmac.New(sha256.New, hmk)` so I need to import them. – Intermernet May 21 '13 at 06:54
  • Why are you HMAC'ing the scrypt result? – Luke May 21 '13 at 06:57
  • Please have a look at http://crackstation.net/hashing-security.htm#properhashing which was the inspiration for the library that the code above is representative of. Basically, the HMAC key used there should come from elsewhere (something like https://www.yubico.com/YubiHSM). This means that if the entire DB of users, salts and hashes is compromised, it can't easily be reversed without that separate HMAC. – Intermernet May 21 '13 at 07:08
  • 1
    Salt doesn't need to be secret. Unless you're doing scrypt on the client-side, there is no point in hasing the result of scrypt. The scrypt hash is not reversible, and it takes a lot of computing power to brute-force it. Hashing it again, after doing it on the server, would fall under the section of "Double Hashing & Wacky Hash Functions" of the same article. An HSM is needed when protecting an encryption key. Hashing is one-way. – Luke May 21 '13 at 17:03
  • From the linked article, under "Impossible-to-crack Hashes: Keyed Hashes and Password Hashing Hardware" please see "As long as an attacker can use a hash to check whether a password guess is right or wrong, they can run a dictionary or brute-force attack on the hash. The next step is to add a secret key to the hash so that only someone who knows the key can use the hash to validate a password. This can be accomplished two ways. Either the hash can be encrypted using a cipher like AES, or the secret key can be included in the hash using a keyed hash algorithm like HMAC." – Intermernet May 21 '13 at 17:13
  • I think that's a bit overboard. Assuming the attacker has access to everything, scrypt is designed to make a brute force attack computationally infeasible per hash. – Luke May 21 '13 at 17:34
  • It may be a bit overboard, but that's what's recommended :-) scrypt relies on some constants that need to be increased with processor power. The output hashes change with those constants so, in order to compare the hashes, the constants must remain constant for any given database. With the extra "keyed hash" I don't need to worry about this in 10 years time when everyone has scalable clusters at their fingertips. – Intermernet May 21 '13 at 17:41

1 Answers1

6

You seem to have reversed the arguments to hash() in one of your functions. In Check(), you have:

hchk, err := hash(hmk, pw, s)

While in New() you have:

h, err = hash(pw, hmk, s)

These obviously won't produce the same result leading to the verification failure.

With three similar arguments with the same types like this, mistakes like this aren't too surprising. Perhaps it would be worth seeing whether you could restructure things to let the type system catch this class of error?

James Henstridge
  • 42,244
  • 6
  • 132
  • 114
  • Thanks for that, that's now so obvious it's embarrassing! I've marked as the correct answer, but I don't have enough rep to upvote you. Sorry, I would if I could! I'd been looking at that code for hours and kept looking over the obvious! – Intermernet May 09 '13 at 03:41