515663 - Improper setting of CKA_DERIVE attribute during PKCS #12 import

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Helge Bragstad]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB6; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)

During import of PKCS #12 file (using Firefox 3.5.3), NSS first attempts to unwrap the encrypted key using C_UnwrapKey. If this fails, for instance if the token does not support this mechanism, NSS tries to create the private key using C_CreateObject. At this point, the template for the private key contains an improperly formatted CKA_DERIVE attribute, the pValue member of CK_ATTRIBUTE is NULL. This causes PKCS #11 library to reject the C_CreateObject call and PKCS #12 import fails. Instead, NSS should either set a desired CKA_DERIVE attribute or remove it from the template to let the tokens default value apply.

Reproducible: Always

Steps to Reproduce:
1. Load a PKCS #11 library that does not support C_UnwrapKey and which requires a well formed formatting of CK_BBOOL attributes in template, i.e. pValue != NULL and ulValueLen == 1.
2. Try to import a PKCS #12 file to this token.

Actual Results:  
Import fails.

Expected Results:  
Import should work

Comment 1

[Luz]

Attachment: Proposed patch

Hello,

We have investigate this bug, and we have found the problem, and a posible solution:

The function pk11_loadPrivKeyWithFlags (in pk11akey.c), builds the template for new key creation.

In this template there are several attributes that are specific for each key type (rsa, dh, ec, ...)

Some of this specific attributes are key components, like CKA_MODULUS, but others are key flags, like CKA_DERIVE.

After template construction, there is a loop that convert the key componentes from signed representation, to unsigned. This is performed in pk11_SignedToUnsigned function, by removing ALL null bytes at the beginning of the component.

The problem is that this function is called for all key specific attributes, that includes key components and key flags. It should be called only for key components.

The key flags are CK_BBOOL type, and are stored in one byte. If the flag value is FALSE, pk11_SignedToUnsigned transform the attribute to zero length value. This is incorrect acording PKCS11 standard. As a consecuence, PKCS11 modules must return an error in the C_CreateObject invocation.

This bug probably is the same reported in #458161, #549418 and this (#515663).

We have added an attachment with a patch proposal.
It works fine with our PKCS11 module.

Hope this help.

Comment 2

[Robert Relyea]

Comment on attachment 430258 [details] [diff] [review]
Proposed patch

r-

