Changing trust on a token cert causes a failure.

RESOLVED FIXED in 3.6

Status

NSS
Libraries
P1
blocker
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

3.4.2
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.4.3])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

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

thanks,

bob

Comment 3

15 years ago
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

15 years ago
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 :)
Attachment #97968 - Flags: review+

Comment 5

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


good catch, wtc
Attachment #97976 - Flags: review+
(Assignee)

Updated

15 years ago
Version: 3.4.2 → 3.6

Updated

15 years ago
Target Milestone: --- → 3.4.3
Version: 3.6 → 3.4.2

Comment 6

15 years ago
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

15 years ago
Created attachment 99577 [details] [diff] [review]
Destroy nssTrust before returning from STAN_ChangeCertTrust (for the tip of NSS)

Updated

15 years ago
Attachment #99575 - Attachment description: Destroy nssTrust before returning from STAN_ChangeCertTrust → Destroy nssTrust before returning from STAN_ChangeCertTrust (for NSS_3_4_BRANCH)
(Assignee)

Comment 8

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

Comment 9

15 years ago
With 3.4 partion fixed, not targetting 3.6
Target Milestone: 3.4.3 → 3.6
(Assignee)

Comment 10

15 years ago
With 3.4 partion fixed, now targetting 3.6
(Assignee)

Updated

15 years ago
Attachment #99577 - Flags: review+
(Assignee)

Comment 11

15 years ago
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

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

Comment 13

15 years ago
Yes.

Comment 14

15 years ago
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?
Attachment #100025 - Flags: review+

Comment 15

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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

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

Updated

15 years ago
Priority: -- → P1

Updated

15 years ago
Severity: normal → blocker

Comment 20

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

Comment 21

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

Comment 22

15 years ago
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

15 years ago
I opened bug 171969 about the final matter.  We should close this bug.
(Assignee)

Comment 24

15 years ago
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Whiteboard: [3.4.3]

Comment 25

15 years ago
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.
Attachment #100574 - Attachment is obsolete: true

Updated

15 years ago
Attachment #100025 - Attachment description: Store trust in internal token if trust in target token fails. → Store trust in internal token if trust in target token fails. (for the tip of NSS)

Updated

15 years ago
Attachment #97968 - Attachment description: Handle Trust Change corner cases. → Handle Trust Change corner cases. (for NSS_3_4_BRANCH)

Updated

15 years ago
Attachment #97976 - Attachment description: Incremental patch → Incremental patch (for NSS_3_4_BRANCH)
You need to log in before you can comment on or make changes to this bug.