480280 - The CKA_EC_POINT PKCS#11 attribute is encoded in the wrong way: missing encapsulating octet string

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alex Dupre]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; it-IT; rv:1.8.1.17) Gecko/20080924 Firefox/2.0.0.17
Build Identifier: tested on 3.11.9, but it should be the same in all releases

The PKCS#11 specifications say that the CKA_EC_POINT should be a byte array representing the DER-encoding of ANSI X9.62 ECPoint value. This means it should be a der encoded octet string containing the public key bitstring (04 LEN 04 X Y), where the first 04 is the octet string tag, LEN is the the content length, the second 04 identifies the uncompressed form, and X and Y are the point coordinates. All real PKCS#11 token I tried (ncipher, utimaco, trustway) do the correct encoding, while the NSS softoken strips the octet string and returns only the content (04 X Y).

Reproducible: Always

Steps to Reproduce:
1. create an EC keypair
2. read the CKA_EC_POINT attribute

Actual Results:  
Only the content.

Expected Results:  
The content encapsulated in a der encoded octet string.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I do not confirm the assertion of this bug at this time.  
However, I am making the other members of the FIPS team aware of this.

Comment 2

[Robert Relyea]

It appears the assertion is correct. It appears this is the reason for a couple of existing hardware compatibility issues.

At least one other vendor holds the points unwrapped in their implementation. That is the certicom PKCS #11 module.

I see 3 separate issues that need to be looked at: 1) PK11 wrap *must* be able to support compliant PKCS #11 modules. 2) Softoken *should* have a compliant PKCS #11 interface. 3) The binary compatibility needs to continue to work.

Issue 1 with 3 can be resolved as follows:

Task 1) On export of any EC point from the token, NSS can detect if the point is wrapped or not, and unwrap it for it's own use if it is wrapped.

Task 2) On import of any EC point to a token, NSS can try importing one way, and revert to the second way on error. This is a bit tricky since the current softoken would accept the wrapped key, and only detect the problem when we go use the key. This means if to be able to use a non-compliant softoken with the existing NSS, we would need to try importing the unwrapped type first. If any of the other tokens also do not detect the bad key issue until we try to use it, then we won't have compatibility. I do not yet know if the certicom PKCS #11 module will accept a DER wrapped key (and use it correctly), if it will return an error, or if it will accept it, but not be able to parse it correctly (as the old softoken does). To decide which way to go here we need to answer the following questions:
         1) Do we need to be able to use old versions of softoken with a new version of NSS when doing ECC. (The answer to this question without the qualifier is an unqualified 'YES', I don't know the answer with the qualifier).
         2) Do we need to continue support the Certicom PKCS #11 module? Answer Yes
         2a) Can Certicom accept properly formatted CKA_EC_POINT values?
         2b) If it can't, does it return an error on C_CreateObject if one is supplied?
         3) Do any of the tokens that correctly format CKA_EC_POINT values accept unwrapped CKA_EC_POINT values without being able to actually use the values?

If the answers to 1 is YES or 2b is NO (which implies 2a is NO) then we must try the unwrapped point first. If the answer to 3 is YES then we must try the wrapped point first. Of course this means if 3==YES && (1 ==Yes || 2b==NO) then we are officially stuck.

Resolving Issue 2 with 3 is trickier. Here are the options:

Task 3: Accept wrapped CKA_EC_POINT on input. This will allow both old an new applications to use softoken. Old versions of NSS could use this softoken without change. Whichever way Task 2 falls, NSS will end up importing the public key on the first try. The CKA_ID must remain based on the unwrapped value of CKA_EC_POINT. This is complicated by the fact the we will store the CKA_EC_POINT value in the database. In the lg_database, this value isn't really stored, so there isn't a database compatibility issue, but in sql databases the actual full value is stored. Do we need to adjust this value on input to the database? Not doing so means that an application using an old version of NSS that tries to export an old stored ECC public key may extract an incorrect if the key was imported by a newer version of NSS.

