Error Returns from C_Wrap

[johughes99]

I have just been adding some more tests in my test harness as regards C_Wrap and C_Unwrap.

I have noticed that when you attempt to wrap a key that is not permitted (ie cant be wrapped) in Softhsm you get an error return of CKR_KEY_UNEXTRACTABLE - instead of what I would expect of CKR_KEY_NOT_WRAPPABLE.

I have seen this in the following situations:

• Wrapping a EC public key with CKM_RSA_PKCS
• Wrapping a RSA public key with CKM_RSA_PKCS
• Wrapping a EC public key with CKM_AES_CBC
• Wrapping a RSA public key with CKM_AES_CBC

However, in the cases of:

• Wrapping a EC private key with CKM_AES_CBC
• Wrapping a RSA private key with CKM_AES_CBC

I get a CKR_GENERAL_ERROR rather than CKR_KEY_NOT_WRAPPABLE.

Although perhaps a bit late - would it be possible for #600 to look at this?

[keldonin]

@johughes99 , are you contemplating to integrate a fix to that #600 PR? Shouldn't it be a different one instead, for clear separation of concern?

[johughes99]

Whatever is best. However I will not be doing a PR - I only test - not fix :-) I found this because I was testing another PKCS#11 library and as usual I compared those results against what SoftHSM does - and this is where I came across this issue

[keldonin]

In your description, do you mean that you are returned a CKR_KEY_UNEXTRACTABLE instead of CKR_KEY_NOT_WRAPPABLE, even if the key to wrap has CKA_EXTRACTABLE set to CK_TRUE?

Also, the second part of your description seems to refer a different issue (CKR_GENERAL_ERROR) - maybe subject to a separate issue application?

PR #600 is about implementing CKM_XXX_CBC_PAD (AES and DES); the patch does not alter the current behaviour of CKM_AES_CBC; IMO this should be treated with a separate PR - but maybe once #600 has been merged, as it concerns the same sections of code.

@halderen I'm offering to look at this; assuming that #600 is still candidate for merging, I will wait until it is merged to work on a patch.

[johughes99]

Notice that I said public keys above concerning CKR_KEY_UNEXTRACTABLE - and you can't set CKA_EXTRACTABLE on a public key. Which is why the error return makes no sense (I think).

The public key template I set up is as follows:

publicTemplate.addAttribute(CKA_CLASS, (unsigned long) CKO_PUBLIC_KEY)
                            .addAttribute(CKA_KEY_TYPE, (unsigned long)CKK_EC)
                            .addAttribute(CKA_EC_PARAMS, ecParams )
                            .addAttribute(CKA_VERIFY, true )
                            .addAttribute(CKA_DERIVE, true )
                            .addAttribute(CKA_TOKEN, true )
                            .addAttribute(CKA_PRIVATE, true )
                            .addAttribute(CKA_WRAP, true )
                            .addAttribute(CKA_ID,id_public );

the private key issue may be separate issue - but the errors still pertain to calls to C_WrapKey - and these this only occur in the CKM_AES_CBC mechanism.

I'm also seeing some errors in the CKM_AES_CBC_PAD mechanism - but I will leave that for another time

[johughes99]

BTW - unless I'm misreading the code when I look at SymmetricAlgorithmTests::testAesWrapUnwrap() in SymmetricAlgorithmTests.cpp there appears to be no tests for the CKM_AES_CBC or CKM_AES_CBC_PAD mechanisms when used with c_WrapKey

[johughes99]

Ignore my comment re CKM_AES_CBC_PAD - SoftHSMv2 doesn't support wrapping with this mechanism

[johughes99]

re the CKR_GENERAL_ERROR in trying to wrap none secret key objects Just looked at SoftHSM.cpp on line 6478

change from:

if ((pMechanism->mechanism == CKM_RSA_PKCS || pMechanism->mechanism == CKM_RSA_PKCS_OAEP ) && keyClass != CKO_SECRET_KEY)
    return CKR_KEY_NOT_WRAPPABLE;

to

if ((pMechanism->mechanism == CKM_RSA_PKCS || pMechanism->mechanism == CKM_RSA_PKCS_OAEP || pMechanism->mechanism == CKM_AES_CBC) && keyClass != CKO_SECRET_KEY)
    return CKR_KEY_NOT_WRAPPABLE;

Current code is missing a sanity check. Will need to change the comment line above as well.

I changed my local copy - and it works fine now

If someone can do a PR and submit?

[keldonin]

Ignore my comment re CKM_AES_CBC_PAD - SoftHSMv2 doesn't support wrapping with this mechanism

That's precisely what #600 provides.

[johughes99]

as regards the CKR_KEY_UNEXTRACTABLE issue. I have debugged it - and can see what is going on. Its not really an error - per se. Its just a very confusing error.

The problem is that the code first checks whether the key object is extractable - - as in

// Check if the to be wrapped key can be wrapped
if (key->getBooleanValue(CKA_EXTRACTABLE, false) == false) {
    return CKR_KEY_UNEXTRACTABLE;
}

but this occurs before the check whether that type of key can be wrapped (as per the code I have provided an update for).

Can I suggest that these checks are done the other way around. That way a more sensible error is returned to the application

[keldonin]

With respect to the issues you describe:

• Wrapping a EC public key with CKM_RSA_PKCS
• Wrapping a RSA public key with CKM_RSA_PKCS
• Wrapping a EC public key with CKM_AES_CBC
• Wrapping a RSA public key with CKM_AES_CBC

It results into the class of error of the kind "wrapping a public key with whatever mechanism". there is no point in doing that, as C_WrapKey() is meant to wrap a secret or a private key.

CKR_KEY_NOT_WRAPPABLE according to the specification, applies only to secret or private keys, and is not applicable in this case (https://docs.oasis-open.org/pkcs11/pkcs11-base/v3.0/os/pkcs11-base-v3.0-os.html , line 1949):

• _CKR_KEY_NOT_WRAPPABLE: Although the specified private or secret key does not have its CKA_EXTRACTABLE attribute set to CK_FALSE, Cryptoki (or the token) is unable to wrap the key as requested (possibly the token can only wrap a given key with certain types of keys, and the wrapping key specified is not one of these types). Compare with CKR_KEYUNEXTRACTABLE.

IMO, the library should instead return CKR_KEY_FUNCTION_NOT_PERMITTED, or CKR_ARGUMENTS_BAD, but that is the call of SoftHSM designers, I would say. @rijswijk & @halderen to chime in.

[johughes99]

I understand what the base spec says - but I could argue that the spec is wrong and in fact this error return should be allowed for wrapping public keys. I have seen at least one implementation return this value

But if we want struct compiance with the spec - and I checked with version 3.0 - then only CK_ARGUMENTS_BAD is the only sensible error return - as CKR_KEY_FUNCTION_NOT_PERMITTED is not a permitted return value.

As you say its up to its up to @rijswijk & @halderen - but in any case the source needs to be changed to cope with the two situations I have identified.

[rijswijk]

I agree with @keldonin on CKR_KEY_NOT_WRAPPABLE, and I agree with @johughes99 on CKR_KEY_FUNCTION_NOT_PERMITTED, so I think the correct behaviour in this case should be to return CKR_ARGUMENTS_BAD. This may require changes at more points in the code, so I suggest decoupling that from other changes in the code, but making sure new code (i.e. #600) is in line with this behaviour.