Fixed pkcs11-register defaults on macOS and Windows #3053

frankmorgner · 2024-02-27T13:17:23Z

pkcs11-register command line defaults are based on the platform. On macOS/Windows, we don't want to register pkcs#11 modules to avoid conflict with the native API. For this to be effective, we need to build cmdline explicitly on those platforms.

Checklist

Documentation is added or updated
New files have a LGPL 2.1 license statement
PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

frankmorgner · 2024-03-05T11:22:34Z

The generated commandline files are causing problems with -Wshorten-64-to-32, which is why e59586d detects and disables this warning. The strict build, however, explicitly enables this warning - overwriting the local setting for the command line file:

clang -DHAVE_CONFIG_H -I. -I../..  -I../../src -D'DEFAULT_PKCS11_PROVIDER="/usr/local/lib/opensc-pkcs11.so"' -D'DEFAULT_ONEPIN_PKCS11_PROVIDER="/usr/local/lib/onepin-opensc-pkcs11.so"'   -Wno-unknown-warning-option -Wno-shorten-64-to-32 -Wall -Wextra -Wno-unused-parameter -Werror -Wstrict-aliasing=2 -DDEBUG_PROFILE=1 -pedantic -Werror -Wall -Wno-strict-prototypes -Wno-empty-translation-unit -Wno-incompatible-library-redeclaration -Wno-variadic-macros -Wno-unused-function -Wno-incompatible-pointer-types-discards-qualifiers -Wno-gnu-zero-variadic-macro-arguments -Wno-format-pedantic -Wno-pedantic -Wno-incompatible-function-pointer-types -Wshorten-64-to-32 -c -o goid_tool-goid-tool-cmdline.o `test -f 'goid-tool-cmdline.c' || echo './'`goid-tool-cmdline.c

@Jakuje , do you know how to circumvent this?

Jakuje · 2024-03-05T11:53:55Z

@Jakuje , do you know how to circumvent this?

I was fixing this manually in b21d01e#diff-b9804e986581c6f7b6926feede5d5abddf4af651138eca115894fef96632e309

Together with the typo that I managed to push also to upstream:

https://savannah.gnu.org/bugs/?64743
https://git.savannah.gnu.org/cgit/gengetopt.git/commit/?id=bef6597d1d077fe751a89307ea4f7cc5701653ef

I can try to fix also the cast in gengetopt, but given that this commit was first in 4 years since last release, I am not sure how active development is and when we can expect this to get pulled by distributions. I filled this here:

https://savannah.gnu.org/bugs/index.php?65417

For the time being, I believe we should not regenerate the cmdline files as part of the build process (at least not for the goid that is having this issue).

Side note: Once you do the detection of the -Wshorten-64-to-32 support in configure, I think we can remove it from the list of forced CFLAGS in strict build as it becomes and it should be pulled automatically.

Other option to avoid this issue might be not to use the int argument in the goid tool. The short/long already have the casts or do not need it so they should not demonstrate this issue and code change should be quite minimal.

frankmorgner · 2024-03-06T23:40:32Z

Thanks for tracking this to the original source.

In CI, we're currently not using --enable-strict to set default CFLAGS so I think the detection in configure.ac will not help.

I pushed a commit with the suggested workaround of using a short.

Jakuje · 2024-03-07T07:34:09Z

One more thing. Once more regenerate the cmdline files during the build it is a question, whether the resulting files should be in git at all. Indeed, it helps to keep track of changes in the gengetopt tool (if there are some), but otherwise it will just cause these issues. If we add gengetopt as a dependency, we can generate them during the build (and during the make dist).

If we want to keep them in git, I think we should not codespell and code style check them as there is really nothing the PR contributor could do about typos in them and formatting issues. Unfortunately, I do not see an option in https://github.com/yshui/git-clang-format-lint to exclude files (but that should not be an issue), but codespell has an exclude_file, which can contain the files to exclude so thats what