Task 4: What to return when queried. This is the real problem. We don't know what the upper level software will accept. For binary compatibility, it appears we need to continue to present the old, wrong, value. If it's OK to break this compatibility (old versions of NSS will not work with a new softoken. Neither would any app that uses softoken directly, that is not already able to cope with the differences in CKA_EC_POINT encoding). Because this is a 'pull' case (the application is querying the token for a value), we don't have the option of trying one thing, and then another.


My recommendation is clearly to to Task 1 and 3. Answer the questions needed to figure out how to handle Task 2. 

Task 4 then becomes a question of which is more important: correctness or compatibility.

bob

Comment 3

[Alex Dupre]

I don't know all the scenarios, but how common is to have different versions of the softoken and NSS library? Since both are compiled from the same NSS tarball I think they are in sync most of the times, isn't it?
The hardware tokens I currently own doesn't allow to import externally generated keys, so the question 3 in task 2 isn't applicable (but if you give me a sample test code I could try if I find one).

About the certicom pkcs#11 module you are referring to, is it a soft or hardware token? They could be informed about the bug.

I see the issue as a PKCS#11 developer that like to use in a transparent way different hardware tokens and the nss softoken to internally generate and use ECC keypairs. In this scenario Task 4 is the most important, because it's the only one that generates incompatibilities. How many developers are currently using directly the softoken with ECC support, ignoring this issue, and how many will face it in the future trying to switch different cryptoki implementations? 
We are still in the early days of ECC, I think this binary incompatibility change in order to reach standard compliance is the way to follow.

Comment 4

[Robert Relyea]

Alex it is not uncommon for us to mix versions of NSS and softoken (usually older softoken with newer NSS), but those cases I know it happens does not use ECC. I'm looking to some of my other NSS developers and users, however to answer this question for themselves.

There is also an issue of applications using softoken directly. I, myself, do not like to encourage that, but despite my warnings, I get a fair number of bugs from applications doing just that. Before we make an incompatible change to softoken, we need to make sure we don't unnecessarily break those applications. Hopefully those applications are also designed to work with other pkcs #11 modules and can handle correct pkcs #11 implementations of ECC as well.

In short, I myself, and my company in particular, do not have any need to preserve binary compatibility on this interface, but I need to verify with the other NSS customers and developers if they too have no issue as well.

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Bob, 
I think there needs to be some discussion of this issue in the cryptoki 
mailing list.  All the participating vendors who make modules with ECC 
mechanisms need to know that this inconsistency exists among implementations
and perhaps we/they should collectively decide on a resolution.  

Also, could you cite the URL, chapter and verse of the spec that calls for
the use of the encapsulation in an octet string?

Finally, in comment 1, you used the words "wrap" and "unwrap" in numerous
places where I think you meant "encapsulate" and "unencapsulate", e.g. 

> On export of any EC point from the token, NSS can detect if the point
> is wrapped or not, and unwrap it for it's own use if it is wrapped.

> the current softoken would accept the wrapped key, and only detect the 
> problem when we go use the key. This means if to be able to use a 
> non-compliant softoken with the existing NSS, we would need to try 
> importing the unwrapped type first.

> Task 3: Accept wrapped CKA_EC_POINT on input.

I think in all those places, you mean "encapsulated" but you wrote "wrapped".
Please confirm.  

I think the only place where you wrote "wrapped" and meant "wrapped" (as in
encrypted), was when you wrote:

> I do not yet know if the certicom PKCS #11 module will accept a DER wrapped 
> key [...]

It's very important (especially when writing to cryptoki) that we use the 
precise terms throughout this, to avoid confusion.

Comment 6

[Robert Relyea]

> I think there needs to be some discussion of this issue in the cryptoki 
> mailing list.  All the participating vendors who make modules with ECC 
> mechanisms need to know that this inconsistency exists among implementations
> and perhaps we/they should collectively decide on a resolution.  

 I would agree if it were the case that half the vendors implemented one way, and half the other. My initial investigation turned up that Only a few misimplemented the feature (namely us, and maybe certicom). At this point it's clear, our code is wrong and it is really up to us to figure out how to conform to the spec.

> Also, could you cite the URL, chapter and verse of the spec that calls for
> the use of the encapsulation in an octet string?

