Last Comment Bug 166894 - Changing trust on a token cert causes a failure.
: Changing trust on a token cert causes a failure.
Status: RESOLVED FIXED
[3.4.3]
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4.2
: x86 Windows 2000
: P1 blocker (vote)
: 3.6
Assigned To: Robert Relyea
: Bishakha Banerjee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-05 10:49 PDT by Robert Relyea
Modified: 2003-03-04 11:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Handle Trust Change corner cases. (for NSS_3_4_BRANCH) (2.78 KB, patch)
2002-09-05 10:54 PDT, Robert Relyea
bugz: review+
Details | Diff | Splinter Review
Incremental patch (for NSS_3_4_BRANCH) (900 bytes, patch)
2002-09-05 11:34 PDT, Wan-Teh Chang
bugz: review+
Details | Diff | Splinter Review
Destroy nssTrust before returning from STAN_ChangeCertTrust (for NSS_3_4_BRANCH) (1.96 KB, patch)
2002-09-17 15:36 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Destroy nssTrust before returning from STAN_ChangeCertTrust (for the tip of NSS) (1.52 KB, patch)
2002-09-17 15:44 PDT, Wan-Teh Chang
rrelyea: review+
Details | Diff | Splinter Review
Store trust in internal token if trust in target token fails. (for the tip of NSS) (1.97 KB, patch)
2002-09-20 15:23 PDT, Robert Relyea
wtc: review+
Details | Diff | Splinter Review
delete specific token instances from objects (1.60 KB, patch)
2002-09-25 10:08 PDT, Ian McGreer
no flags Details | Diff | Splinter Review

Description Robert Relyea 2002-09-05 10:49:59 PDT
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 2002-09-05 10:54:34 PDT
Created attachment 97968 [details] [diff] [review]
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 2002-09-05 10:55:26 PDT
Ian, could you review this patch (remember it's against NSS 3.4.2 branch).

thanks,

bob
Comment 3 Wan-Teh Chang 2002-09-05 11:34:42 PDT
Created attachment 97976 [details] [diff] [review]
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 2002-09-05 12:59:53 PDT
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 2002-09-05 13:00:30 PDT
Comment on attachment 97976 [details] [diff] [review]
Incremental patch (for NSS_3_4_BRANCH)


good catch, wtc
Comment 6 Wan-Teh Chang 2002-09-17 15:36:51 PDT
Created attachment 99575 [details] [diff] [review]
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 2002-09-17 15:44:06 PDT
Created attachment 99577 [details] [diff] [review]
Destroy nssTrust before returning from STAN_ChangeCertTrust (for the tip of NSS)
Comment 8 Robert Relyea 2002-09-20 15:23:13 PDT
Created attachment 100025 [details] [diff] [review]
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 2002-09-20 15:24:37 PDT
With 3.4 partion fixed, not targetting 3.6
Comment 10 Robert Relyea 2002-09-20 15:24:58 PDT
With 3.4 partion fixed, now targetting 3.6
Comment 11 Robert Relyea 2002-09-24 11:29:13 PDT
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 2002-09-24 12:01:25 PDT
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 2002-09-24 17:15:10 PDT
Yes.
Comment 14 Wan-Teh Chang 2002-09-24 19:06:36 PDT
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 2002-09-25 08:50:09 PDT
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 2002-09-25 09:03:32 PDT
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 2002-09-25 09:42:08 PDT
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 2002-09-25 09:48:04 PDT
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 2002-09-25 10:08:51 PDT
Created attachment 100574 [details] [diff] [review]
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 2002-09-27 11:46:58 PDT
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 2002-09-30 18:19:50 PDT
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 2002-09-30 18:23:11 PDT
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 2002-10-01 12:32:00 PDT
I opened bug 171969 about the final matter.  We should close this bug.
Comment 24 Robert Relyea 2002-10-01 13:11:29 PDT
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 2003-03-04 11:24:45 PST
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.

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