|
It's possible for multiple certificates to have the same subject name,
and if that happens then ssl3_output_cert_chain() may select the wrong
one because it just picks a certificate by name and doesn't actually
_check_ if it really is the right one.
There's a function which gets this right; X509_STORE_CTX_get1_issuer().
We should use it. dtls1_output_cert_chain() has the same problem.
We fix the check for self-signed certificates too, while we're at it.
Although that's much less likely to be a problem in practice.
Patch applies to 0.9.8 and HEAD.
Fix the
Index: ssl/d1_both.c
===================================================================
RCS file: /home/dwmw2/openssl-cvs/openssl/ssl/d1_both.c,v
retrieving revision 1.4.2.9
diff -u -p -r1.4.2.9 d1_both.c
--- ssl/d1_both.c 2 Apr 2009 22:32:15 -0000 1.4.2.9
+++ ssl/d1_both.c 31 May 2009 17:55:41 -0000
@@ -808,7 +808,6 @@ unsigned long dtls1_output_cert_chain(SS
unsigned long l= 3 + DTLS1_HM_HEADER_LENGTH;
BUF_MEM *buf;
X509_STORE_CTX xs_ctx;
- X509_OBJECT obj;
/* TLSv1 sends a chain with nothing in it, instead of an alert */
buf=s->init_buf;
@@ -827,6 +826,7 @@ unsigned long dtls1_output_cert_chain(SS
for (;;)
{
+ X509 *issuer;
n=i2d_X509(x,NULL);
if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
{
@@ -837,16 +837,18 @@ unsigned long dtls1_output_cert_chain(SS
l2n3(n,p);
i2d_X509(x,&p);
l+=n+3;
- if (X509_NAME_cmp(X509_get_subject_name(x),
- X509_get_issuer_name(x)) == 0) break;
- i=X509_STORE_get_by_subject(&xs_ctx,X509_LU_X509,
- X509_get_issuer_name(x),&obj);
+ /* self-signed */
+ if (xs_ctx.check_issued(&xs_ctx, x, x))
+ break;
+
+ i=X509_STORE_CTX_get1_issuer(&issuer, &xs_ctx, x);
if (i <= 0) break;
- x=obj.data.x509;
+
/* Count is one too high since the X509_STORE_get uped the
* ref count */
X509_free(x);
+ x = issuer;
}
X509_STORE_CTX_cleanup(&xs_ctx);
Index: ssl/s3_both.c
===================================================================
RCS file: /home/dwmw2/openssl-cvs/openssl/ssl/s3_both.c,v
retrieving revision 1.43
diff -u -p -r1.43 s3_both.c
--- ssl/s3_both.c 26 Apr 2005 16:02:39 -0000 1.43
+++ ssl/s3_both.c 31 May 2009 17:51:31 -0000
@@ -271,8 +271,6 @@ unsigned long ssl3_output_cert_chain(SSL
unsigned long l=7;
BUF_MEM *buf;
X509_STORE_CTX xs_ctx;
- X509_OBJECT obj;
-
int no_chain;
if ((s->mode & SSL_MODE_NO_AUTO_CHAIN) || s->ctx->extra_certs)
@@ -297,6 +295,7 @@ unsigned long ssl3_output_cert_chain(SSL
for (;;)
{
+ X509 *issuer;
n=i2d_X509(x,NULL);
if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
{
@@ -311,16 +310,17 @@ unsigned long ssl3_output_cert_chain(SSL
if (no_chain)
break;
- if (X509_NAME_cmp(X509_get_subject_name(x),
- X509_get_issuer_name(x)) == 0) break;
+ /* self-signed */
+ if (xs_ctx.check_issued(&xs_ctx, x, x))
+ break;
- i=X509_STORE_get_by_subject(&xs_ctx,X509_LU_X509,
- X509_get_issuer_name(x),&obj);
+ i=X509_STORE_CTX_get1_issuer(&issuer, &xs_ctx, x);
if (i <= 0) break;
- x=obj.data.x509;
+
/* Count is one too high since the X509_STORE_get uped the
* ref count */
X509_free(x);
+ x = issuer;
}
if (!no_chain)
X509_STORE_CTX_cleanup(&xs_ctx);
--
dwmw2
|
|
On Tue, 2009-06-02 at 13:40 +0200, Stephen Henson via RT wrote:
> If however we are going to revise this I'd say we should use
> X509_verify_cert to build the chain instead of more ad-hoc stuff.
This seems to work... only tested for SSLv3/TLSv1, not DTLS.
Index: ssl/d1_both.c
===================================================================
RCS file: /home/dwmw2/openssl-cvs/openssl/ssl/d1_both.c,v
retrieving revision 1.4.2.9
diff -u -p -r1.4.2.9 d1_both.c
--- ssl/d1_both.c 2 Apr 2009 22:32:15 -0000 1.4.2.9
+++ ssl/d1_both.c 2 Jun 2009 14:06:50 -0000
@@ -801,14 +801,30 @@ int dtls1_send_change_cipher_spec(SSL *s
return(dtls1_do_write(s,SSL3_RT_CHANGE_CIPHER_SPEC));
}
+static int dtls1_add_cert_to_buf(BUF_MEM *buf, unsigned long *l, X509 *x)
+ {
+ int n;
+ unsigned char *p;
+
+ n=i2d_X509(x,NULL);
+ if (!BUF_MEM_grow_clean(buf,(int)(n+(*l)+3)))
+ {
+ SSLerr(SSL_F_DTLS1_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
+ return(-1);
+ }
+ p=(unsigned char *)&(buf->data[*l]);
+ l2n3(n,p);
+ i2d_X509(x,&p);
+ *l+=n+3;
+
+ return(0);
+ }
unsigned long dtls1_output_cert_chain(SSL *s, X509 *x)
{
unsigned char *p;
- int n,i;
+ int i;
unsigned long l= 3 + DTLS1_HM_HEADER_LENGTH;
BUF_MEM *buf;
- X509_STORE_CTX xs_ctx;
- X509_OBJECT obj;
/* TLSv1 sends a chain with nothing in it, instead of an alert */
buf=s->init_buf;
@@ -819,54 +835,36 @@ unsigned long dtls1_output_cert_chain(SS
}
if (x != NULL)
{
- if(!X509_STORE_CTX_init(&xs_ctx,s->ctx->cert_store,NULL,NULL))
+ X509_STORE_CTX xs_ctx;
+
+ if (!X509_STORE_CTX_init(&xs_ctx,s->ctx->cert_store,NULL,NULL))
{
SSLerr(SSL_F_DTLS1_OUTPUT_CERT_CHAIN,ERR_R_X509_LIB);
return(0);
}
- for (;;)
+ xs_ctx.cert = x;
+ X509_verify_cert(&xs_ctx);
+ for (i=0; i < sk_X509_num(xs_ctx.chain); i++)
{
- n=i2d_X509(x,NULL);
- if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
+ x = sk_X509_value(xs_ctx.chain, i);
+
+ if (dtls1_add_cert_to_buf(buf, &l, x))
{
- SSLerr(SSL_F_DTLS1_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
- return(0);
+ X509_STORE_CTX_cleanup(&xs_ctx);
+ return 0;
}
- p=(unsigned char *)&(buf->data[l]);
- l2n3(n,p);
- i2d_X509(x,&p);
- l+=n+3;
- if (X509_NAME_cmp(X509_get_subject_name(x),
- X509_get_issuer_name(x)) == 0) break;
-
- i=X509_STORE_get_by_subject(&xs_ctx,X509_LU_X509,
- X509_get_issuer_name(x),&obj);
- if (i <= 0) break;
- x=obj.data.x509;
- /* Count is one too high since the X509_STORE_get uped the
- * ref count */
- X509_free(x);
}
-
X509_STORE_CTX_cleanup(&xs_ctx);
}
-
/* Thawte special :-) */
if (s->ctx->extra_certs != NULL)
for (i=0; i<sk_X509_num(s->ctx->extra_certs); i++)
{
x=sk_X509_value(s->ctx->extra_certs,i);
- n=i2d_X509(x,NULL);
- if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
- {
- SSLerr(SSL_F_DTLS1_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
+
+ if (dtls1_add_cert_to_buf(buf, &l, x))
return(0);
- }
- p=(unsigned char *)&(buf->data[l]);
- l2n3(n,p);
- i2d_X509(x,&p);
- l+=n+3;
}
l-= (3 + DTLS1_HM_HEADER_LENGTH);
Index: ssl/s3_both.c
===================================================================
RCS file: /home/dwmw2/openssl-cvs/openssl/ssl/s3_both.c,v
retrieving revision 1.43
diff -u -p -r1.43 s3_both.c
--- ssl/s3_both.c 26 Apr 2005 16:02:39 -0000 1.43
+++ ssl/s3_both.c 2 Jun 2009 14:07:13 -0000
@@ -264,15 +264,31 @@ int ssl3_send_change_cipher_spec(SSL *s,
return(ssl3_do_write(s,SSL3_RT_CHANGE_CIPHER_SPEC));
}
+static int ssl3_add_cert_to_buf(BUF_MEM *buf, unsigned long *l, X509 *x)
+ {
+ int n;
+ unsigned char *p;
+
+ n=i2d_X509(x,NULL);
+ if (!BUF_MEM_grow_clean(buf,(int)(n+(*l)+3)))
+ {
+ SSLerr(SSL_F_SSL3_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
+ return(-1);
+ }
+ p=(unsigned char *)&(buf->data[*l]);
+ l2n3(n,p);
+ i2d_X509(x,&p);
+ *l+=n+3;
+
+ return(0);
+ }
+
unsigned long ssl3_output_cert_chain(SSL *s, X509 *x)
{
unsigned char *p;
- int n,i;
+ int i;
unsigned long l=7;
BUF_MEM *buf;
- X509_STORE_CTX xs_ctx;
- X509_OBJECT obj;
-
int no_chain;
if ((s->mode & SSL_MODE_NO_AUTO_CHAIN) || s->ctx->extra_certs)
@@ -289,58 +305,45 @@ unsigned long ssl3_output_cert_chain(SSL
}
if (x != NULL)
{
- if(!no_chain && !X509_STORE_CTX_init(&xs_ctx,s->ctx->cert_store,NULL,NULL))
+ if (no_chain)
{
- SSLerr(SSL_F_SSL3_OUTPUT_CERT_CHAIN,ERR_R_X509_LIB);
- return(0);
+ if (ssl3_add_cert_to_buf(buf, &l, x))
+ return(0);
}
-
- for (;;)
+ else
{
- n=i2d_X509(x,NULL);
- if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
+ X509_STORE_CTX xs_ctx;
+
+ if (!X509_STORE_CTX_init(&xs_ctx,s->ctx->cert_store,NULL,NULL))
{
- SSLerr(SSL_F_SSL3_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
+ SSLerr(SSL_F_SSL3_OUTPUT_CERT_CHAIN,ERR_R_X509_LIB);
return(0);
}
- p=(unsigned char *)&(buf->data[l]);
- l2n3(n,p);
- i2d_X509(x,&p);
- l+=n+3;
-
- if (no_chain)
- break;
-
- if (X509_NAME_cmp(X509_get_subject_name(x),
- X509_get_issuer_name(x)) == 0) break;
-
- i=X509_STORE_get_by_subject(&xs_ctx,X509_LU_X509,
- X509_get_issuer_name(x),&obj);
- if (i <= 0) break;
- x=obj.data.x509;
- /* Count is one too high since the X509_STORE_get uped the
- * ref count */
- X509_free(x);
- }
- if (!no_chain)
+
+ xs_ctx.cert = x;
+ i = X509_verify_cert(&xs_ctx);
+ printf("X509_verify_cert() returned %d\n", i);
+ for (i=0; i < sk_X509_num(xs_ctx.chain); i++)
+ {
+ x = sk_X509_value(xs_ctx.chain, i);
+
+ if (ssl3_add_cert_to_buf(buf, &l, x))
+ {
+ X509_STORE_CTX_cleanup(&xs_ctx);
+ return 0;
+ }
+ }
X509_STORE_CTX_cleanup(&xs_ctx);
+ }
}
-
/* Thawte special :-) */
if (s->ctx->extra_certs != NULL)
for (i=0; i<sk_X509_num(s->ctx->extra_certs); i++)
{
x=sk_X509_value(s->ctx->extra_certs,i);
- n=i2d_X509(x,NULL);
- if (!BUF_MEM_grow_clean(buf,(int)(n+l+3)))
- {
- SSLerr(SSL_F_SSL3_OUTPUT_CERT_CHAIN,ERR_R_BUF_LIB);
+
+ if (ssl3_add_cert_to_buf(buf, &l, x))
return(0);
- }
- p=(unsigned char *)&(buf->data[l]);
- l2n3(n,p);
- i2d_X509(x,&p);
- l+=n+3;
}
l-=7;
--
dwmw2
|