https://github.com/codespell-project/actions-codespell?tab=readme-ov-file#parameter-exclude_file

frankmorgner · 2024-03-08T06:59:34Z

There is no easy way to get gengetopt on Windows - we would have to compile this ourselves. Yes, we are already doing this for OpenSSL and zlib, but those libraries are popular enough to have compiled binaries available somewhere. Basically, developer on-boarding would be a little more complicated on Windows. #2577 would fix that to some extent, but l currently don't have time to take a deeper look into this.

packaging/opensc.spec

frankmorgner · 2024-03-12T18:07:00Z

Finally... after many hickups and corner cases, gengetopt is now an optional build dependency and we don't need to handle cmdline files in the source code distribution.

Jakuje · 2024-03-13T08:52:12Z

configure.ac

+AX_CHECK_COMPILE_FLAG([-Wshorten-64-to-32], [have_shorten_warning_option="yes"], [have_shorten_warning_option="no"])
+AM_CONDITIONAL([HAVE_SHORTEN_WARNING_OPTION], [test "${have_shorten_warning_option}" = "yes"])


I think I mentioned this in some previous comments, given that we have this part of configure, can you remove it from the linux-strict build?

https://github.com/OpenSC/OpenSC/blob/master/.github/workflows/linux-strict.yml#L37

I think you can also remove the line following line altogether:

https://github.com/OpenSC/OpenSC/blob/master/.github/workflows/linux-strict.yml#L72

I will remove the shorten warning from the strict build. However, I'm not sure what you mean with the second part.

the yaml changed since this comment and as I did not provide the permalink, it points to different place. The original line meant is this one:

OpenSC/.github/workflows/linux-strict.yml

Line 72 in f0a3f67

CFLAGS: ${{ env.CFLAGS }} ${{ matrix.compiler == 'clang' && env.CLANG_CFLAGS || '' }}

as we will not have the CLANG_CFLAGS anymore, this line can be simplified too.

so I remove this line entirely!?

Yes, I think that should do.

src/tools/Makefile.am

.appveyor.yml

frankmorgner · 2024-03-13T13:02:17Z

currently, this PR is designed to integrate gengetopt as mandatory build requirement. There is no other way to change default options based on the platform, I think. For example, if you generate a distribution tarball with cmdline files in it, they will contain the default values for your Linux machine. On macOS, we want different defaults when installing from the distribution tarball.

frankmorgner · 2024-03-13T22:24:22Z

We could do the following:

include the cmdline files in the distribution,
but always with macOS/Windows defaults (safe for Linux, but not ideal).
modify src/tools/pkcs11-register.desktop.in to explicitly add --skip-chrome=off --skip-firefox=off, which "restores" the Linux defaults

The source code files for command line interface of pkcs11-register are switched to macOS/Windows defaults in case of adapting the configuration of the browsers. Setting up gengetopt as requirement in Linux is much easier. Also regenerates command line files for all other tools.

…ization this is disabled by default due to Windows/macOS defaults. Note that autostart can completely be disabled by using ./configure --disable-autostart-items

frankmorgner · 2024-09-10T22:54:12Z

We could do the following:

* include the cmdline files in the distribution,

* but always with macOS/Windows defaults (safe for Linux, but not ideal).

* modify src/tools/pkcs11-register.desktop.in to explicitly add `--skip-chrome=off --skip-firefox=off`, which "restores" the Linux defaults

I have reverted my changes to the above approach. This now means, that we don't strictly need gengetopt at compile time and the cmdline options have the defaults for macos/windows. On Linux, where we more likely have gengetopt available, the defaults can be changed with the availability of the tool. Also, the XDG autostart of pkcs11-register enables firefox/chrome integration.

This change also resolves most of the review comments.

Jakuje

After the last part of the comment is resolved, I think we are good to go for 0.26.0

xhanulik · 2024-09-11T12:27:37Z

After discussion with @Jakuje, I'll merge this one to have the release candidate out today. Please, create additional PR with changes from #3053 (comment).

