SoftHSMv2 C_Initialize fails with segfault

[RickyDoug]

Per the Oasis 2.40 docs: _2.1 Structure packing Cryptoki structures are packed to occupy as little space as is possible. Cryptoki structures SHALL be packed with 1-byte alignment.

However in the develop branch of SoftHSMv2 there is no packing for gcc, which means the PKCS11 API has no hope of running with compliant host software. Granted, MANY implementations of PKCS11 ignore this requirement, but it's not a good excuse to ignore the specification. At the very least it should be a configurable item. I will be glad to provide a patch if anyone is interested.

[RickyDoug]

https://github.com/opendnssec/SoftHSMv2/pull/702 Has the patch with default of CK_ULONG to unsigned long and with packing enabled. Your CI is broken, and I've added an issue for that.

[dengert]

As you have said: "Granted, MANY implementations of PKCS11 ignore this requirement, but it's not a good excuse to ignore the specification."

The SoftHSM version of pkcs11.h was updated by https://github.com/opendnssec/SoftHSMv2/commit/ecba266562ca9b6beaf9777690e04810539a550d

Interoperability is more of major concern. packages such as SoftHSM, OpenSSH, p11-kit, OpenSC and OpenPGP all use a version of pkcs11.h which only do packing on windows. These pkcs11.h have at least these copyrights:

     Copyright 2006, 2007 g10 Code GmbH
     Copyright 2006 Andreas Jellinghaus

Some also have additions for PKCS11 V3.0. These files are used on at least RedHat and Ubuntu and work with gcc.

Based on the title of this issue: "SoftHSMv2 C_Initialize fails with segfault" it sounds like you may have uses a pkcs11.txt which used packing. To interoperate with other pkcs11 implemations I would suggest that you use the pkcs11.h which comes with SoftHSM or one of the other packages in your application.

[RickyDoug]

The other issue is all of the rest of the code that although it interfaces with PKCS11, it only uses a partial implementation, notably several permutations of unsigned long, unsigned long int, etc. It is unfortunate there was no attempt to make it compatible (i.e. CK_ULONG) which make this task all the much harder. I'm just amazed that so many developers rely so heavily on gcc, and not only that, on gcc acting the same across versions, platforms and architectures. As long as it's all intel 64 bit, it works, but step outside that and something breaks. I suppose I should file reports against the other offenders as well. Why have a spec, then completely ignore the parts one doesn't like? It's kinda not a spec anymore. I do find it humorous that of all the platforms it was properly implemented on, it was Windows, an OS that has probably the worst record ever on security. When I am done, do you want a patch?

[dengert]

I am not a member of the SoftHSM project, but am interested in interoperability of PKCS11 applications and libraries on a platform and architecture. So changing the packing used by SoftHSM and your application might cause interoperability problems with other packages trying to use different alignment.

All of the PKCS11 packages listed above also run on MacOS.

What platform are you using? What compiler?

Latest PKCS11 standard: https://docs.oasis-open.org/pkcs11/pkcs11-base/v3.0/pkcs11-base-v3.0.pdf "Chairs: Tony Cox (tony.cox@cryptsoft.com), Cryptsoft Pty Ltd Robert Relyea (rrelyea@redhat.com), Red Hat"

Says where to find header files: OASIS pkcs11.h https://github.com/oasis-tcs/pkcs11/blob/pkcs11-3.00/published/3-00/pkcs11.h which clearly addresses Windows.

Als says: "This specification replaces or supersedes: • PKCS #11 Cryptographic Token Interface Base Specification Version 2.40. Edited by Robert Griffin and Tim Hudson."

https://github.com/oasis-tcs/pkcs11/blob/pkcs11-3.00/published/3-00/pkcs11.h says: "In a UNIX environment, you're on your own for this. You might not need to do (or be able to do!) anything."

The above comments, IMHO, on Unix systems, applications and packages using PKCS11 must be consistent on the platform.

FYI - the problems with alignment: https://en.wikipedia.org/wiki/Data_structure_alignment