I'm looking at page 217 of the PKCS #11 v2.20 spec (28 June 2004). Section 12.3.3, table 55 "Eliptic Curve Public Key Object Attributes".

CKA_EC_POINT   byte array   DER-encoding of ANSI X9.62 ECPoint value Q.

> I think in all those places, you mean "encapsulated" but you wrote "wrapped".
> Please confirm.

I did not mean 'wrapped with an encryption key' in this case encoded, encapsulated and wrap are synonyms (since no key is involved).

Anyway, this is really an NSS policy, how far can we break compatibility in correcting this. What I really need is your answers to the following question:


1) Is it ok to break the ability to use an old softoken in a new version of NSS for the case of ECC.
2) Is it ok to break the ability to use a new softoken in an old version of NSS for the case of ECC.
3) Is there any issue of a non-NSS app that uses softoken in the ECC case.

Most exactly, what will break is the following:

1) old NSS and non-NSS apps may not be able to extract or generate ECC public keys with a new softoken (This happens if we do Task 4). These are already broken for the majority of ECC tokens.
2) new NSS will not be able to use old softokens to verify ECC signatures or do ECC key exchanges (assuming Task 2 and correct ECC points first).


BTW I need answers soon. The current code complete schedule is March 31, and this item is both significant and not on that schedule.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

There is one Sun product that uses NSS Softoken without the rest of NSS.
It is Java's PKCS#11 Java Crypto Engine (JCE).  I don't know if it 
offers ECC, but if it does, then binary compatibility will likely be 
an issue to be resolved.  I can't find the right people to discuss this
at the moment.  

One of my concerns is that any changes to Softoken are probably going to 
require complementary changes to libSSL.  Otherwise, we run the risk of 
changing the bits we're sending (and/or receiving) over the wire.  
I sure wish we had run into this back while the ECC interoperability 
working group was active.  I think the mailing list still exists, but I 
don't know how many of the participants are still reachable.

Comment 8

[Alex Dupre]

I'm aware of the Java PKCS#11 provider, actually I'm using the tokens with java applications. The ECC code was donated by Sun to OpenSSL and NSS, so they share the same bugs. The other most famous Java PKCS#1 provider (IAIK) does the right thing.

For JDK a bug has been opened to fix this issue:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6763530

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

The PKCS#11 working group is discussing this topic, and will decide what 
is the right answer (soon, I hope).

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

After reading the (not yet complete) discussion of this in the PKCS#11 
working group mailing list, I propose that we "fix" this as follows:

1. Change pk11wrap so that it will accept a CKA_EC_POINT value that is
either way, OCTET STRING, or not, and do the right thing with it either way.

2. Change Softoken so that in the places where it inputs one of these 
attributes, it will accept either form (OCTET STRING or not) and do the 
right thing.

3. That leaves only the question of what form should softoken output when
C_GetAttributeValue is called.  I propose that we change it to output an
OCTET STRING by default, but allow an environment variable such as 
"NSS_EC_POINT_311_COMPAT" to override that, causing C_GetAttributeValue to 
output the same thing it does until now.

Comments?

Comment 11

[Alex Dupre]

Is the PKCS#11 working group mailing list publicly available?

Comments on points:

2) I assume an encapsulated EC point can always be distinguished by a not compressed and not encapsulated EC point. Just out of curiosity, which logic will be implemented? (it's quite unlucky that both representations begin with 0x04).

3) Fine for me.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

See http://www.rsa.com/rsalabs/node.asp?id=2143 for info on how to access
the cryptoki email archives.  I've never used them, so I don't know what 
to expect.

Comment 13

[Wan-Teh Chang]

Nelson, your proposal in comment 10 is fine.  I can't come up
with a solution that doesn't break backward compatibility.  I
assume it is inappropriate for us to change PKCS #11 to match
our implementation.

Our standard method of bumping the shared library version number
(which would mean libsoftokn3.so => libsoftokn4.so) to indicate
an incompatible change would disrupt all NSS users, whether they
use ECC or not.

