Mastercard / pkcs11-tools

A set of tools to manage objects on PKCS#11 cryptographic tokens. Compatible with many PKCS#11 library, including major HSM brands, NSS and softoken.
Other
142 stars 29 forks source link

Commandline option -t can't find token #7

Closed JSchy65 closed 4 years ago

JSchy65 commented 4 years ago

If I usel e.g. one of the tools p11ls, p11rm or p11mv with the -t option followed by the name of an existing token, I always get the message "*** Error: token with label 'xxx' not found".

This maybe also true for other commands as well but I haven't check that.

Cause of problem:

Source: lib/pkcs11_session.c function: rtrim()

The above mentioned source file contains the following line (around line 37): n = strlen((const char )str)>limit ? limit : strlen((const char )str);

That line is not working correctly. The > comparision fails.

Because strlen() calculates a length of 98 for the string and because this value is bigger than the limit of 32, after that comparison, n should have the value 32.

But it has not. n has still the value 98 and therefore the following comparison with the entered token name fails.

This is not an error in that line of code itself ! In my opinion, the root cause of the problem is an error in the gcc compiler if one of the optimization options -O1 or -O2 or -O3 is used.

One idea to solve the problem is to not use the -O2 flag for compilation of that source file.

The second ideas is to switch of the -O2 optimization just for the rtrim() function. This can be done to extend the pototype of that function to:

static CK_UTF8CHAR_PTR rtrim(CK_UTF8CHAR_PTR str, int limit) attribute((optimize("-O0")));

If I do this in my environment, the comparison now works correctly and therefore the commands now find my token by name successfully.

Additional info: gcc (GCC) 8.2.1 20180905 (Red Hat 8.2.1-3) running on an AWS ec2-micro instance.

pkcs11-tools Latest commit db918aa on 22 Feb.

p11mv belongs to pkcs11-tools v1.0.2 (Aug 26 2019) arch/CPU/OS: x86_64/x86_64/linux-gnu using openssl library: OpenSSL 1.0.2t-dev xx XXX xxxx

Hope that helps :-)

By the way, these pkcs11-tools are extremely helpful. Thank you very much for that.

keldonin commented 4 years ago

Hi @JSchy65 , thank you for using the pkcs11 tools, and for reporting an issue along with comprehensive information. I've never been hit by optimization issues so far, so that's an interesting one.

I would need to reproduce the issue. Can you give me more information about the PKCS#11 library you are using? Also, (out of curiosity), why is your token label longer than the limit imposed by CK_TOKEN_INFO?

For the resolution, I would like to find a way that doesn't add compiler-specific syntax, as this code currently compiles on other compilers than LLVM or GCC, and might not like the attribute() directive. Could the issue be caused by the ternary operator ? Have you tried changing rtrim() to use a simple if() {} else {} directive instead?

keldonin commented 4 years ago

Hi @JSchy65 , the way how the comparison was made had several shortcomings, one of them being that it modified the CK_TOKEN_INFO structure, possibly making it unusable for further use.

I have rewritten that section completely:

I tested with several token label sizes, including labels filling entirely the label array, and these seemed to work.

Before I merge this back to the master branch, I would appreciate if you could try it out and confirm it fixes your specific issue. Just checkout branch fix_issue_7, commit 34aa8ae3f226269309b26dda160daa8821f042a6.

Thanks!

JSchy65 commented 4 years ago

Hi Eric, I have tested your fix now and yes I confirm that it fixes the issue I have reported above. I tested with labels of various sizes (1, 10, 31, 32, 33) and also with the gcc compiler with various optimisation leves (-O0, -O1, -O2, -O3). In each case the fix behaved as expected -> Ok from my side :-)

I like the way very much that you have replaced that original implementation of the rtrim() with the new tokenlabelcmp(). In my opinion that makes the source easier to understand and is a more clear approach.

Thank you very much.

keldonin commented 4 years ago

Issue solved, will be published in v1.0.3 release.