[RickyDoug]

Changing packing will almost guarantee a segfault of ANY PKCS11 application/library. Forget interoperability. Changing the packing to the PKCS11 spec is the only way to mostly guarantee interoperability with systems of the same endian. The downside is that packing is less efficient, but one might argue the entire PKCS11 is not terribly efficient.

@dengert - I'm very familiar with all your links. It doesn't change the fact that the latest 3.0 version still says "2.1 Structure packing Cryptoki structures are packed to occupy as little space as is possible. Cryptoki structures SHALL be packed with 1-byte alignment", Following along the doc it says the definition of 'SHALL' is in https://www.ietf.org/rfc/rfc2119.txt, which says :

1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.

So what this really means is the header file supplied by g10 Code/Andreas Jellinghaus and propagated by Red Hat is absolutely not compliant with the specification on anything other than Windows. The oasis developers are clearly not familiar with gcc or any other OS besides windows, so they apparently did not realize or care that GCC also packs structures to 1-byte alignment. It doesn't excuse the fact the it is very simple to add padding to any of these header files, or at the very least make it configurable. It is also sad the PKCS11 API has no way to query the interface to determine packing so one could at least gracefully die if needed.

Anyhow, IMO, the real issue is this entire code base assumes a 64 bit value for CK_ULONG, and if anyone changes this, the entire code blows up - nothing will compile. I'm guessing most of the builds are all on GCC, so they work...for now, and everyone hopes that GCC will behave the same way over machines, architectures, and versions. I have patch to add 32/64 bit sized CK_ULONG and add packing as part of the configuration. It also fixes the mirad API mis-matches where the CPP code uses long to handle CK_ULONG accesses prevalent throughout the code. I'm trying to run all this through a test suite before submitting a pull request. It looks like the tools work, but maybe threaded operations are broken?

[dengert]

I hear what you are saying. OASIS PKCS11 specs say "SHALL" but OASIS provided header file says on Uinx you are on your own. You should should be arguing with OASIS not with SoftHSM.

Did you catch one of the chairs for OASIS is from RedHat?

https://stackoverflow.com/questions/589575/what-does-the-c-standard-state-the-size-of-int-long-type-to-be tries to address how big is a LONG. in C and C++. They say ULONG must be at least 32 bits and the system header files have the sizes.

The "header file supplied by g10 Code/Andreas Jellinghaus" header files are using the sizes from the system header files by saying:

  28    There is an additional API available that does comply better to the
  29    GNU coding standard.  It can be switched on by defining
  30    CRYPTOKI_GNU before including this header file.  For this, the
  31    following changes are made to the specification:
  32 
  33    All structure types are changed to a "struct ck_foo" where CK_FOO
  34    is the type name in PKCS #11.
  35 
  36    All non-structure types are changed to ck_foo_t where CK_FOO is the
  37    lowercase version of the type name in PKCS #11.  The basic types
  38    (CK_ULONG et al.) are removed without substitute.
  39 
  40    All members of structures are modified in the following way: Type
  41    indication prefixes are removed, and underscore characters are
  42    inserted before words.  Then the result is lowercased.

i.e. CK_ULONG is replaced with unsigned long. but for applications that also add:

1555 typedef unsigned long int CK_ULONG;
1556 typedef long int CK_LONG;

IMHO, using the system header files is the best way to go.

[RickyDoug]

I know one of the chairs is from Red Hat, but it that doesn't necessarily help. unsigned long is not a good way to define anything, it's nearly as bad as just using int. stdint.h is the correct way to handle it - then you know exactly what size you are using, either uint32_t or uint64_t on any platform with stdint.h. But I've submitted a patch that handles all of this, and at this point it's out of my hands. It would be nice to see that propagate so at least this tool is configurable and useful outside of a narrow range of compilers and probably only 64 bit intel. That patch is working with CK_ULONG as a uint32_t and packed structures with a third party test suite over a unique interface. I would not consider it completely tested, but no issues so far.