s390x: EVP_CipherInit_ex sequences lead to wrong results#15521
s390x: EVP_CipherInit_ex sequences lead to wrong results#15521juergenchrist wants to merge 2 commits intoopenssl:masterfrom
Conversation
github-actions
bot
added
the
severity: fips change
slontis
added
approval: otc review pending
branch: master
Various different initialization sequences led to bugs on s390x due to caching and processing during key setting. Since, e.g., the direction does not necessarily have to be correct during initialization, this produced bugs in s390x which were not present on other architectures. Fix this by recomputing the function codes on the fly during updates and final operations. Signed-off-by: Juergen Christ <[email protected]>
juergenchrist
force-pushed
the
fix/s390evpinit
branch
from
8cdfb8f to
a6c6035
Compare
test/evp_extra_test.c
Outdated
There was a problem hiding this comment.
Tests normally just do
if (TEST_XXX())
goto err;
There was a problem hiding this comment.
Yep, although it is rather:
if (!TEST_condition(func()))
goto err;
or
ret = TEST_condition(func1())
&& TEST_condition(func2())
.....
@juergenchrist Please look at the other testcases. These TEST_condition calls really help to diagnose what was the erroneous result.
There was a problem hiding this comment.
Thanks for pointing this out. I changed all occurrences but kept the additional info message.
|
The test failure is relevant. |
Various EVP_CipherInit sequences including partial inits and initializations with different "enc" flags caused problems on s390x. Similarly, cipher reinitialization and especially GCM reinitialization with different tag length led to wrong results. Add some unit tests to cover these rather exotic use cases. Signed-off-by: Juergen Christ <[email protected]>
juergenchrist
force-pushed
the
fix/s390evpinit
branch
from
a6c6035 to
5a0bfe3
Compare
t8m
added
approval: review pending
|
One question regarding this: I also find cipher implementations in crypto/evp/e_aes.c. These once are still buggy but I could not figure out how to use them with the 3.0 code. A fix for them for the 1.1.1 branch is in #14900 but I am not sure if it works with 3.0. Can anybody please tell me how to deal with this file? |
|
Configure and build with |
paulidale
added
approval: done
I cannot verify that. I still get the provider implementations used all the time. Even replacing the If that is true I have no idea how I can use the old versions. So the old code looks pretty dead to me and I do not think we need to care about it. But anyway, the fixes in #14900 could be used here, too. |
You are correct. Engines are the only reason most of the legacy code is still needed. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
Merged to master, thanks for the fix. |
Various different initialization sequences led to bugs on s390x due to caching and processing during key setting. Since, e.g., the direction does not necessarily have to be correct during initialization, this produced bugs in s390x which were not present on other architectures. Fix this by recomputing the function codes on the fly during updates and final operations. Signed-off-by: Juergen Christ <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15521)
Various EVP_CipherInit sequences including partial inits and initializations with different "enc" flags caused problems on s390x. Similarly, cipher reinitialization and especially GCM reinitialization with different tag length led to wrong results. Add some unit tests to cover these rather exotic use cases. Signed-off-by: Juergen Christ <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15521)
If an engine implements eg aes-256-gcm how would the legacy platform specific code in e_aes.c be used ? If the platformspecific code in e_aes.c is still somehow relevant in master, first commit from #14900 would be needed for master as well. |
Various different initialization sequences led to bugs on s390x due to caching and processing during key setting. Since, e.g., the direction does not necessarily have to be correct during initialization, this produced bugs in s390x which were not present on other architectures. Fix this by recomputing the function codes on the fly during updates and final operations. Signed-off-by: Juergen Christ <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#15521)
Various EVP_CipherInit sequences including partial inits and initializations with different "enc" flags caused problems on s390x. Similarly, cipher reinitialization and especially GCM reinitialization with different tag length led to wrong results. Add some unit tests to cover these rather exotic use cases. Signed-off-by: Juergen Christ <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#15521)
6 participants
The EVP-API allows users to provide parts and even change the direction (decryption vs encryption or vice versa) without providing all arguments. This led to some errors on s390x where, e.g., a late change in the direction caused the cipher to perform the wrong operation.
This is the master branch equivalent of PR #14900. For more details on why we want this and where the init sequences come from, please see #14900 (comment).
Checklist