alexedwards / argon2id Goto Github PK

DecodeHash goes straight to decoding argon2id params without checking first if vals[1] == "argon2id". I think this check is required and a meaningful error must be returned if wrong hash type is provided.

Thank you for this package, it's just what I needed.

Since forever I've relied on bcrypt's simplicity, where there's only one knob to turn, and the default value (10 in Go and PHP) seems sufficient. Argon2 introduces a whole set of knobs, and the defaults across languages vary significantly.

So, I was wondering how the DefaultParams were chosen, and whether the package should provide more guidance on expected defaults.

1. Parallelism
   Would it be safe to recommend always setting this to at least uint8(runtime.NumCPU())?

2. Iterations
   My confusion here was regarding the different defaults between this package, the underlying golang package (x/crypto/argon2), and libsodium.

This package defaults to t=3. libsodium defaults to 2 for the interactive preset, which is also used as the default for PHP's password_hash().

The /x/crypto/argon2 docs for argon2id say:

The draft RFC recommends[2] time=1, and memory=64*1024 is a sensible number. If using that amount of memory (64 MB) is not possible in some contexts then the time parameter can be increased to compensate.

The RFC says:

   The attack cost estimates from [AB16] imply that for Argon2i, 3 passes is 
   almost optimal for the most of reasonable memory sizes, and that for Argon2d
   and Argon2id, 1 pass maximizes the attack costs for the constant
   defender time.

   The Argon2id variant with t=1 and maximum available memory is
   RECOMMENDED as a default setting for all environments.  This setting
   is secure against side-channel attacks and maximizes adversarial
   costs on dedicated bruteforce hardware.

I see the same text in the most recent version: https://tools.ietf.org/html/draft-irtf-cfrg-argon2-10#section-8.3

So, the question here is whether it makes sense to follow the spec in defaulting to 1, and placing emphasis on increasing memory (as the primary hardening parameter)? Should we always aim to make the hashing process as slow as possible, or is there a point at which the result is good enough?

(Benchmarks on my MBP show t=1 with 64M and 4 threads taking around 30ms, t=2 taking around 60ms. bcrypt with 10 rounds taking ~70ms)

Hi, this is regarding the documentation in Changing the Parameter.
Currently it is,

params := &Params{
	Memory:      128 * 1024,
	Iterations:  4,
	Parallelism: 4,
	SaltLength:  16,
	KeyLength:   32,
}
hash, err := argon2id.CreateHash("pa$$word", argon2id.DefaultParams)

Shouldn't it be?

params := &argon2id.Params{
	Memory:      128 * 1024,
	Iterations:  4,
	Parallelism: 4,
	SaltLength:  16,
	KeyLength:   32,
}
hash, err := argon2id.CreateHash("pa$$word", params)

Hi,
first of all thank you for your work! It's great!

I can't find why you use argon2.IDKey instead of argon2.Key. Can you explain me the differences?

t=1

I have a function CreateUser that takes an email and password from the body of the post request and hashes the password using argon2id.CreateHash(requestBody.password, argon2id.DefaultParams) then inserts the email address and hashed password into the database.

I have another function LoginUser that also takes the requestBody.password and uses argon2id.ComparePasswordAndHash(requestBody.password, database.password)
However the comparison always returns false and it appears that the hash created changes under different function scopes - how do I get round this issue?

