129298 - Multiple nickname sources confuse NSS

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

The same cert can have multiple nicknames depending on which token it comes
from. Therefore, the object Nickname should be stored in the objectInstance
structure so nickname construction will be consistant for a given token.

Comment 1

[Wan-Teh Chang]

Assigned the bug to Ian and targeted it for NSS 3.4.
Ian, this is the bug we talked about at the NSS meeting
today.

Comment 2

[Ian McGreer]

Attachment: associate nicknames with token instances

I created a function NSSCertificate_GetNickname, which takes an NSSToken as an
argument, returning the cert's nickname for that token if provided, else just
the nickname for the first token in the instance list.

Bob, review?  Wan-Teh, approval?  This is a fairly large patch, but the changes
are straightforward.

Comment 3

[Robert Relyea]

Looks pretty good, just a couple of comments:
1) I don't think create_instance should fail if you fail to find a label, we
should just set the label to "". not all objects have labels. We didn't fail on
read of a cert if it didn't have a label.
2) In the future we probably want some more 'intellegent' default nickname
selection. For now I think this should be OK.

bob

Comment 4

[Ian McGreer]

create_instance doesn't fail if there is no nickname.  nssCKObject_GetAttributes
only returns failure for a worse kind of error -- if the attribute doesn't
exist, it sets the output to NULL.

I've already decided I want to split that function into
nssCKObject_GetOptionalAttributes and nssCKObject_GetRequiredAttributes, in
order to make the distinction clear (the second function *would* fail if the
attribute didn't exist).  But that is for later.

Comment 5

[Robert Relyea]

This is a change from our previous code (which succeeded if the attribute didn't
exist).

I think you are right, this *shouldn't* be an issue in a properly implemented
token (even if LABEL isn't specified, it's a required parameter for all objects).

My paranoia about some non-complient token failing still exists. I'm trying to
think of the downside returning an instance with doesn't have a label if it
fails to find the attribute.

Anyway it's not as serious as I had originally thought. I had forgotten that
LABEL is a required attribute for all objects.

bob

Comment 6

[Ian McGreer]

Attachment: rev 2, based on wtc comments

Bob,

This code still succeeds if the CKA_LABEL attribute doesn't exist.  The
nickname will be NULL, as before.  So it will be no worse off for non-compliant
tokens than it was previously.

Comment 7

[Robert Relyea]

OK, then the patch is fine.

bob

Comment 8

[Wan-Teh Chang]

Attachment: Additional patch, to be applied on top of the previous patch

This patch fixes the problem that the local variable
'stanNick' may be used uninitialized.  It has been
checked in.

Comment 9

[Wan-Teh Chang]

*** Bug 129594 has been marked as a duplicate of this bug. ***

Comment 10

[Wan-Teh Chang]

Comment on attachment 73035 [details] [diff] [review]
rev 2, based on wtc comments

r=wtc.	I reviewed and approved this patch yesterday.

Comment 11

[Wan-Teh Chang]

Comment on attachment 73202 [details] [diff] [review]
Additional patch, to be applied on top of the previous patch

Ian reviewed this patch yesterday.

Comment 12

[Ian McGreer]

Attachment: fix a nickname collision bug

Wan-Teh noticed we are using PORT_Free on a buffer allocated in an arena. 
Specifically, if someone calls CERT_NewTempCertificate with one nickname, then
calls CERT_AddTempCertToPerm with a *different* nickname, we will make an
incorect call to PORT_Free.

This patch would fix that case.

Comment 13

[Ian McGreer]

The problem described in comment #12 is restricted to callers of
CERT_AddTempCertToPerm, which is supposed to be nobody, but is PSM.

PSM should not encounter the problem, however, since I verified it does not use
that code path.  It will encounter the small leak caused by the PORT_Strdup
call, but that leak should be very minor, and only occurs when a cert is downloaded.

Wan-Teh is opening a new bug for this issue.  This bug is closed.

Comment 14

[Wan-Teh Chang]

Comment on attachment 73209 [details] [diff] [review]
fix a nickname collision bug

I opened bug 129709 for this new issue.