166894 - Changing trust on a token cert causes a failure.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

If you have a cert in a token that's writable, but does not support trust
objects, then we may get a failure when trying to change the trust on that token.

Comment 1

[Robert Relyea]

Attachment: Handle Trust Change corner cases. (for NSS_3_4_BRANCH)

This patch is for NSS 3.4. In consists of two pieces: part 1, translate the
current NSS 3.6 fixes for handling changing trust on a certificate if it lives
in multiple tokens to NSS 3.4. This is the code around stan_GetTokenTrust().

Part 2 needs to be included in NSS 3.6. It is the code around the failure to
import trust case. In this case the R/W token we selected does not understand
trust objects, we move the cert to the internal token (which does understand
trust objects), then change the trust there.

Comment 2

[Robert Relyea]

Ian, could you review this patch (remember it's against NSS 3.4.2 branch).

thanks,

bob

Comment 3

[Wan-Teh Chang]

Attachment: Incremental patch (for NSS_3_4_BRANCH)

This incremental patch should be applied on top of
Bob's patch to fix a bug in his patch.

Comment 4

[Ian McGreer]

Comment on attachment 97968 [details] [diff] [review]
Handle Trust Change corner cases. (for NSS_3_4_BRANCH)


reviewed this as major part of patch, next patch also required

I never thought so little code could be so confusing :)

Comment 5

[Ian McGreer]

Comment on attachment 97976 [details] [diff] [review]
Incremental patch (for NSS_3_4_BRANCH)


good catch, wtc

Comment 6

[Wan-Teh Chang]

Attachment: Destroy nssTrust before returning from STAN_ChangeCertTrust (for NSS_3_4_BRANCH)

In STAN_ChangeCertTrust, we are not always destroying
'nssTrust' before returning.  This seems like a memory
leak on those code paths.

This patch adds a 'done' label to the function and
we return from the function by 'goto done'.  This
ensures that 'nssTrust' is destroyed before returning.

Please review this patch.  This patch is against the
NSS_3_4_BRANCH.  The tip needs a similar patch.  Thanks.

Comment 7

[Wan-Teh Chang]

Attachment: Destroy nssTrust before returning from STAN_ChangeCertTrust (for the tip of NSS)

Comment 8

[Robert Relyea]

Attachment: Store trust in internal token if trust in target token fails. (for the tip of NSS)

This patch is the NSS tip equivalent of 97968, and should be targeted for NSS
3.6.

Comment 9

[Robert Relyea]

With 3.4 partion fixed, not targetting 3.6

Comment 10

[Robert Relyea]

With 3.4 partion fixed, now targetting 3.6

Comment 11

[Robert Relyea]

The merge of mine and Wan-Teh's patches should be straight forward. The don't
modify any common code. The only addition would be my new:

if (!newInstance) {
    return PR_FAILURE;
}

should become:

if (!newInstance) {
    nssrv = PR_FAILURE;
    goto done;
}


bob

Comment 12

[Wan-Teh Chang]

Thanks, Bob, for the code review.  I've checked in
my patches (to destroy nssTrust before returning
from STAN_ChangeCertTrust) on the tip and NSS_3_4_BRANCH.

Are you waiting for a code review of your patch?

Comment 13

[Robert Relyea]

Yes.

Comment 14

[Wan-Teh Chang]

Comment on attachment 100025 [details] [diff] [review]
Store trust in internal token if trust in target token fails. (for the tip of NSS)

I verified that this is the equivalent of
attachment 97968 [details] [diff] [review] for the tip.  (Remember to
make the 'goto done' change because my patch
is already checked in.)  It would be nice if
Ian could also review this patch.

Question: the code on the tip does this:

    newInstance = nssToken_ImportCertificate(...);
    if (!newInstance) {
	return PR_FAILURE;
    }
    nssPKIObject_AddInstance(&c->object, newInstance);
    newInstance = nssToken_ImportTrust(...);

If nssToken_ImportTrust fails, do we need to destroy
the instance that was added to c->object by the
preceding nssPKIObject_AddInstance call?

Comment 15

[Ian McGreer]

I can answer that.

No -- that instance was for a certificate, and was added to the PKICertificate.
 The variable is then reused for a trust object.  If the trust import fails,
that does not change the fact that the cert import already succeeded, and
therefore a new cert instance was created on the token.

Comment 16

[Wan-Teh Chang]

The whole purpose of importing the cert to some other
token is so that we can import the trust to the same
token.  So, if the trust import fails, it seems that
we should also undo the cert import.

Comment 17

[Ian McGreer]

Sure -- I didn't look at the rest of the code.  I thought, based on the fragment
you showed, that you were wondering whether the memory occupied by newInstance
needed to be freed.  That is not the case.  However, if the trust import fails,
the actual cert instance should be deleted via nssToken_DeleteStoredObject first
need to call nssPKIObject_RemoveInstanceForToken to get rid of the cert's
reference to the instance).

Comment 18

[Ian McGreer]

I have the order reversed.  This is maybe a bit confusing (it is to me now,
anyway), but you call nssToken_DeleteStoredObject and then
nssPKIObject_RemoveInstanceForToken.

Comment 19

[Ian McGreer]

Attachment: delete specific token instances from objects

Per my last two comments, this patch is a better way to handle the case
described.  First of all, there were no callers of
nssPKIObject_RemoveInstanceForToken, secondly, I can think of no reason to call
that function other than to delete the instance.  So I combined the two
operations into one function.  This should be called to delete the cert
instance if the trust import fails.

Comment 20

[Wan-Teh Chang]

Thomas, could you try /u/wtc/thomask/mozilla (debug and
optimized builds for Solaris 8) on a hardware token?
That source tree has Bob's patch for NSS 3.6.  Thanks.

Comment 21

[Robert Relyea]

I'm a little worried about blindly deleting the cert on import failure. I'm not
so sure that in the current code, that we don't try to import the cert even if
it has already been imported. In that case if we delete the cert we delete an
entry that was already there.

On the other hand, if we import the cert, but fail to set the trust, we haven't
harmed anything. The cert is in the db or token, but it shouldn't interfere with
operations.

I think we should put ian's patch in longer term (3.7) as long as we can
convince ourselves that the import is unique (or add code to guarrentee it is
unique), and we're able to get some testing against the new code.

bob

Comment 22

[Robert Relyea]

The blocking part of this bug has been checked in, I vote we move the last part
out to 3.7, as long as Thomas is able to test with this patch.

bob

Comment 23

[Ian McGreer]

I opened bug 171969 about the final matter.  We should close this bug.

Comment 24

[Robert Relyea]

Marking fixed. The remaining issue has been reported in a separate bug targetted
for 3.7. Leaving the target on this bug as 3.6 so that it shows up on any
attempt to list the fixed in 3.6

Comment 25

[Wan-Teh Chang]

Comment on attachment 100574 [details] [diff] [review]
delete specific token instances from objects

Marked the patch obsolete.  The issue addressed by this patch
is now bug 171969.