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

Fix two bugs with CommonCrypto GCM that can result in invalid output. #1330

Merged
merged 4 commits into from
Sep 17, 2014
Merged

Fix two bugs with CommonCrypto GCM that can result in invalid output. #1330

merged 4 commits into from
Sep 17, 2014

Conversation

reaperhulk
Copy link
Member

Bug 1: Call to AAD but no call to update. Get null tag bytes.
Bug 2: Call to update without call to AAD. Get null ciphertext bytes.

These bugs are ridiculous so there are comments added to try to help us avoid confusion in the future...

Fixes #1329

@reaperhulk
Fix two bugs with CommonCrypto GCM that can result in invalid output.
270b9d4 
Bug #1: Call to AAD but no call to update. Get null tag bytes.
Bug #2: Call to update without call to AAD. Get null ciphertext bytes.

Fixes pyca#1329
@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2277/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2278/

alex
alex reviewed Sep 12, 2014
View reviewed changes
tests/hazmat/backends/test_commoncrypto.py
encryptor = cipher.encryptor()
computed_ct = encryptor.update(pt) + encryptor.finalize()
assert computed_ct == ct
assert encryptor.tag == tag
Copy link
Member

Choose a reason for hiding this comment

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

Should these tests just be run against all backends?

Copy link
Member Author

Choose a reason for hiding this comment

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

We certainly could do that. They should succeed on all backends and it might uncover future backends that have similar issues? I'll move them to be run for all backends.

@alex alex added this to the Sixth Release milestone Sep 12, 2014
@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2279/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ed54991 on reaperhulk:fix-commoncrypto-gcm into b8599c0 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c48abb0 on reaperhulk:fix-commoncrypto-gcm into b8599c0 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c48abb0 on reaperhulk:fix-commoncrypto-gcm into b8599c0 on pyca:master.

@reaperhulk
Copy link
Member Author

Test failures are because OpenSSL 1.0.1 on Ubuntu 12.04 apparently has a similar bug where supplying AAD but no call to update results in an invalid tag. I'll probably need to expand this PR to make an empty call to update in finalize on the OpenSSL backend as well...

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2286/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9a11c00 on reaperhulk:fix-commoncrypto-gcm into b8599c0 on pyca:master.

@Ayrx
Copy link
Contributor

Ayrx commented Sep 17, 2014

LGTM. 👍

alex added a commit that referenced this pull request Sep 17, 2014
@alex
Merge pull request #1330 from reaperhulk/fix-commoncrypto-gcm
506f65b 
Fix two bugs with CommonCrypto GCM that can result in invalid output.
@alex alex merged commit 506f65b into pyca:master Sep 17, 2014
@reaperhulk reaperhulk deleted the fix-commoncrypto-gcm branch September 20, 2014 01:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
backend bugs
Milestone
Sixth Release
Development

Successfully merging this pull request may close these issues.

GCM on OpenSSL without calling authenticate_additional_data does not produce ciphertext.
5 participants
@reaperhulk @jenkins-cryptography @coveralls @Ayrx @alex