search

perlin-network / noise

A decentralized P2P networking stack written in Go.
https://godoc.org/github.com/perlin-network/noise
MIT License
1.78k stars 213 forks source link

use crypto/rand package instead of math/rand #291

Open damien-white opened 3 years ago

damien-white commented 3 years ago

Reference issue: #289

There is a small mistake that has potentially big security-related consequences. I replaced the "math/rand" package in aead.go to "crypto/rand".

This PR fixes issue #289 as "crypto/rand" is a more secure solution and the package that is officially advised by the Go core team. It produces a stronger random number or psuedo-random number and is a more secure solution than "math/rand". This PR fixes the issue and should be easy to merge as only 2 lines are changed and they are both inside of the imports array (the replacement plus the sorting of imports added a line to the diff).

I looked to see if you had any contributing guidelines as I would love to help contribute further to this project. Fantastic work with everything so far. I love it. 👍

NHAS commented 3 years ago

I just want to note, that while this is a good start, it doesnt remediate the full issue.

As noted in here: https://github.com/perlin-network/noise/issues/289

Just using a properly random source would require the rotation of keys every 2^32 messages.

The best and complete solution would implement a repetition resistant nonce algorithm details of which I've noted here https://github.com/perlin-network/noise/issues/289#issuecomment-716212349

damien-white commented 3 years ago

Thank you @NHAS.

All great and valid points. I will look into revising my PR and see if I can come up with something that satisfies these contraints and solves the issue. I think everyone would agree that using the best, most complete solution would be the most ideal way of handling this.

My apologies, I have been away for the past few days. I will get right on it!

NHAS commented 3 years ago

Thats totally fine! Merging the change as it currently is, is a step in the right direction for sure.

I've actually started writing my own solution to it as well, so I'd be interested to see what we both come up with.

damien-white commented 3 years ago

You work looks really great. Wow. You really got a lot done in a short time. Some of the function renaming adds a few extra lines to the diff but overall, I personally think that the functions (renamed) still make complete sense, the code is clear and it looks good to me.

I don't have any personal say but I am very impressed. Great work. 👍

NHAS commented 3 years ago

Thank you! I've actually just spotted a small issue with it. So I'll give that a patch