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

P11-kit ignores packing and CK_LONG definition on non-windows systems. #488

Open
RickyDoug opened this issue Apr 10, 2023 · 7 comments
Open
Labels

Comments

@RickyDoug
Copy link

The P11-kit does not implement 1-byte packing per the PKCS11 specification, which clearly states this. There are bug reports relating to this as well where we see a mismatch between lengths. At the very least, the packing should be configurable as should be the type of CK_ULONG, CK_LONG, which are assumed to be long int, instead of using stdint and specifying the size, this decision is left up to the compiler do decide how bit CK_ULONG is. Since a long can be 32, 64 or something larger, this is an issue with the implementation used.
It is unfortunate that the same header file that is incorrect is being used across a wide variety of projects, and mis-implementing the PKCS11 API.

@ueno
Copy link
Member

ueno commented Apr 10, 2023

Could you be more specific about this, at least link to the bug reports you mentioned? The statement is about how the PKCS#11 specification is designed, so the structures defined there are by nature aligned without any padding, not a guidance for implementation. The pkcs11.h file distributed by p11-kit (common/pkcs11.h) does not include <stdint.h> but defines CK_ULONG to be unsigned long, which is a machine word.

@ueno ueno added the question label Apr 10, 2023
@RickyDoug
Copy link
Author

I haven't delved into very many of these reports, but this one is likely related:
#376
Other projects have reports of segfaults on on calling C_Initialize, which is packing related.

But the issues is twofold. Unsigned long can be either 32 bits or 64 bits, and not specifying a size is at the least bad practice.
The second issue is not packing the structures results in an API that does not comply with PKCS11, according to OASIS:

"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 writers do not appear to know about anything 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.

I think the bigger issue is that the pkcs11.h file in this project is used a lot of places, and this is a problem because now we have a fully compatible implementation on Windows but not on anything else.
I can supply a patch that at least makes this a configurable option so maybe someone, somewhere will finally start building compatible APIs.

@ueno
Copy link
Member

ueno commented Apr 10, 2023

I haven't delved into very many of these reports, but this one is likely related:
#376
Other projects have reports of segfaults on on calling C_Initialize, which is packing related.

It's about the p11-kit RPC protocol, which has nothing to do with pkcs11.h you are talking here.

@ueno
Copy link
Member

ueno commented Apr 10, 2023

Linking to softhsm/SoftHSMv2#701 to avoid the same discussion spreading across multiple issues. I generally agree with @dengert that this should be first discussed with OASIS. The reference header files have this problem already; for example, CK_INFO is 88 bytes with 8-byte alignment, while it is 76 bytes with 1-byte alignment, not because of the CK_ULONG definition but because the CK_VERSION fields (2 bytes) are padded.

That said, I'm not sure if it can be fixed by simply updating the header file, as many PKCS#11 modules (either open source or proprietary) are built upon those definitions (i.e., the reference header). If some of the applications switch to assuming the packed format, all access to those modules will break (and even if it works, that would be slower).

@dengert
Copy link

dengert commented Apr 11, 2023

For cross platform, some network byte order, size of CK_LONG and CK_ULONG and padding, would need to be defined. Besides p11-kit, are there any other applications that send PKCS11 structures over the network? ASN1 could also be used to transmit PKCS11 between platforms.

@RickyDoug
Copy link
Author

Those are all valid points, but adding the ability to configure these things to known items would at least allow package managers to choose their poison, which is what I provided for the softhsmv2....It defaults to the long int/unpacked, but it adds the ability to pack and to choose the size of CK_ULONG, CK_LONG. This also exposed some potential issues in the SoftHSMv2 that just assumed CK_ULONG is a unsigned long int. If we can at least propagate configurability, it's at least a step in the right direction.

@RickyDoug
Copy link
Author

RickyDoug commented Apr 11, 2023

@dengert byte order is a whole other can of worms. You are, of course right, a full cross platform would include this, but on the same system it's not necessary. Ideally, the PKCS11 API would have a call that allowed one to discover all of this information rather than just taking a plunge and segfaulting when the caller is wrong. And yes, there are other applications that send things over more than just networks, ASN1 is one of those, but conceivably anything that serializes/deserializes could go across any transport. Once you hit byte order, it becomes a really messy endeavor that has no hope of being performant.

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

No branches or pull requests

3 participants