As soon as we have made a decision, we should announce it in
the newsgroup and offer to help our customers rewrite their
code to work with both encodings of CKA_EC_POINT.  Hopefully
that'll avoid the need of another NSS environment variable.
(ECC support should still be mostly a checklist item today;
there should be very few ECC deployments.)

Comment 14

[Robert Relyea]

> I assume it is inappropriate for us to change PKCS #11 to match
> our implementation.

Especially if the spec is more self-consistant (it defines 2 attributes, both of them encapsulated. We encapsulate one but not the other attribute.

RE comment 10

I agree 1 and 2 are given. I have a patch for #2 already.

3 looks like a good compromise. 

There is still the issue of what NSS does when it writes the attribute (imports a public key for to sign with). Do we control that with an evironment variable, or do we attempt one way when we import and look for an error. (rembering that we know that the old softoken will not return an error when you import the encapsulated data).

bob

Comment 15

[Robert Relyea]

Attachment: Handle properly handled encodings, add an environment variable to allow old apps to continue to work.

Comment 16

[Robert Relyea]

Comment on attachment 369610 [details] [diff] [review]
Handle properly handled encodings, add an environment variable to allow old apps to continue to work.

This is a big patch, but hopefully it's bigness is mostly in comments to help the reviewer.

bob

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369610 [details] [diff] [review]
Handle properly handled encodings, add an environment variable to allow old apps to continue to work.

Sadly, r-.  There are several bugs in this patch, IINM.

This patch was really difficult to review, and took much more time 
than I would have guessed (for its size).  There were two primary
reasons for that:

a) This patch modifies a couple of functions that are already over
1000 lines long.  They have HUGE switch statements with a case for
every different mechanism, and the individual cases are usually 
more than a page of code each.  We REALLY need to break those up 
into a series of smaller functions with common signatures.

b) Despite the many new comments, and the fact that this patch has
more comments than lines of new code, there were still things that
were way too inobvious.  For example, new functions with NO comments
about which arguments are inputs and which are outputs.  They're
mixed (it's not true that all the input arguments come first, or 
all come last), and they don't use const for inputs arguments that
they really must never modify.  

But the reason for r- is not those issues.

1) in function PK11_ImportPublicKey, the new code in case ecKey 
leaks the allocated ckaId when it returns an error code.

2) New function pk11_get_Decoded_ECPoint, please rearrange the 
parameters so inputs come before outputs, and use const where 
possible for inputs.  Please add a comment explaining what each
argument is for.  This is not an error, but I spent a lot of 
time trying to figure this out.

3) The following new command and related code are incorrect, IINM.

>+    /* get the expected key length for the passed in curve.
>+     * pk11_get_EC_PointLenInBytes only returns valid values for curves
>+     * NSS has traditionally recognized. If the curve is not recognized,
>+     * it will return '0', and we have to figure out if the key was
>+     * encoded or not heuristically. If the ecParams are invalid, it
>+     * will return -1 for the keyLen.

I see no path in pk11_get_EC_PointLenInBytes that returns a negative 
value.  So, the following test is dead code.

>+     */
>+    keyLen = pk11_get_EC_PointLenInBytes(arena, ecParams);
>+    if (keyLen < 0) {
>+	return CKR_ATTRIBUTE_VALUE_INVALID;
>+    }


4) Big booboo here.  You coded a mem copy where you wanted a mem compare, I think.

>+	if ((rv != SECSuccess)
>+	    || ((publicKeyValue->len & 1) != 1)
>+	    || (publicKeyValue->data[0] != EC_POINT_FORM_UNCOMPRESSED)
>+	    || (PORT_Memcpy(&encodedPublicValue.data[encodedPublicValue.len -
>+		 	    publicKeyValue->len], publicKeyValue->data, 
>+			    publicKeyValue->len) != 0)) {


5) The patch fragment below looks wrong.  It uses the encoder to 
encode a new "pubValue", but then it frees the new pubValue without
ever using it.  It uses the input to the encoder, rather than the 
output.

