162976 - CRL updates need to be atomic in softoken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

When installing a CRL for an issuer in the cert database, if a CRL already
exists for the issuer in the cert database, we delete the old CRL first before
adding the new one. We need to make sure that this is all done in an atomic
fashion, so that there is no period of time in the middle of the update where
the application could query the CRL for the issuer and not find one.

I don't know if this is a problem currently or not as I haven't had time to look
at the database code, and I don't have a test case for it. The test would be
very timing sensitive and there is no way to automatically prove or disprove
this bug with one. Still, it could probably be written, with a very large CRL,
and running multiple threads at high priority querying the CRL in a loop shortly
before the update happens in another thread.

The application I have in mind that will depend on this atomic update is the CRL
cache.

Comment 1

[Julien Pierre]

This likely needs to be accomplished by locking of the database.

Comment 2

[Wan-Teh Chang]

Julien, have you already taken care of this with
the CRL cache?

Comment 3

[Julien Pierre]

No, not yet.

Comment 4

[Robert Relyea]

This is really 3.7

Comment 5

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 6

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 7

[Julien Pierre]

Actually, the problem is broader than just softoken.

The function crl_storeCrl in crl.c attempts to find an existing CRL in the
token, and to delete it, before trying to store the new one.

There is therefore a gap during which no CRL exists in the token.
I'm not sure how to resolve this.

The problem is made more complex by the fact that there are no strict rules on
CRL objects in PKCS#11 since they are our own extensions.

Bob, what if we just did the import a the higher level without doing the prior
deletion ? Would the token successfully "replace" the object ? Or would it
instead add the object, thus having two copies ? What is the standard PKCS#11
behavior when trying this with other objects (eg. certs) ?

Perhaps the CRL is a special case because we only ever store one per issuer,
whereas for certs we may store multiple certs under the same subject (eg.
expired certs).

Comment 8

[Robert Relyea]

It really depends on the object and implementation. In theory when you create a
new object, you create a new object. both will live in the system at the same
time. In our module (and in some other vendor's modules), however, it's not
always possible to have two objects with slightly different attributes.... so
from an application point of view, the semantics of creating two objects with
the same indexes and different values is undefined.

It may be possible to look up the object and do a change attribute on it. This
type of operation is defined on an object by object basis. Most objects do not
allow you  to change the Value.

The other thing, which is probably better, would be to create the new crl first.
In most tokens that would have both CRL's in the system at the same time. Then
we would delete the old CRL. In our system creating the new CRL would
effectively over write the old on. In both cases the update would be atomic.

bob

Comment 9

[Julien Pierre]

Bob,

You described what I would like to do - create the new CRL first, before
deleting the old one.
However, it seems to me that with our token, the new object will have the same
PKCS#11 token object ID as the old one. So when we go to the deletion step,
won't we be deleting the object we just created ?
Should we just check if the PKCS#11 token object ID has changed and not do the
deletion if it hasn't ?

Comment 10

[Robert Relyea]

Creating a new one will create a new objectID.

bob

Comment 11

[Robert Relyea]

Attachment: Make CRL update atomic.

This patch implements the last proposal I made in comment #8 .
I've tested it against all.sh, but not against any CRL test suites.

Comment 12

[Julien Pierre]

Unfortunately, there is no CRL test suite yet. I tested crlutil from the
command-line and verified that additions and deletions still worked with the
patch. This doesn't exercise all the cases, though. We want those operations to
happen within the same application. I'm going to try with PSM next and see what
happens with my certs after a couple of CRL updates.

Full testing would require the following sort of program :

Checking a cert's validity in a loop, preferably from multiple threads, and run
on a multiprocessor machine. At some point, a background thread would update the
CRL in the softoken . The validity test should keep the same result throughout
and should be repeated both for a revoked cert and a valid cert. In particular
there should be no change in the validity status of the cert at the time the CRL
is updated.

I already have a patch for certutil that makes it act as a verifying program in
a loop with threads. I would need to modify it to add another thread to update
the CRL.

Ideally, we would want to have a separate thread do the CRL updates in the token
in a loop, but we need a truly different CRL each time we do an update,
otherwise nothing will actually happen underneath. Unfortunately, we don't have
a way to programmatically generate CRLs in NSS at this time.

What we could do is grab a bunch of CRLs from our internal CA which regenerates
the CRL every half hour, and check them in as data files for this test.

Comment 13

[Robert Relyea]

Checked in

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.33; previous revision: 1.32
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.77; previous revision: 1.76
done
Checking in pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.25; previous revision: 1.24
done
Checking in pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.46; previous revision: 1.45
done