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

Fix to/from array conversions for windows #103

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

droppingin
Copy link

@droppingin droppingin commented May 20, 2023

First of all, thank you for the wonderful library. Now to the case:

General

ulong is at least 32 bit integer, according to the standard.

The go-uint has 64 bits, on windows and linux, but C.sizeof_ulong is 0x4 on windows, and 0x8 on linux.

But even if the value is in HSM, the code can't read it properly, because bytesToUlong checks
if sliceSize > C.sizeof_ulong.

The key of the problem is that widely used uint can't be trivially mirrored to the C code because it's length is in C (generally) undefined.

Affected functions:

  • common.ulongToBytes()
  • common.bytesToUlong()

Affected platform: windows10 but i guess, any recent windows will have the same issue.

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_COUNTER that 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 FindAllKeyPairs which came initially from bytesToUlong. I temporarily fixed it on my local machine.

Then i prepared a setup at home:

  • Install SoftHSM2 e.g. from here.
  • Make sure that SOFTHSM2_CONF env 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
  • Adapt config file in the root of the repo, use pin entered above:
{
  "Path" : "D:\\WHATEVER_PATH_TO_SoftHSM2\\lib\\softhsm2-x64.dll",
  "TokenLabel": "crypto11_test",
  "Pin" : "YOUR_PIN_HERE"
}

Checkout db3e080, run go test - both TestUlongToBytes and TestBytesToUlong fail.
Initially, i played around with the TestULongMasking but 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!

@droppingin droppingin force-pushed the feature/fix_byte_conversions_win64 branch from faeffac to 27c03bd Compare May 21, 2023 19:47
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 droppingin force-pushed the feature/fix_byte_conversions_win64 branch from 27c03bd to b748786 Compare May 21, 2023 20:09
@TheNitek
Copy link

Any chance to get this merged?

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

Successfully merging this pull request may close these issues.

2 participants