>--- lib/softoken/pkcs11c.c	28 Jan 2009 19:53:39 -0000	1.108
>+++ lib/softoken/pkcs11c.c	27 Mar 2009 01:52:05 -0000
>@@ -4015,8 +4015,20 @@ dhgn_done:

>-	crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
>+	if (getenv("NSS_USE_NON_STANDARD_CKA_EC_POINT")) {
>+	    crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
> 				sftk_item_expand(&ecPriv->publicValue));
>+	} else {
>+	    SECItem *pubValue = SEC_ASN1EncodeItem(NULL, NULL, 
>+			&ecPriv->publicValue, SEC_OctetStringTemplate);
>+	    if (!pubValue) {
>+		crv = CKR_ARGUMENTS_BAD;
>+		goto ecgn_done;
>+	    }
>+	    crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
>+				sftk_item_expand(&ecPriv->publicValue));
>+	    SECITEM_FreeItem(pubValue, PR_TRUE);
>+	}

Items 4 and 5 are the big problems, the reasons for r-.

Other comments and suggestions.

1. I think that perhaps the big tables of error probabilities in your 
new comment don't really belong in the code.  Maybe they should be in 
this bug, and the code should contain a URL pointing to this bug.

2. Instead of NSS_USE_NON_STANDARD_CKA_EC_POINT, I would prefer
              NSS_USE_UNENCODED_CKA_EC_POINT
because customers who must use it for backward compatibility may have
to justify their decisions to do something obviously non-standard.  
I want to reduce inevitable pointless pushback from non-technical folks.

Comment 18

[Robert Relyea]

> a) This patch modifies a couple of functions that are already over
> 1000 lines long. 

I agree, the Derive switch has been a long standing issue. NSC_Derive needs to go into it's own file. Unfortunately now is not the time....;(

> I see no path in pk11_get_EC_PointLenInBytes that returns a negative 
> value.  So, the following test is dead code.

I prefer to future proof functions and allow an error case even if one does not yet exist. pk11_get_EC_PointLenInBytes has a comment where such a negative return code would be returned in the future.

I'll attach a patch with the rest of your comments addressed...


bob

Comment 19

[Robert Relyea]

Attachment: Version 2 compiles, (but not tested).

I'll request a review once I've tested this patch...

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369789 [details] [diff] [review]
Version 2 compiles, (but not tested).

This patch includes a change to pkcs11i.h that defines a new symbol,
CKA_EC_POINT_UNENCODED, which is unused anywhere.  I will assume that
it was not really intended to be part of this patch.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369789 [details] [diff] [review]
Version 2 compiles, (but not tested).

Bob, I'm satisfied that this patch addresses all my previous review
comments about correctness, so I'll give r+ to all parts of this patch
except for the patch to file lib/softoken/pkcs11i.h.  If your testing
shows that all is well, then you can commit.  

Since we are so close to the finish line, it would be good to also get 
another review of these changes, so I'll ask Wan-Teh.  But a second
review is not absolutely needed, as you know.

Comment 22

[Robert Relyea]

re CKA_EC_POINT_UNENCODED.... that's correct the code should not be part of this patch.

Unfortunately testing turned up a latent issue with old db's A new patch will be attached once I've addressed it.

Comment 23

[Robert Relyea]

Comment on attachment 369789 [details] [diff] [review]
Version 2 compiles, (but not tested).

patch fails all.sh

Comment 24

[Robert Relyea]

Attachment: patch version 3, complete, addresses Nelson's comments

Comment 25

[Robert Relyea]

... and passes all.sh.

I'll also attach two patches which will allow interdiff to work better.

