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.

Sign up for GitHub

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

Jump to bottom

Fixes issue #22 (sequential call of encryption API) #23

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Fixes issue #22 (sequential call of encryption API) #23

merged 6 commits into from
Oct 17, 2023

Conversation

ProjectInitiative
Copy link
Contributor

@ProjectInitiative ProjectInitiative commented Oct 16, 2023 •
edited by fadeevab
Loading

Fixes issue #22

@ProjectInitiative
Removing 'self.rng.clone()' as cloning resets the seed is insecure.
82b841a 
- by removing the clone, cocoon has to be mutable now
- all tests pass
- this introduces a potential breaking change, but addresses a pretty big security risk.
@ProjectInitiative
adding test for sequential encrypts
f73f087
@ProjectInitiative
Removing clone from cocoon
4d27896 
- same changes in minicocoon
- updated docs
- removing clone
@ProjectInitiative
removing comment
ad9017b
@ProjectInitiative ProjectInitiative mentioned this pull request Oct 16, 2023
Closed
fadeevab
fadeevab requested changes Oct 16, 2023
View reviewed changes
Copy link
Owner

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

Nice finding. It introduces an API incompatibility though. Need to double-check it before upgrading the crate.

src/lib.rs Outdated Show resolved Hide resolved
src/mini.rs Outdated Show resolved Hide resolved
@fadeevab fadeevab changed the title Fixes issue #22 Fixes issue #22 (sequential call of encryption API) Oct 16, 2023
@fadeevab
Copy link
Owner

fadeevab commented Oct 16, 2023 •
edited
Loading

@ProjectInitiative rustfmt failed in the GitHub actions, need a quick fix from you... Also, maybe let's make 10 encryptions, not 100: to be honest, a few encryptions basically reproduce the issue.

P.S.: I've drafted an advisory. Basically, I need this PR to be released first.

@ProjectInitiative
Copy link
Contributor Author

Pushed requested changes

fadeevab
fadeevab approved these changes Oct 17, 2023
View reviewed changes
Copy link
Owner

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

Some mut are not needed in the examples, I will clean them up in the following commit

@fadeevab fadeevab merged commit 1b63921 into fadeevab:main Oct 17, 2023
3 checks passed
@ProjectInitiative
Copy link
Contributor Author

I did a find and replace for the docs, so that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fadeevab fadeevab fadeevab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ProjectInitiative @fadeevab