Fix race condition when CRL checking is enabled.

CHANGES

@@ -725,6 +725,13 @@
 
  Changes between 0.9.7d and 0.9.7e  [XX xxx XXXX]
 
+  *) Avoid a race condition when CRLs are checked in a multi threaded 
+     environment. This would happen due to the reordering of the revoked
+     entries during signature checking and serial number lookup. Now the
+     encoding is cached and the serial number sort performed under a lock.
+     Add new STACK function sk_is_sorted().
+     [Steve Henson]
+
   *) Add Delta CRL to the extension code.
      [Steve Henson]
 

crypto/asn1/x_crl.c

@@ -63,8 +63,6 @@
 
 static int X509_REVOKED_cmp(const X509_REVOKED * const *a,
 				const X509_REVOKED * const *b);
-static int X509_REVOKED_seq_cmp(const X509_REVOKED * const *a,
-				const X509_REVOKED * const *b);
 
 ASN1_SEQUENCE(X509_REVOKED) = {
 	ASN1_SIMPLE(X509_REVOKED,serialNumber, ASN1_INTEGER),
@@ -72,43 +70,28 @@ ASN1_SEQUENCE(X509_REVOKED) = {
 	ASN1_SEQUENCE_OF_OPT(X509_REVOKED,extensions, X509_EXTENSION)
 } ASN1_SEQUENCE_END(X509_REVOKED)
 
-/* The X509_CRL_INFO structure needs a bit of customisation. This is actually
- * mirroring the old behaviour: its purpose is to allow the use of
- * sk_X509_REVOKED_find to lookup revoked certificates. Unfortunately
- * this will zap the original order and the signature so we keep a copy
- * of the original positions and reorder appropriately before encoding.
- *
- * Might want to see if there's a better way of doing this later...
+/* The X509_CRL_INFO structure needs a bit of customisation.
+ * Since we cache the original encoding the signature wont be affected by
+ * reordering of the revoked field.
  */
 static int crl_inf_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it)
 {
 	X509_CRL_INFO *a = (X509_CRL_INFO *)*pval;
-	int i;
-	int (*old_cmp)(const X509_REVOKED * const *,
-			const X509_REVOKED * const *);
 
 	if(!a || !a->revoked) return 1;
 	switch(operation) {
-
-		/* Save original order */
+		/* Just set cmp function here. We don't sort because that
+		 * would affect the output of X509_CRL_print().
+		 */
 		case ASN1_OP_D2I_POST:
-		for (i=0; i<sk_X509_REVOKED_num(a->revoked); i++)
-			sk_X509_REVOKED_value(a->revoked,i)->sequence=i;
 		sk_X509_REVOKED_set_cmp_func(a->revoked,X509_REVOKED_cmp);
 		break;
-
-		/* Restore original order */
-		case ASN1_OP_I2D_PRE:
-		old_cmp=sk_X509_REVOKED_set_cmp_func(a->revoked,X509_REVOKED_seq_cmp);
-		sk_X509_REVOKED_sort(a->revoked);
-		sk_X509_REVOKED_set_cmp_func(a->revoked,old_cmp);
-		break;
 	}
 	return 1;
 }
 
 
-ASN1_SEQUENCE_cb(X509_CRL_INFO, crl_inf_cb) = {
+ASN1_SEQUENCE_enc(X509_CRL_INFO, enc, crl_inf_cb) = {
 	ASN1_OPT(X509_CRL_INFO, version, ASN1_INTEGER),
 	ASN1_SIMPLE(X509_CRL_INFO, sig_alg, X509_ALGOR),
 	ASN1_SIMPLE(X509_CRL_INFO, issuer, X509_NAME),
@@ -116,7 +99,7 @@ ASN1_SEQUENCE_cb(X509_CRL_INFO, crl_inf_cb) = {
 	ASN1_OPT(X509_CRL_INFO, nextUpdate, ASN1_TIME),
 	ASN1_SEQUENCE_OF_OPT(X509_CRL_INFO, revoked, X509_REVOKED),
 	ASN1_EXP_SEQUENCE_OF_OPT(X509_CRL_INFO, extensions, X509_EXTENSION, 0)
-} ASN1_SEQUENCE_END_cb(X509_CRL_INFO, X509_CRL_INFO)
+} ASN1_SEQUENCE_END_enc(X509_CRL_INFO, X509_CRL_INFO)
 
 ASN1_SEQUENCE_ref(X509_CRL, 0, CRYPTO_LOCK_X509_CRL) = {
 	ASN1_SIMPLE(X509_CRL, crl, X509_CRL_INFO),
@@ -137,12 +120,6 @@ static int X509_REVOKED_cmp(const X509_REVOKED * const *a,
 		(ASN1_STRING *)(*b)->serialNumber));
 	}
 
-static int X509_REVOKED_seq_cmp(const X509_REVOKED * const *a,
-				const X509_REVOKED * const *b)
-	{
-	return((*a)->sequence-(*b)->sequence);
-	}
-
 int X509_CRL_add0_revoked(X509_CRL *crl, X509_REVOKED *rev)
 {
 	X509_CRL_INFO *inf;

crypto/stack/stack.c

@@ -332,3 +332,10 @@ void sk_sort(STACK *st)
 		st->sorted=1;
 		}
 	}
+
+int sk_is_sorted(const STACK *st)
+	{
+	if (st)
+		return 1;
+	return st->sorted;
+	}

crypto/stack/stack.h

@@ -100,6 +100,7 @@ int (*sk_set_cmp_func(STACK *sk, int (*c)(const char * const *,
 			(const char * const *, const char * const *);
 STACK *sk_dup(STACK *st);
 void sk_sort(STACK *st);
+int sk_is_sorted(const STACK *st);
 
 #ifdef  __cplusplus
 }

crypto/x509/x509.h

@@ -435,6 +435,7 @@ typedef struct X509_crl_info_st
 	ASN1_TIME *nextUpdate;
 	STACK_OF(X509_REVOKED) *revoked;
 	STACK_OF(X509_EXTENSION) /* [0] */ *extensions;
+	ASN1_ENCODING enc;
 	} X509_CRL_INFO;
 
 struct X509_crl_st

crypto/x509/x509_vfy.c

@@ -706,6 +706,15 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x)
 	X509_EXTENSION *ext;
 	/* Look for serial number of certificate in CRL */
 	rtmp.serialNumber = X509_get_serialNumber(x);
+	/* Sort revoked into serial number order if not already sorted.
+	 * Do this under a lock to avoid race condition.
+ 	 */
+	if (!sk_X509_REVOKED_is_sorted(crl->crl->revoked))
+		{
+		CRYPTO_w_lock(CRYPTO_LOCK_X509_CRL);
+		sk_X509_REVOKED_sort(crl->crl->revoked);
+		CRYPTO_w_unlock(CRYPTO_LOCK_X509_CRL);
+		}
 	idx = sk_X509_REVOKED_find(crl->crl->revoked, &rtmp);
 	/* If found assume revoked: want something cleverer than
 	 * this to handle entry extensions in V2 CRLs.

util/mkstack.pl

@@ -85,6 +85,7 @@ while(<IN>) {
 #define sk_${type_thing}_shift(st) SKM_sk_shift($type_thing, (st))
 #define sk_${type_thing}_pop(st) SKM_sk_pop($type_thing, (st))
 #define sk_${type_thing}_sort(st) SKM_sk_sort($type_thing, (st))
+#define sk_${type_thing}_is_sorted(st) SKM_sk_is_sorted($type_thing, (st))
 EOF
 	}
 	foreach $type_thing (sort @asn1setlst) {