While I appriciate the patch, I think the cleaner and simpler way of fixing this is to have SignedToUnsigned return at least one byte.
RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v
retrieving revision 1.21
diff -r1.21 pk11obj.c
353c353
<     while (len && (*ptr == 0)) {
---
>     while ((len > 1) && (*ptr == 0)) {

Does this fix your issue?

bob

BTW thanks for the bug and the diagnostic.

Comment 3

[Luz]

Hello Bob.

Thanks for your quick answer.

The solution you propose solves the issue of zero length attributes, and our PKCS11 module works fine with it.

But this really solves another issue that is that any key component (usually big integers) should be at less one byte length.

As a side effect it also solves the zero length issue when this function is called for a CK_BBOOL attribute.

Our patch proposal avoids the pk11_SignedToUnsigned call for CK_BBOOL attributes, because we understand that this function is only for big integers (key components)

We also understand that there are several PKCS11 modules in the field, from different manufacturers, that probably have adapted the behavior to current nss calls. So probably you prefer the solution with less probability of side effects in current modules.

For us, any of the solutions (or both) will be welcomed.

Thanks for all.

Comment 4

[Rickard Bellgrim]

This bug is still present in:
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0

You cannot import the key into SoftHSM. The template for this test key can be revealed with pkcs11-spy.dll. You can see below that CKA_DERIVE is not formatted as a CK_BBOOL.

1726: C_CreateObject
[in] hSession = 0x3
[in] pTemplate[19]: 
    CKA_CLASS             CKO_PRIVATE_KEY      
    CKA_KEY_TYPE          CKK_RSA            
    CKA_ID                066a0020 / 20
    FE5F267A 3B10F6D0 9DAE3F04 28475024 5783CF00
    CKA_MODULUS           066a0038 / 128
    C24DB79C 1EE24CD0 BCFD105E 74E05F33 81921192 9EDECBBE 6F73A464 3451F8C0
    FE6EE6E2 C6C6CD05 83343E41 6C4FB009 0ACEE4D9 1CF6DE03 1EFA370D C1A1665E
    1EB346F6 E8C2F097 C27E7F78 91E5FC18 4F20A117 833DE2CE 72FB9DF4 D940AD06
    99E92DC4 E4E038B2 635FD2EC 186BC23D E59A4557 B96F4B5B 9A19E556 4AA8D1F7
    CKA_PRIVATE_EXPONENT  066a00b8 / 128
    71049390 13C6BDB7 8CB60617 6B14374B D64A083E A87A1F38 9DCB3E0B AB032315
    DEE8D313 855B8D55 6F83ABD2 9215A7E7 1A8A4D42 9C3E5BD4 A4E815AD C2BB06B7
    245D7E30 62966757 219AEBE7 FA41CEB7 9254C0F1 C58A2E90 5DB44C21 C34EAFD2
    E980BCEA 59F8FE75 95BC9178 3A46B62F B2102935 C3A7C071 880C2F28 07BAC1C1
    CKA_PUBLIC_EXPONENT   066a0138 / 3
    010001
    CKA_PRIME_1           066a0140 / 64
    ED6F3A63 9FFF1A63 CD1FB397 5BEF2CFD 55E47615 45391F0A CFE6C327 AECF07DE
    4E7A2181 3AC642A3 0B69A496 65706F7D 3291CA66 9CCA4E87 3B3478D2 60EA63B3
    CKA_PRIME_2           066a0180 / 64
    D17F212A 919FD3F6 8B5B5FED 342DDEE4 2ABD91D9 34DBDA34 4515C174 CAB3EBBD
    CD7A497D 7AE887FC 9E7CFB88 4873B7D1 FF451D26 DBA760B2 24FEF2DA 78BDC6AD
    CKA_EXPONENT_1        066a01c0 / 64
    E482E22D 33B52F4E D2022AB4 0794FF35 AA4EC09E E40A7FA7 C6438F9B 47909540
    0D4359AF A8435BAA D3B70EE2 782A6802 9482DFCB 362736C0 CBE84A48 1F623625
    CKA_EXPONENT_2        066a0200 / 64
    28CAC151 81A38669 563F4791 6F7C930F 08877B13 B92829F7 CD8FEBE8 6AE3D7BE
    50794440 0BA3A57E F9F94A1D 02468DFF 74DE274B FEAF3BB3 D4625435 581753CD
    CKA_COEFFICIENT       066a0240 / 64
    D120580D E5CD3409 A05C1187 12693080 A4CF9833 7DC08EA3 D6B61EC4 DC352CF0
    E6F7A38E 6CCC8AD4 39B1EEF4 78E22C41 4DEAF817 3C99259E A76BC754 0C733674
    CKA_DECRYPT           True
    CKA_DERIVE            066a0289 / 0
    CKA_SIGN              True
    CKA_SIGN_RECOVER      True
    CKA_UNWRAP            True
    CKA_TOKEN             True
    CKA_PRIVATE           True
    CKA_SENSITIVE         True
Returned:  19 CKR_ATTRIBUTE_VALUE_INVALID

Comment 5

[pavrw]

(In reply to Rickard Bellgrim from comment #4)
I can confirm this.
IMHO, its a firefox bad implementation of the PKCS#11 standard.

Using a <keygen> element on Firefox 6.0.1, the browser display a dialog for selecting the PKCS#11 token that will create the CKO_PRIVATE_KEY object.
This dialog is only displayed when a token is present.

When C_CreateObject is invoked with a template (collection of attributes) the attribute type CKA_DERIVE has a valid pointer, but a length of 0.

AFAIK this should never happend. The correct value should be sizeof(CK_BBOOL).

>     CKA_DERIVE            066a0289 / 0

Comment 6

[Robert Relyea]

Changing to assigned, since this is clearly confirmed.

Comment 7

[Robert Relyea]

Attachment: Don't bring an unsigned attribute under 1 byte

Codify patch suggested in my comment body and verified by the initial writer as working.

Comment 8

[Robert Relyea]

That should put it on my radar long enough to get a one line patch checked in.

Comment 9

[Robert Relyea]

Checking in pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.23; previous revision: 1.22
done

Comment 10

[Wan-Teh Chang]

Comment on attachment 563248 [details] [diff] [review]
Don't bring an unsigned attribute under 1 byte

r=wtc.

I agree with Luz's comment 1 and comment 3 (but not his patch).
This patch really fixes a different problem.  I examined all
the callers of pk11_SignedToUnsigned.  pk11_loadPrivKeyWithFlags
is the only caller that does not take care to pass only the
attributes that represent signed integers to pk11_SignedToUnsigned,
and this is a regression introduced in rev. 1.23 of pk11obj.c
(the fix for bug 465926).

So, I recommend that we also check in a better version of
Luz's patch, so that pk11_loadPrivKeyWithFlags also calls
pk11_SignedToUnsigned only on attributes that represents
signed integers.

Here is an example of how to do it properly.  It uses
the signattr variable to mark the beginning of signed
integer attributes.  Notice that CKA_VERIFY, CKA_DERIVE,
etc. are before signattr:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11akey.c&rev=1.33&mark=190-191,202-203#188

The it calls pk11_SignedToUnsigned starting with signattr:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11akey.c&rev=1.33&mark=234,236-238#233

Luz, would you have time to write a patch using this
approach?

Comment 11

[Wan-Teh Chang]

Comment on attachment 430258 [details] [diff] [review]
Proposed patch

Hmm... I looked at Luz's patch more carefully.  It's actually
using the approach I recommended.  I wonder if we can reorder
the attributes in the template to make it easier to verify
the correctness of the function.

Comment 12

[Robert Relyea]

Yeah, That's what Luz's patch did. I personally decided it presented to much extra bookkeeping, and the patch to pk11_SignedToUnsigned() is sufficient.

The one area where it wouldn't be is if we passed actual CK_ULONGs. The original code was depending on the fact the pk11_SignedtoUnsigned() would be invariant for CK_BOOL.

bob

Comment 13

[Vadzim]

Hi, I am developing PKCS#11 token and I have found this problem when I was testing my token with Mozilla 7.0.
What do you think when Mozilla and Thunderbird will be patched to solve this bug?

Vadzim