Last Comment Bug 401610 - Shared DB fails on IOPR tests
: Shared DB fails on IOPR tests
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: x86 Linux
: -- normal (vote)
: 3.12
Assigned To: Robert Relyea
:
Mentors:
Depends on:
Blocks: 398379
  Show dependency treegraph
 
Reported: 2007-10-29 13:42 PDT by Robert Relyea
Modified: 2007-10-30 10:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
write the public key out when importing a PKCS #12 blob (6.80 KB, patch)
2007-10-29 14:01 PDT, Robert Relyea
alvolkov.bgs: review-
Details | Diff | Review
version2: write public key from pkcs12 (6.87 KB, patch)
2007-10-29 16:16 PDT, Robert Relyea
alvolkov.bgs: review+
nelson: review+
Details | Diff | Review
Version 3: eliminate redundant params (9.98 KB, patch)
2007-10-29 17:34 PDT, Robert Relyea
nelson: review-
Details | Diff | Review
Version 3b: remove const (9.42 KB, patch)
2007-10-29 18:19 PDT, Robert Relyea
nelson: review+
Details | Diff | Review

Description Robert Relyea 2007-10-29 13:42:35 PDT
Turning on IOPR causes the shared DB tests (but not upgrade DB tests) to fail.
Comment 1 Robert Relyea 2007-10-29 13:48:21 PDT
In the normal database, we implicitly create a public key for every private key imported. NSS is supposed to create the public key explicitly as well (so that the public key will be on other tokens).

Lack of a public key shows up when we try to determine if a given certificate is a user cert. If we have not yet logged in, we can't search for private keys, so we use the public key to determine this. It turns out some tests in the IOPR tests end up looking for certs which have been imported with pkcs #12 before the token is authenticated. The PKCS #12 code is not importing the public key along with the private key.

I'll attach a patch that corrects the problem.

Comment 2 Robert Relyea 2007-10-29 14:01:49 PDT
Created attachment 286601 [details] [diff] [review]
write the public key out when importing a PKCS #12 blob

This patch writes the public key out when importing a pkcs 12 blob.
This patch also adds the CKA_ID to a public key if that public key is a token public key.
It also fixes a bug in ssl_iopr.sh where the ssl failure message was not written to the error log.
Comment 3 Robert Relyea 2007-10-29 14:08:48 PDT
Comment on attachment 286601 [details] [diff] [review]
write the public key out when importing a PKCS #12 blob

This should be a short patch to review...
Comment 4 Alexei Volkov 2007-10-29 14:44:11 PDT
Comment on attachment 286601 [details] [diff] [review]
write the public key out when importing a PKCS #12 blob

Looks good, but r- for two reasons:

1. Should set PORT_SetError(SEC_ERROR_BAD_KEY) in case when ckaid == NULL because the key type
is not one of rsa, dsa, ec and dh key types.

> 						 sizeof(CK_BBOOL) ); attrs++;
>+    if (isToken) {
>+	ckaId = pk11_MakeIDFromPublicKey(pubKey);
>+	if (ckaId == NULL) {
>+	    return CK_INVALID_HANDLE;
>+	}
>+	PK11_SETATTRS(attrs, CKA_ID, ckaId->data, ckaId->len); attrs++;
>+    }


2. pubKey should be check for NULL before an attempt to import it.

   } else {
>+	/* try to import the public key. Failure to do so is not fatal,
>+	 * not all tokens can store the public key */
>+	PK11_ImportPublicKey(key->slot, pubKey, PR_TRUE);
> 	key->installed = PR_TRUE;
>     }
Comment 5 Robert Relyea 2007-10-29 16:16:08 PDT
Created attachment 286620 [details] [diff] [review]
version2: write public key from pkcs12

This patch corrects the issues alexei found. The first issue was definately a bug that needed to be fixed. The second issue could not happen (these are private functions and I can show that pubKey could never be NULL), but it also doesn't hurt and makes the code more robust against changes.
Comment 6 Alexei Volkov 2007-10-29 16:29:26 PDT
Comment on attachment 286620 [details] [diff] [review]
version2: write public key from pkcs12

r=alexei
Comment 7 Nelson Bolyard (seldom reads bugmail) 2007-10-29 16:38:56 PDT
Comment on attachment 286620 [details] [diff] [review]
version2: write public key from pkcs12


> static SECStatus
> sec_pkcs12_add_key(sec_PKCS12SafeBag *key, SECItem *publicValue, 
>-		   KeyType keyType, unsigned int keyUsage, 
>+		   SECKEYPublicKey *pubKey, KeyType keyType, 
>+		   unsigned int keyUsage, 
> 		   SECItem *nickName, void *wincx)


>-		rv = sec_pkcs12_add_key(key, publicValue, keyType, 
>+		rv = sec_pkcs12_add_key(key, publicValue, pubKey, keyType, 
> 		                        keyUsage, nickName, wincx);

Having both publicValid *and* pubKey as arguments to the same function
seems redundant.  Is it not so?
Comment 8 Robert Relyea 2007-10-29 16:53:53 PDT
publicValue can be derived from the public key, though they aren't the same. (public Value is a component of the public key.
Comment 9 Robert Relyea 2007-10-29 17:17:04 PDT
I'll attach a patch sortly that removes keyType and publicValue from sec_pkcs12_add_key and extracts that value from the public key.

It simplifies some of the memory management as well...
Comment 10 Robert Relyea 2007-10-29 17:34:49 PDT
Created attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

This version removes the redundant use of publicValue, type and public key.
Comment 11 Robert Relyea 2007-10-29 17:44:45 PDT
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

Interdiff should work to show the change from the previous patch
Comment 12 Nelson Bolyard (seldom reads bugmail) 2007-10-29 17:53:18 PDT
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

I like this version a lot better.   I have a few quibbles over
the use of "const".  I think that in at least one of the places
where you used const, some compilers would declare a fatal error.





>+static const SECItem *
>+sec_pkcs12_get_public_value_and_type(SECKEYPublicKey *pubKey, KeyType *type);


>+    const SECItem *publicValue = NULL;

>+    /* get the value and type from the public key */
>+    publicValue = sec_pkcs12_get_public_value_and_type(pubKey, &keyType);

> 	    rv = PK11_ImportPrivateKeyInfo(key->slot, 
> 			  key->safeBagContent.pkcs8KeyBag, 
>-			  nickName, publicValue, PR_TRUE, PR_TRUE,
>+			  nickName, (SECItem *)publicValue, PR_TRUE, PR_TRUE,
> 			  keyUsage,  wincx);

Here you declare publicValue to be a pointer to const SECItem, but you 
cast away the const nature of it.  I think casting away const is bad.
In this case, I think the solution is for it not to be declared const 
in the first place, and for sec_pkcs12_get_public_value_and_type to return
a pointer to a non-const SECItem

> 	    rv = PK11_ImportEncryptedPrivateKeyInfo(key->slot,
> 					key->safeBagContent.pkcs8ShroudedKeyBag,
>-					key->pwitem, nickName, publicValue, 
>+					key->pwitem, nickName, 
>+					(SECItem *)publicValue, 
> 					PR_TRUE, PR_TRUE, keyType, keyUsage,
> 					wincx);

Again, casting away const.




>+static const SECItem *
>+sec_pkcs12_get_public_value_and_type(SECKEYPublicKey *pubKey,
>+					KeyType *type)
>+{
>+    SECItem *pubValue = NULL;

This variable is the value to be returned, but it is not the
same type as the declared return value type.  It is not a pointer
to const.

>-	    pubValue = SECITEM_DupItem(&pubKey->u.dsa.publicValue);
>+	    pubValue = &pubKey->u.dsa.publicValue;

>-	    pubValue = SECITEM_DupItem(&pubKey->u.dh.publicValue);
>+	    pubValue = &pubKey->u.dh.publicValue;

>-	    pubValue = SECITEM_DupItem(&pubKey->u.rsa.modulus);
>+	    pubValue = &pubKey->u.rsa.modulus;

>-	    pubValue = SECITEM_DupItem(&pubKey->u.ec.publicValue);
>+	    pubValue = &pubKey->u.ec.publicValue;

None of the values above are addresses of const SECItems.

>     return pubValue;
> }

I'd expect that return to generate errors on some platforms, 
because it's returning a pointer to non-const SECItem, but the 
function's definition says it returns pointer to const.

I think the solution is to just not use const in any of these places.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2007-10-29 17:54:31 PDT
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

r- due to the const issues.
I'm open to pursuasion if these are really non-issues.
Comment 14 Robert Relyea 2007-10-29 18:03:37 PDT
I'd be very surprised that a compiler would choke on this use of const. You can
always implicitly cast from non-const to const, just not the other way.

const here is to indicate the caller can't modify the value (and that the
caller doesn't need to free the value).

The NSS functions where we cast the const away are because those functions
(like many NSS functions) do not correctly declare their const-ness.

I will remove the const usage if you insist, however (or if you can actually
point to a compiler that will actually choke on this;).

bob
Comment 15 Robert Relyea 2007-10-29 18:19:53 PDT
Created attachment 286627 [details] [diff] [review]
Version 3b: remove const

This version removes the const, but I prefer the const value myself. I am quite sure the going to non-const to const is not a compilier error on any modern compilier (that is any compilier that even knows about const). Anyway in the interest in getting this in, I've supplied both patches with my rational. I'll check in whichever has the r+ tomorrow;).

bob
Comment 16 Robert Relyea 2007-10-29 18:21:26 PDT
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

 resetting this to review ? now that I have given the const rational. set to r+ if you are convinced or r- if not and r+ the const removal patch.

Thank,

bob
Comment 17 Nelson Bolyard (seldom reads bugmail) 2007-10-29 18:37:42 PDT
Comment on attachment 286627 [details] [diff] [review]
Version 3b: remove const

I prefer this one. 
I'm sure this is OK with all compilers.
Comment 18 Nelson Bolyard (seldom reads bugmail) 2007-10-29 19:08:35 PDT
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

Even if it is always permissible to implicitly cast non-const to const in all circumstances, in this case, the patch casts non-const to const, and then must cast away const in all uses of that const result.  
I'd much rather not cast away const.  Every time I read code that casts away const, I'm suspicious of it, and wonder if it is a lurking bug.  I'd rather that the code not cast away const if it can be avoided.  Since nothing about this struct was inherently ever const, I see no benefit to casting it to const for a very short time, and then casting it away again.
Comment 19 Robert Relyea 2007-10-30 10:20:18 PDT
I'll check in the patch without the const. (though I really hate loosing the self-documentation in the code.

The casting away of const is really because of a long standing problem in NSS itself (we don't actually report const values appropriately in many cases), so you are correct, the bug is the fact that the functions we are calling is not declared const. (BTW if the did modify the parameters, that to would lead to bugs, whether or not I actually declare the functions a const).

Comment 20 Robert Relyea 2007-10-30 10:31:57 PDT
I'd be very surprised that a compiler would choke on this use of const. You can
always implicitly cast from non-const to const, just not the other way.

const here is to indicate the caller can't modify the value (and that the
caller doesn't need to free the value).

The NSS functions where we cast the const away are because those functions
(like many NSS functions) do not correctly declare their const-ness.

I will remove the const usage if you insist, however (or if you can actually
point to a compiler that will actually choke on this;).

bob
Comment 21 Robert Relyea 2007-10-30 10:32:28 PDT
Checking in tests/iopr/ssl_iopr.sh;
/cvsroot/mozilla/security/nss/tests/iopr/ssl_iopr.sh,v  <--  ssl_iopr.sh
new revision: 1.3; previous revision: 1.2
done
Checking in lib/pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.19; previous revision: 1.18
done
Checking in lib/pkcs12/p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.38; previous revision: 1.37
done

Note You need to log in before you can comment on or make changes to this bug.