Skip to content

Check the tag on EVP_Cipher() finalize: Poly1305 and OCB AEADs - #32173

Closed
bbbrumley wants to merge 2 commits into
openssl:masterfrom
bbbrumley:bbb_fixzerolen
Closed

bbbrumley wants to merge 2 commits into
openssl:masterfrom
bbbrumley:bbb_fixzerolen

Conversation

@bbbrumley

Copy link
Copy Markdown
Contributor

At the EVP level, for AEADs EVP_Cipher(ctx, out, NULL, 0) performs finalization. For consistency across AEADs (GCM, etc.), on decrypt it must check the tag. OCB and ChaCha20-Poly1305 took an early exit on the empty message (with or without AAD) and returned success without checking, so a forged tag (with or without AAD) was accepted before this change.

Follow-up to #31555

Assisted-by: Claude:claude-opus-4-8

Checklist
  • tests are added or updated

At the EVP level, for AEADs EVP_Cipher(ctx, out, NULL, 0) performs
finalization. For consistency across AEADs (GCM, etc.), on decrypt it
must check the tag. OCB and ChaCha20-Poly1305 took an early exit on the
empty message (with or without AAD) and returned success without
checking, so a forged tag (with or without AAD) was accepted before this
change.

Follow-up to openssl#31555

Assisted-by: Claude:claude-opus-4-8
@openssl-ci-bot openssl-ci-bot Bot added the approval: review pending This pull request needs review by a committer label Aug 4, 2026
@bbbrumley

Copy link
Copy Markdown
Contributor Author

The SIV change is mostly unrelated, but I rolled it in since I was mucking with that test anyway.

Hope it's OK.

@github-actions github-actions Bot added the severity: fips change The pull request changes FIPS provider sources label Aug 4, 2026

@fwh-dc fwh-dc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor nit, changes looks good!

Comment thread providers/implementations/ciphers/cipher_aes_gcm_siv_hw.c Outdated

@fwh-dc fwh-dc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@openssl-ci-bot openssl-ci-bot Bot 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 Aug 5, 2026
@beldmit beldmit added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Aug 5, 2026
@openssl-machine openssl-machine 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 Aug 6, 2026
@openssl-machine

Copy link
Copy Markdown
Collaborator

This pull request is ready to merge

@jogme

jogme commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Merged to master. Thank you Billy!

@jogme jogme closed this Aug 7, 2026
openssl-machine pushed a commit that referenced this pull request Aug 7, 2026
At the EVP level, for AEADs EVP_Cipher(ctx, out, NULL, 0) performs
finalization. For consistency across AEADs (GCM, etc.), on decrypt it
must check the tag. OCB and ChaCha20-Poly1305 took an early exit on the
empty message (with or without AAD) and returned success without
checking, so a forged tag (with or without AAD) was accepted before this
change.

Follow-up to #31555

Assisted-by: Claude:claude-opus-4-8

Reviewed-by: Frederik Wedel-Heinen <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
MergeDate: Fri Aug  7 13:34:35 2026
(Merged from #32173)
@t8m t8m added triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present and removed approval: review pending This pull request needs review by a committer labels Aug 17, 2026
rickyringler pushed a commit to rickyringler/openssl that referenced this pull request Aug 21, 2026
At the EVP level, for AEADs EVP_Cipher(ctx, out, NULL, 0) performs
finalization. For consistency across AEADs (GCM, etc.), on decrypt it
must check the tag. OCB and ChaCha20-Poly1305 took an early exit on the
empty message (with or without AAD) and returned success without
checking, so a forged tag (with or without AAD) was accepted before this
change.

Follow-up to openssl#31555

Assisted-by: Claude:claude-opus-4-8

Reviewed-by: Frederik Wedel-Heinen <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
MergeDate: Fri Aug  7 13:34:35 2026
(Merged from openssl#32173)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants