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

Always reset IV for CBC mode on cipher context reinitialization #14811

Closed
wants to merge 2 commits into from
Closed

Always reset IV for CBC mode on cipher context reinitialization #14811

wants to merge 2 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Apr 9, 2021

This is necessary to keep compatibility with 1.1.1 implementation
of the CBC mode ciphers.

Fixes #14704

Checklist
  • tests are added or updated

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Apr 9, 2021
paulidale
paulidale approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

CFB mode was mentioned in the bug report. Is that covered here?

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 9, 2021
@t8m
Copy link
Member Author

t8m commented Apr 9, 2021

CFB mode was mentioned in the bug report. Is that covered here?

Good catch! Actually both CFB and OFB modes need this as well.

@t8m t8m requested a review from paulidale April 9, 2021 13:56
@t8m t8m added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Apr 9, 2021
@t8m
Always reset IV for CBC, OFB, and CFB mode on cipher context reinit
2f219d2 
This is necessary to keep compatibility with 1.1.1 implementation
of the CBC, OFB, and CFB mode ciphers.

Fixes openssl#14704
kaduk
kaduk reviewed Apr 9, 2021
View reviewed changes
test/evp_extra_test.c Outdated Show resolved Hide resolved
paulidale
paulidale approved these changes Apr 10, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

One wonders if we should support this IV reset for all mode?
Or do we want it to go away quietly?

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 10, 2021
@kaduk
Copy link
Contributor

kaduk commented Apr 10, 2021

One wonders if we should support this IV reset for all mode?
Or do we want it to go away quietly?

IIRC these are the only modes for which the "IV" changes -- the other modes don't need a reset because it would be a noop.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 11, 2021
openssl-machine pushed a commit that referenced this pull request Apr 12, 2021
@t8m
Always reset IV for CBC, OFB, and CFB mode on cipher context reinit
4ec4b06 
This is necessary to keep compatibility with 1.1.1 implementation
of the CBC, OFB, and CFB mode ciphers.

Fixes #14704

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #14811)
@t8m
Copy link
Member Author

t8m commented Apr 12, 2021

Merged to master. Thank you for the review!

@t8m t8m closed this Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaduk kaduk kaduk left review comments

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EVP_CipherInit_ex(ctx, NULL, NULL, NULL, NULL, -1) no longer resets IV in CBC mode
4 participants
@t8m @kaduk @openssl-machine @paulidale