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

Reduce minimal out length in CRYPTO_128_unwrap_pad #6266

Closed
wants to merge 1 commit into from

Conversation

yhwang
Copy link
Contributor

@yhwang yhwang commented May 16, 2018

In aes_wrap_cipher(), the minimal out buff length is (inlen - 8).
Since it calls CRYPTO_128_unwrap_pad() underneath, it makes sense to
reduce the minimal out length in CRYPTO_128_unwrap_pad() to align to
its caller.

Signed-off-by: Yihong Wang yh.wang@ibm.com

Checklist

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 16, 2018
memcpy(aiv, out, 8);
unsigned char buff[16];
memcpy(buff, in, inlen);
block(buff, buff, key);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not like you're obliged to perform block operation in place, you can simply block(in, buff, key) and skip memcpy. And in case of allocation of intermediate buffer for protected secret value, it's only appropriate to wipe it with OPENSSL_cleanse. Even if it's on stack.

Styling nit. It's considered appropriate to add an empty line after declarations, in this case after buff declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dot-asm thanks for your review. just back from vacation and updated my change per your comments.

In `aes_wrap_cipher()`, the minimal out buff length is `(inlen - 8)`.
Since it calls `CRYPTO_128_unwrap_pad()` underneath, it makes sense to
reduce the minimal out length in `CRYPTO_128_unwrap_pad()` to align to
its caller.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
@yhwang yhwang force-pushed the reduce-out-size-wrap128-pad branch from 1db1bd9 to c1cfe38 Compare May 21, 2018 22:33
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 21, 2018
@richsalz
Copy link
Contributor

Close/open to kick the CLA bot.

@richsalz richsalz closed this May 30, 2018
@richsalz richsalz reopened this May 30, 2018
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 30, 2018
@dot-asm dot-asm added the approval: review pending This pull request needs review by a committer label May 30, 2018
@richsalz richsalz 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 May 30, 2018
@dot-asm
Copy link
Contributor

dot-asm commented May 30, 2018

There is no 2nd approval tick. I suppose you, Rich, meant to approve, but it's shouldn't be mind-reading exercise. You have to click on "approve", just replacing label doesn't formally count.

@richsalz
Copy link
Contributor

oops, closed window before clicking 'send review'

levitte pushed a commit that referenced this pull request May 30, 2018
In `aes_wrap_cipher()`, the minimal out buff length is `(inlen - 8)`.
Since it calls `CRYPTO_128_unwrap_pad()` underneath, it makes sense to
reduce the minimal out length in `CRYPTO_128_unwrap_pad()` to align to
its caller.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #6266)
@dot-asm
Copy link
Contributor

dot-asm commented May 30, 2018

Merged. Thanks!

@dot-asm dot-asm closed this May 30, 2018
@yhwang yhwang deleted the reduce-out-size-wrap128-pad branch May 31, 2018 05:58
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants