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

KMAC buffer overflow fix #14810

Closed
wants to merge 7 commits into from
Closed

KMAC buffer overflow fix #14810

wants to merge 7 commits into from

Conversation

paulidale
Copy link
Contributor

@paulidale paulidale commented Apr 9, 2021

Fixes a buffer overflow (two byte on the stack).
Also increases the length of the customisation string.

Fixes #14795

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Apr 9, 2021
@paulidale paulidale added this to the 3.0.0 beta1 milestone Apr 9, 2021
@paulidale paulidale self-assigned this Apr 9, 2021
@paulidale
Copy link
Contributor Author

I've also run through and converted the sizes to size_t.

Previously there was an off by two error allowing a stack buffer overrun.
Avoided this by allocating a correct sized buffer on the stack.  A side effect
is that the maximum size of the customisation string can be increased.
@paulidale
Copy link
Contributor Author

Should be all happy for a re-review now.

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Looks good to me.. A few negative tests might be good.
e.g Custom string = 256 bytes..

@paulidale
Copy link
Contributor Author

Added an overly long customisation string test case (257 characters).

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Approved if test pass..

@slontis slontis 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 11, 2021
@slontis
Copy link
Member

slontis commented Apr 12, 2021

Pauli can you try to remember to add the #fixes to the description at the top please... ( I added it here and in another recent PR).

openssl-machine pushed a commit that referenced this pull request Apr 12, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14810)
openssl-machine pushed a commit that referenced this pull request Apr 12, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14810)
openssl-machine pushed a commit that referenced this pull request Apr 12, 2021
Previously there was an off by two error allowing a stack buffer overrun.
Avoided this by allocating a correct sized buffer on the stack.  A side effect
is that the maximum size of the customisation string can be increased.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14810)
openssl-machine pushed a commit that referenced this pull request Apr 12, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14810)
@paulidale
Copy link
Contributor Author

Merged to master, I'll try to remember to include the fixes.

@paulidale paulidale closed this Apr 12, 2021
@paulidale paulidale deleted the kmac-fix branch June 9, 2021 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kmac_set_ctx_params() check is wrong.
3 participants