src/tools/goid-tool-cmdline.c

New in 0.26.1; 2025-01-14 General improvements Align allocations of sc_mem_secure_alloc (OpenSC/OpenSC#3281) Fix -O3 gcc optimization failure on amd64 and ppc64el (OpenSC/OpenSC#3299) pkcs11-spy Avoid crash while spying C_GetInterface() (OpenSC/OpenSC#3275) TCOS Fix reading certificate (OpenSC/OpenSC#3296) New in 0.26.0; 2024-11-13 Security CVE-2024-45615: Usage of uninitialized values in libopensc and pkcs15init (OpenSC/OpenSC#3225) CVE-2024-45616: Uninitialized values after incorrect check or usage of APDU response values in libopensc (OpenSC/OpenSC#3225) CVE-2024-45617: Uninitialized values after incorrect or missing checking return values of functions in libopensc (OpenSC/OpenSC#3225) CVE-2024-45618: Uninitialized values after incorrect or missing checking return values of functions in pkcs15init (OpenSC/OpenSC#3225) CVE-2024-45619: Incorrect handling length of buffers or files in libopensc (OpenSC/OpenSC#3225) CVE-2024-45620: Incorrect handling of the length of buffers or files in pkcs15init (OpenSC/OpenSC#3225) CVE-2024-8443: Heap buffer overflow in OpenPGP driver when generating key (OpenSC/OpenSC#3219) General improvements Fix reselection of DF after error in PKCSOpenSC/OpenSC#15 layer (OpenSC/OpenSC#3067) Unify OpenSSL logging throughout code (OpenSC/OpenSC#2922) Extend the p11test to support kryoptic (OpenSC/OpenSC#3141) Fix for error in PCSC reconnection (OpenSC/OpenSC#3150) Fixed various issues reported by OSS-Fuzz and Coverity in drivers, PKCS#11 and PKCS#15 layer PKCS#15 Documentation for PKCS#15 profile files (OpenSC/OpenSC#3132) minidriver Support PinCacheAlwaysPrompt usable for PIV cards (OpenSC/OpenSC#3167) pkcs11-tool Show URI when listing token information (OpenSC/OpenSC#3125) and objects (OpenSC/OpenSC#3130) Do not limit size of objects to 5000 bytes (OpenSC/OpenSC#3174) Add support for AES CMAC (OpenSC/OpenSC#3184) Add support for AES GCM encryption (OpenSC/OpenSC#3195) Add support for RSA OAEP encryption (OpenSC/OpenSC#3175) Add support for HKDF (OpenSC/OpenSC#3193) Implement better support for wrapping and unwrapping (OpenSC/OpenSC#3198) Add support for EdDSA sign and verify (OpenSC/OpenSC#2979) pkcs15-crypt Fix PKCS#1 encoding function to correctly detect padding type (OpenSC/OpenSC#3075) piv-tool Fix RSA key generation (OpenSC/OpenSC#3158) Avoid possible state change when matching unknown card (OpenSC/OpenSC#3112) sc-hsm-tool Cleanse buffer with plaintext key share (OpenSC/OpenSC#3226) pkcs11-register Fix pkcs11-register defaults on macOS and Windows (OpenSC/OpenSC#3053) IDPrime Fix identification of IDPrime 840 cards (OpenSC/OpenSC#3146) Fix container mapping for IDPrime 940 cards (OpenSC/OpenSC#3220) Reorder ATRs for matching cards (OpenSC/OpenSC#3154) OpenPGP Fix state tracking after erasing card (OpenSC/OpenSC#3024) Belpic Disable Applet V1.8 (OpenSC/OpenSC#3109) MICARDO Deactivate driver (OpenSC/OpenSC#3152) SmartCard-HSM Fix signing with secp521r1 signature (OpenSC/OpenSC#3157) eOI Set model via sc_card_ctl function (OpenSC/OpenSC#3189) Rutoken increase the minimum PIN size to support Rutoken ECP BIO (OpenSC/OpenSC#3208) JPKI Adjust parameters for public key in PKCS#15 emulator (OpenSC/OpenSC#3182) D-Trust Add support for ECDSA signatures and ECDH key agreement for D-Trust Signatures Cards 4.1/4.4 (OpenSC/OpenSC#3240, OpenSC/OpenSC#openwrt#3248) Signed-off-by: Daniel Golle <[email protected]>

New in 0.26.1; 2025-01-14 General improvements Align allocations of sc_mem_secure_alloc (OpenSC/OpenSC#3281) Fix -O3 gcc optimization failure on amd64 and ppc64el (OpenSC/OpenSC#3299) pkcs11-spy Avoid crash while spying C_GetInterface() (OpenSC/OpenSC#3275) TCOS Fix reading certificate (OpenSC/OpenSC#3296) New in 0.26.0; 2024-11-13 Security CVE-2024-45615: Usage of uninitialized values in libopensc and pkcs15init (OpenSC/OpenSC#3225) CVE-2024-45616: Uninitialized values after incorrect check or usage of APDU response values in libopensc (OpenSC/OpenSC#3225) CVE-2024-45617: Uninitialized values after incorrect or missing checking return values of functions in libopensc (OpenSC/OpenSC#3225) CVE-2024-45618: Uninitialized values after incorrect or missing checking return values of functions in pkcs15init (OpenSC/OpenSC#3225) CVE-2024-45619: Incorrect handling length of buffers or files in libopensc (OpenSC/OpenSC#3225) CVE-2024-45620: Incorrect handling of the length of buffers or files in pkcs15init (OpenSC/OpenSC#3225) CVE-2024-8443: Heap buffer overflow in OpenPGP driver when generating key (OpenSC/OpenSC#3219) General improvements Fix reselection of DF after error in PKCSOpenSC/OpenSC#15 layer (OpenSC/OpenSC#3067) Unify OpenSSL logging throughout code (OpenSC/OpenSC#2922) Extend the p11test to support kryoptic (OpenSC/OpenSC#3141) Fix for error in PCSC reconnection (OpenSC/OpenSC#3150) Fixed various issues reported by OSS-Fuzz and Coverity in drivers, PKCS#11 and PKCS#15 layer PKCS#15 Documentation for PKCS#15 profile files (OpenSC/OpenSC#3132) minidriver Support PinCacheAlwaysPrompt usable for PIV cards (OpenSC/OpenSC#3167) pkcs11-tool Show URI when listing token information (OpenSC/OpenSC#3125) and objects (OpenSC/OpenSC#3130) Do not limit size of objects to 5000 bytes (OpenSC/OpenSC#3174) Add support for AES CMAC (OpenSC/OpenSC#3184) Add support for AES GCM encryption (OpenSC/OpenSC#3195) Add support for RSA OAEP encryption (OpenSC/OpenSC#3175) Add support for HKDF (OpenSC/OpenSC#3193) Implement better support for wrapping and unwrapping (OpenSC/OpenSC#3198) Add support for EdDSA sign and verify (OpenSC/OpenSC#2979) pkcs15-crypt Fix PKCS#1 encoding function to correctly detect padding type (OpenSC/OpenSC#3075) piv-tool Fix RSA key generation (OpenSC/OpenSC#3158) Avoid possible state change when matching unknown card (OpenSC/OpenSC#3112) sc-hsm-tool Cleanse buffer with plaintext key share (OpenSC/OpenSC#3226) pkcs11-register Fix pkcs11-register defaults on macOS and Windows (OpenSC/OpenSC#3053) IDPrime Fix identification of IDPrime 840 cards (OpenSC/OpenSC#3146) Fix container mapping for IDPrime 940 cards (OpenSC/OpenSC#3220) Reorder ATRs for matching cards (OpenSC/OpenSC#3154) OpenPGP Fix state tracking after erasing card (OpenSC/OpenSC#3024) Belpic Disable Applet V1.8 (OpenSC/OpenSC#3109) MICARDO Deactivate driver (OpenSC/OpenSC#3152) SmartCard-HSM Fix signing with secp521r1 signature (OpenSC/OpenSC#3157) eOI Set model via sc_card_ctl function (OpenSC/OpenSC#3189) Rutoken increase the minimum PIN size to support Rutoken ECP BIO (OpenSC/OpenSC#3208) JPKI Adjust parameters for public key in PKCS#15 emulator (OpenSC/OpenSC#3182) D-Trust Add support for ECDSA signatures and ECDH key agreement for D-Trust Signatures Cards 4.1/4.4 (OpenSC/OpenSC#3240, OpenSC/OpenSC##3248) Signed-off-by: Daniel Golle <[email protected]>

frankmorgner force-pushed the register-defaults branch 3 times, most recently from fdb35e5 to e59586d Compare February 28, 2024 14:58

frankmorgner force-pushed the register-defaults branch 3 times, most recently from 09c9794 to 4ad2952 Compare March 11, 2024 14:30

Jakuje reviewed Mar 11, 2024

View reviewed changes

packaging/opensc.spec Outdated Show resolved Hide resolved

frankmorgner force-pushed the register-defaults branch 3 times, most recently from d2b4e45 to 0e74297 Compare March 12, 2024 18:05

Jakuje reviewed Mar 13, 2024

View reviewed changes

frankmorgner added 3 commits September 10, 2024 23:31

pkcs11-register: Chrome supports native crypto modules like Firefox

fc6eab1

include .dirstamp in gitignore

606e205

frankmorgner force-pushed the register-defaults branch from e7a0e37 to 4ce8791 Compare September 10, 2024 22:00

frankmorgner added 5 commits September 11, 2024 00:49

fixed shorten-64-to-32 warning for gengetopt generated files

5c4910e

goid-tool: workaround for compiler warning of generated code

bfc0d59

fixed dtrust-tool build (extension)

f57ca28

fixed formatting

3aa31b0

XDG autostart: launch pkcs11-register WITH Firefox and Chrome initial…

67c1b90

…ization this is disabled by default due to Windows/macOS defaults. Note that autostart can completely be disabled by using ./configure --disable-autostart-items

frankmorgner force-pushed the register-defaults branch from 4ce8791 to 67c1b90 Compare September 10, 2024 22:49

Jakuje approved these changes Sep 11, 2024

View reviewed changes

xhanulik approved these changes Sep 11, 2024

View reviewed changes

src/tools/goid-tool-cmdline.c Outdated Show resolved Hide resolved

Fix spelling error

a69913c

xhanulik merged commit 67929fc into OpenSC:master Sep 11, 2024

		AX_CHECK_COMPILE_FLAG([-Wshorten-64-to-32], [have_shorten_warning_option="yes"], [have_shorten_warning_option="no"])
		AM_CONDITIONAL([HAVE_SHORTEN_WARNING_OPTION], [test "${have_shorten_warning_option}" = "yes"])

Fixed pkcs11-register defaults on macOS and Windows #3053

Fixed pkcs11-register defaults on macOS and Windows #3053

Uh oh!

Conversation

frankmorgner commented Feb 27, 2024

Checklist

Uh oh!

frankmorgner commented Mar 5, 2024

Uh oh!

Jakuje commented Mar 5, 2024

Uh oh!

frankmorgner commented Mar 6, 2024

Uh oh!

Jakuje commented Mar 7, 2024

Uh oh!

frankmorgner commented Mar 8, 2024

Uh oh!

Uh oh!

frankmorgner commented Mar 12, 2024

Uh oh!

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

frankmorgner Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

Jakuje Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

frankmorgner Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Jakuje Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankmorgner commented Mar 13, 2024

Uh oh!

frankmorgner commented Mar 13, 2024

Uh oh!

frankmorgner commented Sep 10, 2024

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

xhanulik commented Sep 11, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants