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

Pass zero-length parameters to providers #11920

Closed
wants to merge 3 commits into from
Closed

Conversation

@kaduk
Copy link
Contributor

@kaduk kaduk commented May 22, 2020

This was reported to me as a failure of HKDF with empty IKM (that appears to be a regression from 1.1.1), but also can manifest in other places, such as the RSA OAEP CMS test that I had to provide a fix for.

In general, we have to be able to differentiate between parameter-absent and parameter-present-but-zero-length: some protocols exist that mandate different behavior in these cases. This approach currently feels the most natural to me, though I could be persuaded that an alternate approach is preferred, as I also think that there are aspects of this approach that are ... not exactly pretty.

@kaduk
Copy link
Contributor Author

@kaduk kaduk commented May 22, 2020

Oops, the "regression from 1.1.1 for HKDF" seems to be premature (and I'll need to adjust the commit message); I had started my development on master but forgotten the details of the report I actually received.
That said, the main thrust of the change still seems valid.

crypto/params.c Outdated Show resolved Hide resolved
kaduk added 3 commits May 21, 2020
Add an extra EVP test that provides empty input key material.  It
currently fails, since we lose the information about "key present but
zero length" as we deserialize parameters in the provider.
Prior to this commit, if a string (or octet string) parameter
was present but indicated it was zero-length, we would return success
but with a NULL output value.  This can be problematic in cases where
there is a protocol-level distinction between parameter-absent and
parameter-present-but-zero-length, which is uncommon but can happen.

Since OPENSSL_malloc() returns NULL for zero-length allocation requests,
make a dummy allocation for this case, to give a signal that the string
parameter does exist but has zero length.
As of the previous commit, when a zero-length (string) parameter
is present in the parameters passed to a provider for a given operation,
we will produce an object corresponding to that zero-length parameter,
indicating to the underlying cryptographic operation that the parameter
was passed.  However, rsa_cms_decrypt() was relying on the previous
behavior, and unconditionally tried to call
EVP_PKEY_CTX_set0_rsa_oaep_label() even when the implicit default label
was used (and thus the relevant local variable was still NULL).
In the new setup that distinguishes present-but-empty and absent
more clearly, it is an error to attempt to set a NULL parameter,
even if it is zero-length.

Exercise more caution when setting parameters, and do not call
EVP_PKEY_CTX_set0_rsa_oaep_label() when there is not actually a
label provided.
@kaduk kaduk force-pushed the kaduk:empty-ikm branch to 962af1c May 26, 2020
@kaduk
Copy link
Contributor Author

@kaduk kaduk commented May 26, 2020

force-pushed with updated commit messages and the change that @levitte requested.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented May 28, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request May 28, 2020
Add an extra EVP test that provides empty input key material.  It
currently fails, since we lose the information about "key present but
zero length" as we deserialize parameters in the provider.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11920)
openssl-machine pushed a commit that referenced this pull request May 28, 2020
Prior to this commit, if a string (or octet string) parameter
was present but indicated it was zero-length, we would return success
but with a NULL output value.  This can be problematic in cases where
there is a protocol-level distinction between parameter-absent and
parameter-present-but-zero-length, which is uncommon but can happen.

Since OPENSSL_malloc() returns NULL for zero-length allocation requests,
make a dummy allocation for this case, to give a signal that the string
parameter does exist but has zero length.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11920)
openssl-machine pushed a commit that referenced this pull request May 28, 2020
As of the previous commit, when a zero-length (string) parameter
is present in the parameters passed to a provider for a given operation,
we will produce an object corresponding to that zero-length parameter,
indicating to the underlying cryptographic operation that the parameter
was passed.  However, rsa_cms_decrypt() was relying on the previous
behavior, and unconditionally tried to call
EVP_PKEY_CTX_set0_rsa_oaep_label() even when the implicit default label
was used (and thus the relevant local variable was still NULL).
In the new setup that distinguishes present-but-empty and absent
more clearly, it is an error to attempt to set a NULL parameter,
even if it is zero-length.

Exercise more caution when setting parameters, and do not call
EVP_PKEY_CTX_set0_rsa_oaep_label() when there is not actually a
label provided.

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

@kaduk kaduk commented May 28, 2020

Merged to master; closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants