Shared DB fails on IOPR tests

RESOLVED FIXED in 3.12

Status

NSS
Libraries
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

9.98 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review-
Details | Diff | Splinter Review
9.42 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Turning on IOPR causes the shared DB tests (but not upgrade DB tests) to fail.
(Assignee)

Comment 1

10 years ago
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.

(Assignee)

Updated

10 years ago
Blocks: 398379
(Assignee)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
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...
Attachment #286601 - Flags: review?(alexei.volkov.bugs)

Comment 4

10 years ago
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;
>     }
Attachment #286601 - Flags: review?(alexei.volkov.bugs) → review-
(Assignee)

Comment 5

10 years ago
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.
Attachment #286601 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #286620 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED

Comment 6

10 years ago
Comment on attachment 286620 [details] [diff] [review]
version2: write public key from pkcs12

r=alexei
Attachment #286620 - Flags: review?(alexei.volkov.bugs) → review+
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?
Attachment #286620 - Flags: review+
(Assignee)

Comment 8

10 years ago
publicValue can be derived from the public key, though they aren't the same. (public Value is a component of the public key.
(Assignee)

Comment 9

10 years ago
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...
(Assignee)

Comment 10

10 years ago
Created attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

This version removes the redundant use of publicValue, type and public key.
Attachment #286620 - Attachment is obsolete: true
(Assignee)

Comment 11

10 years ago
Comment on attachment 286622 [details] [diff] [review]
Version 3: eliminate redundant params

Interdiff should work to show the change from the previous patch
Attachment #286622 - Flags: review?(nelson)
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.
Attachment #286622 - Attachment description: Version 3: elliminate redundant params → Version 3: eliminate redundant params
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.
Attachment #286622 - Flags: review?(nelson) → review-
(Assignee)

Comment 14

10 years ago
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
(Assignee)

Comment 15

10 years ago
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
Attachment #286627 - Flags: review?(nelson)
(Assignee)

Comment 16

10 years ago
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
Attachment #286622 - Flags: review- → review?(nelson)
Comment on attachment 286627 [details] [diff] [review]
Version 3b: remove const

I prefer this one. 
I'm sure this is OK with all compilers.
Attachment #286627 - Flags: review?(nelson) → review+
Target Milestone: --- → 3.12
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.
Attachment #286622 - Flags: review?(nelson) → review-
(Assignee)

Comment 19

10 years ago
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).

(Assignee)

Comment 20

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

10 years ago
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
You need to log in before you can comment on or make changes to this bug.