The basic issue is the lgdb doesn't store attributes as they are passed in, but reconstructs attributes from it's native storage. To preserve the lgdb format, the patch includes the following fixes: 1) when creating a public key, the lgdb looks to see if the corresponding private key exists. and returns 'OK' if it does (This is because the old DB never did support storing public keys. It reflects a public key object for every private key object it has stored. The create code uses the CKA_EC_POINT attribute to find the data to look the private key up. The new patch tries decoding the CKA_EC_POINT first, and if it fails to look the key up with that value, fall back to the old one.

The second thing this patch does is properly encode the CKA_EC_POINT on out put if the environment variable is set.

Comment 26

[Robert Relyea]

Attachment: Patch version 3 Subset, part 1, DOES NOT interdiff the the previously attached patches. :(

Comment 27

[Robert Relyea]

Attachment: Patch version 3 Subset, part 2, just contains the new files.

Comment 28

[Robert Relyea]

Hmm it interdiffs for me. It interdiffs with the first patch with no warning, and the second patch with a warning, but the interdiff is faithful to the changes. (the second patch does not have the spurious change to pkcs11i.h you noted previously).

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370090 [details] [diff] [review]
Patch version 3 Subset, part 2, just contains the new files.

I see two problems with this patch, both in error cases.

1. No test is made to see if arena is NULL, and deal with it.
Instead, the code just behaves as if the input was undecodable.
That doesn't seem right, but if that's what you want to do, then
there should be a comment.

>+	arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>+
>+	rv= SEC_QuickDERDecodeItem(arena, &pubKey2Space, 
>+				SEC_OctetStringTemplate, pubKey);
>+	if (rv != SECSuccess) {
>+	    /* decode didn't work, just try the pubKey */
>+	    PORT_FreeArena(arena, PR_FALSE);
>+	    arena = NULL;
>+	} else {
>+	    /* try the decoded pub key first */
>+	    pubKey = &pubKey2Space;
>+	}
>+    }
>+#endif /* NSS_ENABLE_ECC */
>+
>+    PORT_Assert(pubKey->data);

2. This assert should be followed by code to deal with the problem 
in optimized builds.  Instead, we'll just crash in optimized builds.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370089 [details] [diff] [review]
Patch version 3 Subset, part 1, DOES NOT interdiff the the previously attached patches. :(

Well, if you count interdiff showing all the diffs as if they were in one file, the file that was only present in one diff, then, yes, I guess you could call it clean.  :)

Anyway, this part looks OK.  Thanks.
The new second part is close.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370087 [details] [diff] [review]
patch version 3, complete, addresses Nelson's comments

I gave separate reviews to the two parts.  
P1 was ok, P2 wasn't.

Comment 32

[Robert Relyea]

Attachment: (wrong patch)

This patch has passed the first run with all.sh. I'm making a second run turning on the NSS_USE_DECODED_CKA_EC_POINT environment variable, but I fully expect that test to pass.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370249 [details] [diff] [review]
(wrong patch)

Bob, is this the right patch?
This patch modifies 6 files, NONE of which have ever been 
modified by any of the preceding patches for this bug.
So, this patch doesn't address review comments about preceding 
patches for this bug.  
Maybe this patch is for another bug?

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370249 [details] [diff] [review]
(wrong patch)

This is some old nickname patch.  Not an EC_POINT patch.

Comment 35

[Robert Relyea]

Attachment: (wrong patch)

I'm not sure what happened. I may have slipped the mouse when selecting the patch

Comment 36

[Robert Relyea]

Comment on attachment 370255 [details] [diff] [review]
(wrong patch)

Nope... same wrong patch

Comment 37

[Robert Relyea]

Attachment: Patch version 4, compare with version 3 part 2

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370257 [details] [diff] [review]
Patch version 4, compare with version 3 part 2

r=nelson
Please make these additional changes to lgcreate.c:
Please initialize the SECItems pubKeySpace and pubKey2Space,
so that there's no way that we can arrive at label done with 
them still uninitialized, e.g. 


>-    SECItem pubKey;
>+    SECItem pubKeySpace;
>+    SECItem *pubKey;
>+#ifdef NSS_ENABLE_ECC
>+    SECItem pubKey2Space;

Make those 
      SECItem pubKeySpace = { siBuffer, NULL, 0 };
and 
      SECItem pubKey2Space = { siBuffer, NULL, 0 };

Comment 39

[Robert Relyea]

Checking in pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.26; previous revision: 1.25
done
Checking in pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.113; previous revision: 1.112
done
Checking in softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.164; previous revision: 1.163
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.110; previous revision: 1.109
done
Checking in softoken/legacydb/lgattr.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgattr.c,v  <--  lgattr.c
new revision: 1.8; previous revision: 1.7
done
Checking in softoken/legacydb/lgcreate.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgcreate.c,v  <--  lgcreate.c
new revision: 1.4; previous revision: 1.3
done


with nelson's suggested initialization.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

This broke the build on WIndows

The attempt to link softoken's DLL fails with the message:

unresolved external symbol _SEC_OctetStringTemplate_Util

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Julien, can you suggest a good fix for this problem?  

Note that if SEC_OctetStringTemplate has never been exported, then IMO, the 
solution is to export that symbol, and NOT SEC_OctetStringTemplate_Util !

Comment 42

[Robert Relyea]

The problem isn't the _Util. It's the lack of use of the SEC_ASN1_GET() Macro.

I can supply a patch, but I don't have a windows box to build it on here at home.

bob

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

I'm working on a patch now.  I just built it.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - add SEC_ASN1_GET macros (checked in)

This builds.  I presume it is as correct as the code without it is on
non-Windows platforms.

Comment 45

[Robert Relyea]

Comment on attachment 370294 [details] [diff] [review]
patch v1 - add SEC_ASN1_GET macros (checked in)

I just built precisely the same patch on Linux....

The patch is not complete, though, there are 4 more instances in pk11wrap, 2 in pk11akey.c and 2 in pk11skey.c

Comment 46

[Julien Pierre]

Nelson,

NSS_Get_SEC_OctetStringTemplate_Util is already exported from util. As long as you use the proper macros to import that template, Windows should build. No changes to util should be necessary. Only to the code that uses this template.

If you are using this as a subtemplate, try placing SEC_ASN1_MKSUB(SEC_OctetStringTemplate) in your source file, and use SEC_ASN1_GET(SEC_OctetStringTemplate) . See the working example in genname.c .

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

pkcs11.c;            new revision: 1.165; previous revision: 1.164
pkcs11c.c;           new revision: 1.111; previous revision: 1.110
legacydb/lgattr.c;   new revision: 1.9; previous revision: 1.8
legacydb/lgcreate.c; new revision: 1.5; previous revision: 1.4

In reply to comment 45:
> The patch is not complete, though, there are 4 more instances in pk11wrap, 
> 2 in pk11akey.c and 2 in pk11skey.c

Well, those don't cause any build problems for me.

Comment 48

[Julien Pierre]

Attachment: Fix broken build

This fixes the problems reported by Bob in comment 45.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370304 [details] [diff] [review]
Fix broken build

Something must be wrong with my depend builds.
Anyway, please fold the too-long lines in this patch,
then r=nelson

Comment 50

[Julien Pierre]

Checking in pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.27; previous revision: 1.26
done
Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.114; previous revision: 1.113
done

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

So, is this ready to be marked resolved/fixed again?

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

I'm declaring victory on this bug (again :).

Comment 53

[Robert Relyea]

Yes. The cryptoki group is talking about using the stripped version (non-encoded) for the Derive call, but that would only affect pk11wrap.

Comment 54

[Edouard Oger [:eoger]]

Attachment: bobo.c

I'm seeing something similar to what's described on the bug with NSS@HEAD by using SECKEY_ConvertToPublicKey.
I've attached a minimal repro.
The bug doesn't trigger all the time so I suggest you run the repro in a loop: while ./a.out; do :; done.

Example of output:

rawPubKey: [4 63 63 81 58 220 211 33 36 61 159 80 179 133 188 239 101 14 94 64 239 201 239 62 230 35 20 172 2 19 30 96 45 52 177 172 102 78 103 162 22 53 227 151 186 221 138 84 130 194 127 3 49 240 198 251 36 104 51 63 228 239 241 150 64 ]

convertedPubKey: [63 81 58 220 211 33 36 61 159 80 179 133 188 239 101 14 94 64 239 201 239 62 230 35 20 172 2 19 30 96 45 52 177 172 102 78 103 162 22 53 227 151 186 221 138 84 130 194 127 3 49 240 198 251 36 104 51 63 228 239 241 150 64 ]