Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aead.go should import crypto/rand rather than math/rand #289

Open
mdouchement opened this issue Oct 13, 2020 · 3 comments
Open

aead.go should import crypto/rand rather than math/rand #289

mdouchement opened this issue Oct 13, 2020 · 3 comments

Comments

@mdouchement
Copy link

The nonce should not be generated from a pseudo random input.

@NHAS
Copy link

NHAS commented Oct 25, 2020

This has been sitting here a while Im going to bump it.

This is very important, as said in the documentation for the Seal(...) function.

//The nonce must be NonceSize() bytes long and unique for all   
// time, for a given key.

As math/rand isnt cryptographically secure you have a much higher risk of repeating a nonce for a given key, which may lead to complete compromise of the plaintext, and authentication property.

Its also worth noting that even when you do switch to crypto/rand it is not advisable to send more than 2^32 messages with a specific key, due to the likelihood of nonce repetition.

This is (unfortunately) only said in a comment on the encryption example in the golang docs.

// Never use more than 2^32 random nonces with a given key because of the risk of a repeat.
	nonce := make([]byte, 12)
	if _, err := io.ReadFull(rand.Reader, nonce); err != nil {
		panic(err.Error())
	}

This would mean that after some number of messages < 2^32 a keychange would be required between peers.

@NHAS
Copy link

NHAS commented Oct 25, 2020

The better solution, rather changing the key, would be using an algorithm to produce repetition resistant nonces.
A good basis for this is https://tools.ietf.org/id/draft-mcgrew-iv-gen-01.html, section 4.1 (although I'd recommend giving the rest of it a read as it's quite interesting).

Also, here is the RFC for this, which is a bit more precise: https://tools.ietf.org/html/rfc5116#section-3.2

@damien-white
Copy link

damien-white commented Dec 16, 2020

I have submitted a PR for this. There are no existing conflicts. It's a simple fix and makes the codebase more secure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants