Fix to/from array conversions for windows - #103
Closed
droppingin wants to merge 2 commits into
Closed
Conversation
droppingin
force-pushed
the
feature/fix_byte_conversions_win64
branch
from
May 21, 2023 19:47
faeffac to
27c03bd
Compare
uint is at least 32 bit integer, according to the standard. on windows, the uint has 64 bits, as in linux, but: C.sizeof_ulong is 0x4 on windows. ASAN does not like that. but even if the value is stored, the code can't read it properly, because bytesToUlong checks if sliceSize > C.sizeof_ulong
droppingin
force-pushed
the
feature/fix_byte_conversions_win64
branch
from
May 21, 2023 20:09
27c03bd to
b748786
Compare
|
Any chance to get this merged? |
Nicolas-Peiffer
added a commit
that referenced
this pull request
Jul 31, 2026
makeKeyPair, makePrivateKey and makeRSAKeyPair return an error for any CKA_KEY_TYPE this package cannot represent, but the loops that call them only skipped errNoCkaID and errNoPublicHalf. Every other error aborted the whole walk, so a single object of an unknown type made FindAllKeyPairs, FindAllKeys, FindPrivateKeysWithAttributes, FindKeyRSAPairsWithAttributes and FindAllPairedCertificates fail outright with "unsupported key type: %X", hiding every other key on the token. v2 turns that six-year-old report into a regression we ship ourselves: an ML-KEM private key is a CKO_PRIVATE_KEY whose type none of those finders understand, so as soon as a user generates an ML-KEM key pair, enumerating the rest of that token stops working. Wrap the "unsupported key type" errors in a new errUnsupportedKeyType sentinel and skip it in the enumeration loops, next to errNoCkaID and errNoPublicHalf. FindKeysWithAttributes does the same for symmetric keys with no matching Cipher, and makeRSAKeyPair's "not an RSA key pair" wraps the sentinel too, so an RSA-only finder walking a mixed token no longer trips over the first EC key it meets. Singular lookups (FindKeyPair, FindKey, FindPrivateKey) now report "not found or is empty" instead of "unsupported key type" for such keys, which matches how they already behave when the public half is missing. Adds a regression test that puts an ML-KEM key pair and an RSA key pair on one token and checks all four enumerators still succeed. Reported by @Knacktus in #68 back in 2020, and independently run into by @droppingin, who cross-referenced it from #103 while fixing the Windows CK_ULONG conversions. Thank you both — the report was accurate the whole time, it just took us until the ML-KEM work to feel the pain ourselves. Fixes #68 Refs #103 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Hi @droppingin, @TheNitek and @Knacktus 😃 , TY 🙏 for your interest and this PR and issue #68 . And sorry for the very late reply. In 2026 the big project for crypto11 is to add the support of PKCS#11 v3.2 and the support of the ML-KEM PQC algorithm:
The other big project for 2026 is giving + moving the crypto11 project under the Eclipse Foundation. This is the opportunity for us to improve code quality and fix bugs for the future crypto11@v2. With the help of LLM code assistant, I started fixing on f2bc2f6 . And this fix will also impact an other project github.com/eclipse-keypont/pkcs11-go which is the C / Go binding for PKCS11. Stay tuned to see how this resolves. @droppingin and @Knacktus: I mention you in the commit f2bc2f6. We are considering adding a |
Nicolas-Peiffer
added a commit
that referenced
this pull request
Aug 3, 2026
Requires pkcs11-go v1.1.0-rc1, which exports the CK_ULONG codec this package had been carrying a broken copy of. CK_ULONG is a C unsigned long: 8 bytes under LP64 (Linux, macOS), 4 under Windows' LLP64 model and on 32-bit targets. Go's uint is 8 bytes everywhere. bytesToUlong reinterpreted the address of its buffer as a Go uint, which always reads 8 bytes, so on Windows it read 4 bytes past a 4-byte CK_ULONG buffer and returned whatever sat behind it in the top half of the value. Every CK_ULONG attribute was affected — CKA_KEY_TYPE, CKA_MODULUS_BITS, CKA_VALUE_LEN — which is how this first surfaced, as FindAllKeyPairs failing under SoftHSM2 on Windows. The v2 security audit fixed the neighbouring out-of-bounds read on short attributes (a 4-byte CKA_PARAMETER_SET) by copying into a sizeof(CK_ULONG) buffer first, but kept the same width assumption when reading that buffer back, so on Windows the read only moved from overrunning the caller's slice to overrunning our own buffer. Rather than fix the copy here, delete it. Three changes, all of them moving C-ABI knowledge down into the one package that legitimately holds the PKCS#11 headers: 1. ulongToBytes / bytesToUlong are gone; call sites use pkcs11.ULongToBytes / pkcs11.BytesToULong, which size the conversion from the C type and carry the tests. common.go held the only import "C" in crypto11, so the package no longer contains any cgo of its own (it still builds with cgo, through the binding). 2. signPSS builds its parameter with NewMechanismWithParams and NewPSSParams instead of concatenating three CK_ULONGs by hand, so the binding marshals the real CK_RSA_PKCS_PSS_PARAMS struct. This is what the "TODO this is pretty horrible, maybe the PKCS#11 wrapper could be improved to help us out here" comment was waiting for; OAEP and GCM parameters were already built this way. concat had no other caller and is removed with it. 3. FindRSAPrivateKeysWithAttributes gets the same skip the other finders got in f2bc2f6 — it walks every CKO_PRIVATE_KEY on the token, so one EC or ML-KEM key used to abort the whole enumeration. makeRSAPrivateKey's error for a non-RSA key also wrapped a nil err, rendering as "not an RSA key type: %!w(<nil>)" and matching no sentinel at all; it now reports the key type it found and wraps errUnsupportedKeyType. Reported by @droppingin in #103, with the analysis, the C data-model references and a SoftHSM2-for-Windows reproduction that made this easy to confirm. That PR sized the encoding by the magnitude of the value rather than by the platform's CK_ULONG, which would have shortened CK_MAC_GENERAL_PARAMS and CK_RSA_PKCS_PSS_PARAMS on LP64; the binding sizes them from the C type instead. Thanks also to @Knacktus, whose #68 covers the enumeration failure this surfaced through. Refs #68, #103 — #103 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nicolas-Peiffer
added a commit
to eclipse-keypont/pkcs11-go
that referenced
this pull request
Aug 3, 2026
Cryptoki hands attribute values back as raw bytes whose width is the token's, not Go's: CK_ULONG is a C unsigned long, 8 bytes under LP64 and 4 under Windows' LLP64 model, while a Go uint is 64 bits everywhere. This package already encodes that correctly and privately, in ckULong, but exports no way to read one back, so every caller above it has to re-derive the C ABI — which means importing "C" just to learn sizeof(CK_ULONG), or guessing. Guessing is what crypto11 did: it reinterpreted the address of an attribute value as a *uint, which always reads 8 bytes, so on Windows every CK_ULONG attribute came back with garbage in its top half and FindAllKeyPairs failed outright (eclipse-keypont/crypto11#103). Our own tests hint at the same gap — they carry a hand-rolled leUint helper for exactly this. Export three names: - ULongSize, the platform's CK_ULONG width. - ULongToBytes, ckULong under its public name, for raw mechanism parameters such as CK_MAC_GENERAL_PARAMS. NewAttribute already applies it to int/uint, so templates do not need it. - BytesToULong, the missing decode half. It zero-extends a value shorter than a CK_ULONG (SoftHSM returns a 4-byte CKA_PARAMETER_SET) and ignores anything past the first one, so a quirky token cannot make a caller read off the end of the value. The binding is the only package here that legitimately holds the PKCS#11 headers, so this is where the width belongs; consumers can now drop cgo entirely. Refs eclipse-keypont/crypto11#103 eclipse-keypont/crypto11#103 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
I will close this PR for now as I propagated the fixes to eclipse-keypont/pkcs11-go/3f6118a and eclipse-keypont/crypto11/02ec226. Thank you @droppingin for you valuable contribution that has been referenced in both commits 02ec226 and 3f6118a. The fix of #103 will be shipped with next minor version of You can already have a look at eclipse-keypont/pkcs11-go@v1.1.0-rc1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for the wonderful library. I have only one issue that i want to address here. The library does not work under windows when trying to call any function that (de)serializes data to/from bytes. The issue was detected during testing with SoftHSM.
Affected functions:
common.ulongToBytes()common.bytesToUlong()Affected platform: windows10 but i guess, any recent windows will have the same issue.
General
ulongis at least 32 bit integer, according to the standard.The go-
uinthas 64 bits, on windows and linux, but C.sizeof_ulong is 0x4 on windows (MSVC), and 0x8 on linux (GCC, Clang).But even if the value is in HSM, the code can't read it properly, because
bytesToUlongchecksif sliceSize > C.sizeof_ulong.The key of the problem is that widely used
uintcan't be trivially mirrored to the C code because it's length is in C (generally) undefined.Fix
The proposed change uses existing go standard libraries to achieve the same behavior without touching the implementation too much.
I also took a look at the pkcs11 docu to make sure the endianness is correct.
As the code of crypto11 only uses little-endian to convert data, the change is safe.
Note
the both changed common-functions should not be applied to e.g.
CKA_OTP_COUNTERthat requires big-endian (see pkcs11 docu above).Similar issues
Got something similar like FindAllKeyPairs breaks on unsupported key type. BTW @Knacktus what do you think?
Steps to reproduce
Initially i faced the issue on work when trying to read some values. Got something from
FindAllKeyPairswhich came initially frombytesToUlong. I temporarily fixed it on my local machine.Then i prepared a setup at home:
SOFTHSM2_CONFenv variable is exported and points to some valid SoftHSM2 config file, e.g.D:\WHATEVER_PATH_TO_SoftHSM2\etc\softhsm2.conf- there is one that comes (i believe) with the SoftHSM2 installation.softhsm2-util.exe --init-token --label crypto11_test --slot 0, to initialize the slot, enter some test pin{ "Path" : "D:\\WHATEVER_PATH_TO_SoftHSM2\\lib\\softhsm2-x64.dll", "TokenLabel": "crypto11_test", "Pin" : "YOUR_PIN_HERE" }Checkout db3e080, run
go test- bothTestUlongToBytesandTestBytesToUlongfail.Initially, i played around with the
TestULongMaskingbut then decided to split it into 2 parts, each testing its own function.Hope it is ok like that.
Checkout my last commit, repeat
go test- ok.Possible improvements
I added docu-comment to the functions to point their actual little-endian nature.
If you want, i can template it somehow to make endianness more explicit / selectable.
I think it is also reasonable to rename these functions, at least
ulong -> uint.References
wiki about data models - see table
a research about standard data types - see results table
Thank you for your attention and i hope you will find time to review the proposed change!