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

some tweaks for the provider cipher implementation #12039

Closed
wants to merge 2 commits into from
Closed

some tweaks for the provider cipher implementation #12039

wants to merge 2 commits into from

Conversation

kaduk
Copy link
Contributor

@kaduk kaduk commented Jun 4, 2020

Don't error out if asking for an IV using a buffer that's larger than the IV in question (we do this from libcrypto!).
Also, be more consistent about IV handling, since they can be requested both as an octet string and an octet pointer.

kaduk added 2 commits June 3, 2020 19:12
@kaduk
Set cipher IV as and pointer from providers
cc2c661 
OSSL_CIPHER_PARAM_IV can be accessed both as an octet string and as
an octet pointer (for routines like EVP_CIPHER_CTX_iv() that are
in a nebulous undocumented-and-might-go-away-eventually state),
the latter for when there is need to modify the actual value in
the provider.

Make sure that we consistently try to set it as both the string and pointer
forms (not just octet string) and only fail if neither version succeeds.  The
generic cipher get_ctx_params routine was already doing so, but the
AES-variant-, GCM-, and CCM-specific ones were not.
@kaduk
Allow oversized buffers for provider cipher IV fetch
d4b47d8 
When we're fetching an IV, there's no need to enforce that the
provided buffer is exactly the same size as the IV we want to
write into it.  This might happen, for example, when
EVP_CIPHER_CTX_iv_noconst() passes sizeof(ctx->iv) (that is,
EVP_MAX_IV_LENGTH) for an AES-GCM cipher that uses a shorter IV.
AES-OCB and CCM were also affected.
@kaduk kaduk added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member labels Jun 4, 2020
mattcaswell
mattcaswell requested changes Jun 4, 2020
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Also, be more consistent about IV handling, since they can be requested both as an octet string and an octet pointer.

Where do we request them as an octet pointer? I recall there were concerns with over using octet ptr...but I don't immediately recall what the issue was.

providers/implementations/ciphers/cipher_aes_ocb.c Show resolved Hide resolved
@kaduk
Copy link
Contributor Author

kaduk commented Jun 4, 2020

Where do we request them as an octet pointer? I recall there were concerns with over using octet ptr...but I don't immediately recall what the issue was.

I also remember some discussion, but haven't found exactly where yet (that's a TODO for another change I've got pending, the shape of which will depend on those previous discussions).

As far as requesting an octet pointer, see

openssl/crypto/evp/evp_lib.c

Lines 463 to 475 in 19d15f9

unsigned char *EVP_CIPHER_CTX_iv_noconst(EVP_CIPHER_CTX *ctx)
{
int ok;
unsigned char *v = ctx->iv;
OSSL_PARAM params[2] = { OSSL_PARAM_END, OSSL_PARAM_END };
params[0] =
OSSL_PARAM_construct_octet_ptr(OSSL_CIPHER_PARAM_IV, (void **)&v,
sizeof(ctx->iv));
ok = evp_do_ciph_ctx_getparams(ctx->cipher, ctx->provctx, params);
return ok != 0 ? v : NULL;
}

@kaduk
Copy link
Contributor Author

kaduk commented Jun 12, 2020

@levitte I'm waiting for further input on this one, if you get a chance.

levitte
levitte approved these changes Jun 19, 2020
View reviewed changes
Copy link
Member

@levitte levitte 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.
Sorry for the delay.

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Jun 19, 2020
@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 Jun 20, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Jun 20, 2020
@kaduk
Set cipher IV as octet string and pointer from providers
320d96a 
OSSL_CIPHER_PARAM_IV can be accessed both as an octet string and as
an octet pointer (for routines like EVP_CIPHER_CTX_iv() that are
in a nebulous undocumented-and-might-go-away-eventually state),
the latter for when there is need to modify the actual value in
the provider.

Make sure that we consistently try to set it as both the string and pointer
forms (not just octet string) and only fail if neither version succeeds.  The
generic cipher get_ctx_params routine was already doing so, but the
AES-variant-, GCM-, and CCM-specific ones were not.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #12039)
openssl-machine pushed a commit that referenced this pull request Jun 20, 2020
@kaduk
Allow oversized buffers for provider cipher IV fetch
7cc5e0d 
When we're fetching an IV, there's no need to enforce that the
provided buffer is exactly the same size as the IV we want to
write into it.  This might happen, for example, when
EVP_CIPHER_CTX_iv_noconst() passes sizeof(ctx->iv) (that is,
EVP_MAX_IV_LENGTH) for an AES-GCM cipher that uses a shorter IV.
AES-OCB and CCM were also affected.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #12039)
@kaduk
Copy link
Contributor Author

kaduk commented Jun 20, 2020

Thanks for the review!
Pushed to master; closing.

@kaduk kaduk closed this Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell requested changes

@levitte levitte levitte 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.

None yet
4 participants
@kaduk @openssl-machine @levitte @mattcaswell