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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

damien-white
Copy link

@damien-white damien-white commented Dec 16, 2020

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. 👍

Copy link

@dils2k dils2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NHAS
Copy link

NHAS commented Dec 20, 2020

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

As noted in here: #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 #289 (comment)

@damien-white
Copy link
Author

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
Copy link

NHAS commented Dec 20, 2020

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.

@NHAS NHAS mentioned this pull request Dec 20, 2020
@damien-white
Copy link
Author

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
Copy link

NHAS commented Dec 21, 2020

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

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

Successfully merging this pull request may close these issues.

3 participants