// curl http://localhost:7777/debug/pprof/heap > heap.out
// [bruno@localhost wrk2]$ go tool pprof heap.out
// File: main
// Type: inuse_space
// Time: Oct 31, 2019 at 10:46am (-03)
// Entering interactive mode (type "help" for commands, "o" for options)
// (pprof) top
// Showing nodes accounting for 6.94GB, 100% of 6.94GB total
// Dropped 7 nodes (cum <= 0.03GB)
// flat flat% sum% cum cum%
// 6.94GB 100% 100% 6.94GB 100% golang.org/x/crypto/argon2.initBlocks
// 0 0% 100% 6.94GB 100% github.com/alexedwards/argon2id.ComparePasswordAndHash
// 0 0% 100% 6.94GB 100% github.com/valyala/fasthttp.(*Server).serveConn
// 0 0% 100% 6.94GB 100% github.com/valyala/fasthttp.(*workerPool).getCh.func1
// 0 0% 100% 6.94GB 100% github.com/valyala/fasthttp.(*workerPool).workerFunc
// 0 0% 100% 6.94GB 100% gitlab.com/???/???/endpoints.Login
// 0 0% 100% 6.94GB 100% gitlab.com/???/???/infra/server.handler
// 0 0% 100% 6.94GB 100% gitlab.com/???/???/model/users.Authenticate
// 0 0% 100% 6.94GB 100% golang.org/x/crypto/argon2.IDKey
// 0 0% 100% 6.94GB 100% golang.org/x/crypto/argon2.deriveKey

Hello! This repo doesn't have any tags but I think it would be good if it did so any project utilizing it could lock onto a specific version to make sure nothing breaks unexpectedly if this package updates in the future.

Thanks!

It would be helpful to add a method to derive keys from hashes.

Proposed method spec:

func IDKeyFromHash(hash string, keyLen uint32) []byte

https://pkg.go.dev/golang.org/x/crypto/argon2#IDKey

Right now I append a pepper onto all passwords (especially useful for people using terrible passes)

Was wondering if this can be added to the params?

I have my password already as []byte. It feels to me a bit silly to have to convert it to string just that then CreateHash calls converts it back to []byte again.

Should this package provider byte variants of CreateHash and others? It has been recently tagged 1.0.0 so probably it is too late to change this now in existing functions.

Vulnerabilities

DepShield reports that this application's usage of golang.org/x:crypto:0.0.0-20190308221718-c2843e01d9a2 results in the following vulnerability(s):

• (CVSS 5.9) [CVE-2019-11840] Use of Insufficiently Random Values

Test for reproduction

func TestBugReproduction(t *testing.T) {
	// "bug" valid hash: $argon2id$v=19$m=65536,t=1,p=2$UDk0zEuIzbt0x3bwkf8Bgw$ihSfHWUJpTgDvNWiojrgcN4E0pJdUVmqCEdRZesx9tE
	ok, _, err := CheckHash("bug", "$argon2id$v=19$m=65536,t=1,p=2$UDk0zEuIzbt0x3bwkf8Bgw$ihSfHWUJpTgDvNWiojrgcN4E0pJdUVmqCEdRZesx9tE")
	if err != nil {
		t.Fatal(err)
	}
	if !ok {
		t.Fatal("expected password to match")
	}

	// changed one last character of the hash
	ok, _, err = CheckHash("bug", "$argon2id$v=19$m=65536,t=1,p=2$UDk0zEuIzbt0x3bwkf8Bgw$ihSfHWUJpTgDvNWiojrgcN4E0pJdUVmqCEdRZesx9tF")
	if err == nil {
		t.Fatal("Hash validation should fail")
	}

	if ok {
		t.Fatal("Hash validation should fail")
	}

       // same can be done with salt
}

The issue is in base64 decrypt function call here https://github.com/alexedwards/argon2id/blob/master/argon2id.go#L165 and here https://github.com/alexedwards/argon2id/blob/master/argon2id.go#L171. If you don't use strict mode for these calls, you end up successfully matching invalid hashes.

More info:

• https://github.com/golang/go/blob/master/src/encoding/base64/base64.go#L86
• https://tools.ietf.org/html/rfc4648#section-3.5 (Section 3.5)

Solution:

• base64.RawStdEncoding.DecodeString calls should be replaced with base64.RawStdEncoding.Strict().DecodeString

One CreateHash function consumes a lot of resources. Because there is code in func initBlocks
var block0 [1024]byte B := make([]block, memory)

Maybe optimize it?