-
Notifications
You must be signed in to change notification settings - Fork 803
sc-ossl-compat.h cleanup for OpenSSL and LibreSSL #2506
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
Conversation
|
Both OSX and AppVeyor builds still seem to fail because of some missing API. |
|
Anyone want to look at Appveyer configuration? These look like conflicting configuration for cygwin: Half of the Appveyer builds work, half fail. for example: bash -c "exec 0</dev/null && ./configure --with-cygwin-native --disable-openssl --disable-readline --disable-zlib || cat config.log" cl /O1 /GS /W3 /WX /D_CRT_SECURE_NO_DEPRECATE /D_CRT_NONSTDC_NO_WARNINGS /MT /nologo /DHAVE_CONFIG_H /I....\win32 /I....\src /IC:\openpace-Win32\src /IC:\OpenSSL-v111-Win32\include /DOPENSSL_SECURE_MALLOC_SIZE=65536 /IC:\zlib-Win32 "/IC:\Program Files (x86)\Windows Kits\10\Cryptographic Provider Development Kit\Include" "/IC:\Program Files (x86)\Microsoft CNG Development Kit\Include" "/IC:\Program Files (x86)\WiX Toolset v3.11\SDK\VS2015\inc" /DWINVER=0x0601 /D_WIN32_WINNT=0x0601 /DWIN32_LEAN_AND_MEAN /DENABLE_OPENPACE /DENABLE_OPENSSL /DENABLE_ZLIB /DENABLE_MINIDRIVER /DENABLE_SM /DOPENSC_FEATURES=""pcsc openssl zlib"" /Zi /c sc.c ctx.c log.c errors.c asn1.c base64.c sec.c card.c iso7816.c dir.c ef-atr.c ef-gdo.c padding.c apdu.c simpletlv.c gp.c pkcs15.c pkcs15-cert.c pkcs15-data.c pkcs15-pin.c pkcs15-prkey.c pkcs15-pubkey.c pkcs15-skey.c pkcs15-sec.c pkcs15-algo.c pkcs15-cache.c pkcs15-syn.c pkcs15-emulator-filter.c muscle.c muscle-filesystem.c ctbcs.c reader-ctapi.c reader-pcsc.c reader-openct.c ctx.c(848): error C2220: warning treated as error - no 'object' file generated and ctx.c:847: |
|
Looks like OSX builds has same problem, with ctx.c https://github.com/OpenSC/OpenSC/runs/5039586360?check_suite_focus=true Before this PR, it looks like #if defined(ENABLE_OPENSSL) && defined(OPENSSL_SECURE_MALLOC_SIZE) |
|
support for secure memory was added with openssl/openssl@74924dc , i.e. it was added in 1.1.0 and not present in 1.0.2 |
So what is your point? |
|
As a general note this PR was not trying to address deprecated functions in 3.0.0, but rather it was trying to get ride of parts of sc-ossl-compat.h that are for versions < 1.1.1 and work with LibreSSL 3.4.2 and above as well as OpenSSL 1.1.1 and OpenSSL 3.0.0. More changes will be needed as we address building without deprecated 3.0.0 functions. Note at all the checks are clean, accept for one that is not related to the PR: |
|
While writing the last commit, b0b7cc3 showed that LibreSSL may have started as a fork of 1.0.2 but LibreSSL 3.4.2 these days is more like OpenSSL 1.1.1. LibreSSL and OpenSSL all says OpenSC calls ERR_load_crypto_strings in 9 source files and calls OPENSSL_init_crypto in 4 tools. So b0b7cc3 removes all usages of ERR_load_CRYPTO_strings and friends, including OPENSSL_config is also not needed and deprecated in OpenSSL 1.1.0 Both LibreSSL and OpenSSL have been providing macros and dummy functions for some time. But if We may be close to building OpenSC with OpenSSL-3.0.x with I suspect Anyone want to take a look at |
|
Thank you for polishing this PR and taking care to remove the old cruft and moving the rest of the code. Added some minor comments.
If that is so, can you remove the Line 52 in b71ca16
Yes. We discussed this in #2337 (comment) last year. I think it is not possible to do that right now with 3.0, but I did not ask OpenSSL developers yet. I did not look into that recently, but if you (or @vjardin or @viktorTarasov) have the overview of the use case, it would be great to summarize it in an issue. We might hear there is already a way to do that, they might propose some new API or come up with some other solution. |
|
I have been looking at card-iasec.c too. It might be possible to update it.
Will look closer in next few days.
…On Sun, Feb 6, 2022, 9:35 AM Jakub Jelen ***@***.***> wrote:
Thank you for polishing this PR and taking care to remove the old cruft
and moving the rest of the code. Added some minor comments.
We may be close to building OpenSC with OpenSSL-3.0.x with no-deprecated.
Building this PR and using make -k only card-iasecc.c has many errors. It
is using SHA_CTX, SHA_LONG, SHA256_CTX and members of the structures in
iasecc_qsign_data_sha1 and iasecc_qsign_data_sha256 and does not compile
and thus libopensc is not built. But all other files compile.
If that is so, can you remove the --disable-strict from the openssl 3.0
pipeline in CI (as well as the added CFLAGS):
https://github.com/OpenSC/OpenSC/blob/b71ca16b28c4e1a8d35a326f65bed29a8dec2183/.github/build.sh#L52
I suspect card-iasecc.c does the last part of the hash on the card, but
it not clear how to do that in OpenSSL 3.0.
Anyone want to take a look at card-iasecc.c?
Yes. We discussed this in #2337 (comment)
<#2337 (comment)> last
year. I think it is not possible to do that right now with 3.0, but I did
not ask OpenSSL developers yet. I did not look into that recently, but if
you (or @vjardin <https://github.com/vjardin> or @viktorTarasov
<https://github.com/viktorTarasov>) have the overview of the use case, it
would be great to summarize it in an issue. We might hear there is already
a way to do that, they might propose some new API or come up with some
other solution.
—
Reply to this email directly, view it on GitHub
<#2506 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGTIMPUESFGTKHTI2IIY5TUZ2ILFANCNFSM5NMQCYNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
The last set of commits includes 1f3df78 I Have not tested it, but it compiles on LibreSSL, OpenSSL 1.1.1m and 3.0.1 and 3.0.1-no-deprecated @Jakuje you commented on talking to OpenSSL developers.
The change may be as simple as moving two #ifdefs: |
|
Thank you for looking into that. The changes to iasec look reasonable, but still it looks pretty fragile depending on internal structure. I assume the more correct way would be using some OSSL_PARAMS to get the the content of the internal strucutre, but I did not see any OSSL_PARAM that would provide this info. I added one comment (hope it arrived correctly) to the use of #ifs, which would be better in the sc-ossl-compat.h. |
Can we get this code tested before trying to move the code to sc-ossl-compat.h? |
|
In response to:
I submitted an issue to OpenSSL on the deprecation of the SHA_CTX in 3.0.0 See: openssl/openssl#17688 |
|
@viktorTarasov do you have any comments on this PR? Based on the response in openssl/openssl#17688 it is not likely OpenSSL will provide access to internal hash algorithm data. So we have some choices:
It is not clear if these cards are still in use and these days SHA1 should not be used for signatures so if we want to continue to uses these cards, we would only need to address SHA256. If we do (2.) there are a number of open source routines or examples. These have license issues. Anyone want to pick one? So in any case, I will rework the PR to do (1.). If we decide to do (2.) or OpenSSL provides a way to use their internal hash algorithm data. that can be added. |
|
Any one have IAS-ECC smart card? Developer or user? Based on OpenSSL's responses to openssl/openssl#17688 it is likely the card when used with OpenSSL 3.0 will not do signatures. The last commit 541bfe7 Option (2.) is still possible, by using an existing implementation of SHA256, or OpenSC could write one. |
|
So far no developer or user with interest in the The card drivers implements i "IAS ECC Identification Authentication Signature European Citizen Card", "Technical Specifications Revision: 1.0.1", "21/03/2008" which requires partial hash on card. Does the lack of interest mean there are no active cards that require this driver and it "qualified signatures"? I can find what appears to be newer specifications for sale. Being in the U.S.A. I have no personal stack in this driver. But for now I am intending to clean up this PR with the "qualified signatures" returning not supported if linked with OpenSSL 3.0. P.S. |
f86b68a to
fd13d22
Compare
|
Force-pushed this PR to remove a commit and its revert, squashed another and changed name of another. I believe all this review issues have been addressed, accept for the need for SHA_CTX and SHA256_CTX. These are only needed in the
|
Jakuje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as we do not have any feedback from any card owners, this looks ok to me.
|
Thanks. Feedback and testing are needed. |
Remove unused code and misplaced defines from sc-ossl-compat.h to support OpenSSL 1.1.1 and 3.0.0+ and LibreSSL 3.4.2+ The "inline" routines are no longer needed and have been removed. Several other source files were modified to include additional header files or use newer names for functions or macros which are defined in OpenSSL and LibreSSL. Date: Tue Feb 1 20:58:08 2022 -0600 On branch sc-ossl-compat-cleanup Changes to be committed: modified: sc-ossl-compat.h modified: ../pkcs11/framework-pkcs15.c modified: ../pkcs11/openssl.c modified: ../pkcs15init/pkcs15-westcos.c modified: ../tools/piv-tool.c modified: ../tools/pkcs15-init.c interactive rebase in progress; onto 238eff7 Last command done (1 command done): pick 7dea6a5 sc-ossl-compat.h cleanup Next commands to do (12 remaining commands): pick 0d051d1 Handle CRYPTO_secure_* pick b926a52 Fix reset of bn pointer and return false You are currently rebasing. Changes to be committed: modified: src/libopensc/sc-ossl-compat.h modified: src/pkcs11/framework-pkcs15.c modified: src/pkcs11/openssl.c modified: src/pkcs15init/pkcs15-westcos.c modified: src/tools/piv-tool.c modified: src/tools/pkcs15-init.c
Make sure <openssl/crypto.h> is included and test for !defined(LIBRESSL_VERSION_NUMBER) when using CRYPTO_secure_malloc_init, CRYPTO_secure_malloc_initialized and CRYPTO_secure_malloc_done On branch sc-ossl-compat-cleanup Changes to be committed: modified: ctx.c modified: ../minidriver/minidriver.c modified: ../pkcs11/pkcs11-global.c
On branch sc-ossl-compat-cleanup Changes to be committed: modified: pkcs15-westcos.c
On branch sc-ossl-compat-cleanup Changes to be committed: modified: pkcs15-pubkey.c
On branch sc-ossl-compat-cleanup Changes to be committed: modified: pkcs15-init.c
On branch sc-ossl-compat-cleanup Changes to be committed: modified: p11test_case_common.c modified: p11test_case_common.h modified: p11test_case_ec_derive.c modified: p11test_case_multipart.c modified: p11test_case_pss_oaep.c modified: p11test_case_readonly.c
… OPENSSL_config and CRYPTO_malloc_init LibreSSL and OpenSSL 1.1.1 and 3.0.0+ Have deprecated or removed the need by OpenSC to call ERR_load_CRYPTO_strings, ERR_free_strings, OPENSSL_config and CRYPTO_malloc_init calls to these have been removed On branch sc-ossl-compat-cleanup Changes to be committed: modified: src/libopensc/card-westcos.c modified: src/libopensc/pkcs15-prkey.c modified: src/pkcs15init/pkcs15-oberthur-awp.c modified: src/sm/sslutil.h modified: src/tools/gids-tool.c modified: src/tools/piv-tool.c modified: src/tools/pkcs11-tool.c modified: src/tools/pkcs15-init.c modified: src/tools/sc-hsm-tool.c modified: src/tools/westcos-tool.c interactive rebase in progress; onto 238eff7 Last commands done (7 commands done): pick 86fd639 p11tests replace deprecated EVP_PK_* with EVP_PKEY_* pick eeadd82 Remove calls to deprecated ERR_load_CRYPTO_strings, ERR_free_strings, OPENSSL_config and CRYPTO_malloc_init Next commands to do (6 remaining commands): pick 450f344 More EVP_CIPHER_CTX_reset changes pick d8319dc Fix use of EVP_PKEY_CTX_set_rsa_keygen_pubexp vs EVP_PKEY_CTX_set1_rsa_keygen_pubexp You are currently rebasing. Changes to be committed: modified: src/libopensc/card-westcos.c modified: src/libopensc/pkcs15-prkey.c modified: src/pkcs15init/pkcs15-oberthur-awp.c modified: src/sm/sslutil.h modified: src/tools/gids-tool.c modified: src/tools/piv-tool.c modified: src/tools/pkcs11-tool.c modified: src/tools/pkcs15-init.c modified: src/tools/sc-hsm-tool.c modified: src/tools/westcos-tool.c
Changes to be committed: modified: src/libopensc/card-gpk.c modified: src/libopensc/card-piv.c
…a_keygen_pubexp EVP_PKEY_CTX_set1_rsa_keygen_pubexp is used in 3.0 EVP_PKEY_CTX_set_rsa_keygen_pubexp is used in 1.1.1 and LibreSSL previous commit in this PR tried to use just one for all cases. On branch sc-ossl-compat-cleanup Changes to be committed: modified: src/pkcs15init/pkcs15-westcos.c
Update to use EVP_Digest* for SHA1 or SHA256 in iasecc_qsign_data_sha1 and iasecc_qsign_data_sha256. These routines extract partial hash data before EVP_DigestFinal. The data is then sent to the card to do the final round of the hash. LibreSSL, OpenSSL 1.1.m and 3.0.1 all define in sha.h identical SHA_CTX and SHA256_CTX structures. But if 3.0.1 is built with no-depracated the definition of the structures (and other defines) are undefined. In this these are defind in card-iasecc.c All three versions have a way to access the data in these structures: LibreSSL: md_data = (SHA_CTX *)mdctx->md_data; 1.1.1m: md_data = EVP_MD_CTX_md_data(mdctx); 3.0.1: md_data = EVP_MD_CTX_get0_md_data(mdctx); I believe that the depraction of the structures in 3.0.1 is an oversight as EVP_MD_CTX_get0_md_data (added in 3.0.0) will return the address of the structure but not the definition of the structure. On branch sc-ossl-compat-cleanup Changes to be committed: modified: card-iasecc.c
OpenSSL 3.0.0 EVP_MD_CTX_get0_md_data returns NULL. See discussion openssl/openssl#17688 on what it would take to support this. This commit will allow card-iasecc.c to authenticate but signatures will fail with using `OpenSSL-3.0.0 even when compiling with with different API for example: -DOPENSSL_API_COMPAT=0x10100000L
On branch sc-ossl-compat-cleanup Changes to be committed: modified: pkcs15-oberthur-awp.c
…3.0+ card-iasecc.c is the only place in OpenSC that needs acces to internal hash routines in order to pass intermediate hash data to the card so card can do last round of a hash on the card. LibreSSL and OpenSC-1.1.1 provide access to the SHA_CTX and SHA256_CTX structures. But OpenSSL 3.0 no longer provides access to internal hash data. This commit modifies the iasecc_qsign_data_sha1 and iasecc_qsign_data_sha256 routines to return SC_ERROR_NOT_SUPPORTED when compiled using OpenSSL 3.0.0 or greater. It is not clear at this time if this driver is even used. If the "dsign" routines are needed, a future commit could add a non-OpenSSL SHA1 and SHA256 routine to allow accss to internal data. On branch sc-ossl-compat-cleanup Changes to be committed: modified: card-iasecc.c
OpenSSL is dropping support for engines. If and when GOST developers convert GOST to an OpenSSL provider, we can look at loading the provider if needed. On branch sc-ossl-compat-cleanup Changes to be committed: modified: openssl.c
a9234ee to
d4c4a98
Compare
Rebased against master of today. I also added d4c4a98 to address use of engine for GOST #2586 OpenSC builds using OpenSSL-1.1.1q, OpenSSL 3.0.5 and LibreSSL-3.4.1. OpenSC builds on XUbuntu-22.04 using Ubuntu provided OpenSSL 3.0.2 15 Mar 2022.
This is a lot here, as it gets rid of support for OpenSSL < 1.1.1. So when do you want to drop that too? |
frankmorgner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I didn't notice the change regarding the gost engine, though...
|
nevermind, I just saw that check which disabled gost on openssl >= 3.0 |
On branch sc-ossl-compat-cleanup Changes to be committed: modified: openssl.c
|
Sorry, one more thing, now when the OpenSSL 3.0 build works without the warnings, we should remove the workaround in Line 52 in 58d8099
and build again with -Werror as on any other platform. Then I think we are good to merge this for the 0.23.0 release.
|
On branch sc-ossl-compat-cleanup Changes to be committed: modified: build.sh
I think dropping support for OpenSSL <1.1.1 can wait and will be problematic because of libressl which does not support much of the newer APIs as far as I know. We can create a new issue for this to keep track of that. If there will be no more comments, I will merge it in few days. |
|
While developing the sc-compact-cleanup I was surprised that Libressl has been keeping up with newer versions of OpenSSL. I was using LibreSSL 3.4.1 in February from October 14th, 2021. The latest stable release is 3.5.3 from May 22 See https://www.libressl.org/releases.html The only warning where a number of these: |
Remove unused code and misplaced defines from sc-ossl-compat.h
to support OpenSSL 1.1.1 and 3.0.0+ and LibreSSL 3.4.2+
The "inline" routines are no longer needed and have been removed, as new functions replaced them.
Many of the changes to previous version of
sc-ossl-compat.hadded many#ifndef ... #define ... #endifbut made the assumption that the the header files where already included. But this depended on the order of include files in each source file. This had lead to confusion. This dependency has been removed.The proper way to add
#ifndef ... #define... #endiftosc-ossl-compat.hwould be to include the needed header files in the source files beforesc-ossl-compat.his included, or to add the include tosc-ossl-compat.h.Several other source files were modified to include additional header files
or use newer names for functions or macros which are defined in OpenSSL and LibreSSL.
This replaces #2492 which did not include LibreSSL.
Date: Tue Feb 1 20:58:08 2022 -0600
On branch sc-ossl-compat-cleanup
Changes to be committed:
modified: sc-ossl-compat.h
modified: ../pkcs11/framework-pkcs15.c
modified: ../pkcs11/openssl.c
modified: ../pkcs15init/pkcs15-westcos.c
modified: ../tools/piv-tool.c
modified: ../tools/pkcs15-init.c
A spreed sheet of comments and actions: sc-ossl-compat.pdf You will note that many of the routines used in all three are deprecated in 3.0.0, so these also need to be addressed in future changes.
Tested using PIV card with OpenSSL-1.1.1m, OpenSSL-3.0.1 and LibreSSL-3.4.2
Checklist