Attached is an updated patch for CVS HEAD, plus a patch for the 1.0.2
branch.
Are you still accepting patches for 1.0.1?
Any chance of reviewing these patches soon?
Thanks.
On 19/06/12 21:15, Rob Stradling via RT wrote:
> The OCSP Stapling Callback function (s->ctx->tlsext_status_cb) is called
> during the parsing of the ClientHello message, before the server has
> decided which cipher to use. However, since the choice of cipher can
> influence which server certificate is sent, this means that the wrong
> OCSP Response may be sent in cases where multiple server certificates
> are configured.
>
> The attached patch against CVS HEAD makes the following changes:
> - Moves the s->ctx->tlsext_status_cb() call to just after the cipher
> has been chosen. This involves splitting ssl_check_clienthello_tlsext()
> into two functions: "early" and "late".
> - Updates SSL_get_certificate() so that it returns the server
> certificate that actually gets sent. (This is the function that Apache
> httpd's OCSP Stapling code calls in order to determine which OCSP
> Response to send).
>
> I've tested this patch successfully with an installation of httpd 2.4.2
> that has both an RSA cert and an ECC cert configured.
>
> If this patch is OK, I'd like to backport it to the OpenSSL 1.0.x branch
> as well.
>
--
Rob Stradling
Senior Research & Development Scientist
COMODO - Creating Trust Online
Index: ssl/s3_srvr.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/s3_srvr.c,v
retrieving revision 1.239
diff -u -r1.239 s3_srvr.c
--- ssl/s3_srvr.c 15 Aug 2012 15:15:05 -0000 1.239
+++ ssl/s3_srvr.c 7 Sep 2012 10:00:12 -0000
@@ -1432,6 +1432,16 @@
* s->tmp.new_cipher - the new cipher to use.
*/
+ /* Handles TLS extensions that we couldn't check earlier */
+ if (s->version >= SSL3_VERSION)
+ {
+ if (ssl_check_clienthello_tlsext_late(s) <= 0)
+ {
+
SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO,SSL_R_CLIENTHELLO_TLSEXT);
+ goto err;
+ }
+ }
+
if (ret < 0) ret=1;
if (0)
{
Index: ssl/ssl_lib.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/ssl_lib.c,v
retrieving revision 1.242
diff -u -r1.242 ssl_lib.c
--- ssl/ssl_lib.c 31 Aug 2012 11:18:54 -0000 1.242
+++ ssl/ssl_lib.c 7 Sep 2012 10:00:12 -0000
@@ -2336,7 +2336,7 @@
#endif
-static int ssl_get_server_cert_index(SSL *s)
+static int ssl_get_server_cert_index(const SSL *s)
{
int idx;
idx = ssl_cipher_get_cert_index(s->s3->tmp.new_cipher);
@@ -2347,7 +2347,7 @@
return idx;
}
-CERT_PKEY *ssl_get_server_send_pkey(SSL *s)
+CERT_PKEY *ssl_get_server_send_pkey(const SSL *s)
{
CERT *c;
int i;
@@ -2833,6 +2833,14 @@
/* Fix this function so that it takes an optional type parameter */
X509 *SSL_get_certificate(const SSL *s)
{
+ if (s->server)
+ {
+ CERT_PKEY *certpkey;
+ certpkey = ssl_get_server_send_pkey(s);
+ if (certpkey && certpkey->x509)
+ return certpkey->x509;
+ }
+
if (s->cert != NULL)
return(s->cert->key->x509);
else
Index: ssl/ssl_locl.h
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/ssl_locl.h,v
retrieving revision 1.155
diff -u -r1.155 ssl_locl.h
--- ssl/ssl_locl.h 31 Aug 2012 11:18:54 -0000 1.155
+++ ssl/ssl_locl.h 7 Sep 2012 10:00:12 -0000
@@ -934,7 +934,7 @@
int ssl_undefined_function(SSL *s);
int ssl_undefined_void_function(void);
int ssl_undefined_const_function(const SSL *s);
-CERT_PKEY *ssl_get_server_send_pkey(SSL *);
+CERT_PKEY *ssl_get_server_send_pkey(const SSL *s);
unsigned char *ssl_get_authz_data(SSL *s, size_t *authz_length);
EVP_PKEY *ssl_get_sign_pkey(SSL *s,const SSL_CIPHER *c, const EVP_MD **pmd);
int ssl_cert_type(X509 *x,EVP_PKEY *pkey);
@@ -1201,6 +1201,7 @@
unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned
char *limit);
unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned
char *limit);
int ssl_parse_clienthello_tlsext(SSL *s, unsigned char **data, unsigned char
*d, int n);
+int ssl_check_clienthello_tlsext_late(SSL *s);
int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **data, unsigned char
*d, int n);
int ssl_prepare_clienthello_tlsext(SSL *s);
int ssl_prepare_serverhello_tlsext(SSL *s);
Index: ssl/t1_lib.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/t1_lib.c,v
retrieving revision 1.139
diff -u -r1.139 t1_lib.c
--- ssl/t1_lib.c 29 Aug 2012 13:18:34 -0000 1.139
+++ ssl/t1_lib.c 7 Sep 2012 10:00:12 -0000
@@ -123,7 +123,7 @@
static int tls_decrypt_ticket(SSL *s, const unsigned char *tick, int ticklen,
const unsigned char *sess_id, int sesslen,
SSL_SESSION **psess);
-static int ssl_check_clienthello_tlsext(SSL *s);
+static int ssl_check_clienthello_tlsext_early(SSL *s);
int ssl_check_serverhello_tlsext(SSL *s);
#endif
@@ -2241,7 +2241,7 @@
return 0;
}
- if (ssl_check_clienthello_tlsext(s) <= 0)
+ if (ssl_check_clienthello_tlsext_early(s) <= 0)
{
SSLerr(SSL_F_SSL_PARSE_CLIENTHELLO_TLSEXT,SSL_R_CLIENTHELLO_TLSEXT);
return 0;
@@ -2642,7 +2642,7 @@
return 1;
}
-static int ssl_check_clienthello_tlsext(SSL *s)
+static int ssl_check_clienthello_tlsext_early(SSL *s)
{
int ret=SSL_TLSEXT_ERR_NOACK;
int al = SSL_AD_UNRECOGNIZED_NAME;
@@ -2661,42 +2661,11 @@
else if (s->initial_ctx != NULL &&
s->initial_ctx->tlsext_servername_callback != 0)
ret = s->initial_ctx->tlsext_servername_callback(s, &al,
s->initial_ctx->tlsext_servername_arg);
- /* If status request then ask callback what to do.
- * Note: this must be called after servername callbacks in case
- * the certificate has changed.
- */
- if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb)
- {
- int r;
- r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
- switch (r)
- {
- /* We don't want to send a status request response */
- case SSL_TLSEXT_ERR_NOACK:
- s->tlsext_status_expected = 0;
- break;
- /* status request response should be sent */
- case SSL_TLSEXT_ERR_OK:
- if (s->tlsext_ocsp_resp)
- s->tlsext_status_expected = 1;
- else
- s->tlsext_status_expected = 0;
- break;
- /* something bad happened */
- case SSL_TLSEXT_ERR_ALERT_FATAL:
- ret = SSL_TLSEXT_ERR_ALERT_FATAL;
- al = SSL_AD_INTERNAL_ERROR;
- goto err;
- }
- }
- else
- s->tlsext_status_expected = 0;
-
#ifdef TLSEXT_TYPE_opaque_prf_input
{
/* This sort of belongs into ssl_prepare_serverhello_tlsext(),
* but we might be sending an alert in response to the client
hello,
- * so this has to happen here in
ssl_check_clienthello_tlsext(). */
+ * so this has to happen here in
ssl_check_clienthello_tlsext_early(). */
int r = 1;
@@ -2748,8 +2717,8 @@
}
}
-#endif
err:
+#endif
switch (ret)
{
case SSL_TLSEXT_ERR_ALERT_FATAL:
@@ -2767,6 +2736,59 @@
}
}
+int ssl_check_clienthello_tlsext_late(SSL *s)
+ {
+ int ret=SSL_TLSEXT_ERR_OK;
+ int al;
+
+ /* If status request then ask callback what to do.
+ * Note: this must be called after servername callbacks in case
+ * the certificate has changed, and must be called after the cipher
+ * has been chosen because this may influence which certificate is sent
+ */
+ if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb)
+ {
+ int r;
+ r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
+ switch (r)
+ {
+ /* We don't want to send a status request response */
+ case SSL_TLSEXT_ERR_NOACK:
+ s->tlsext_status_expected = 0;
+ break;
+ /* status request response should be sent */
+ case SSL_TLSEXT_ERR_OK:
+ if (s->tlsext_ocsp_resp)
+ s->tlsext_status_expected = 1;
+ else
+ s->tlsext_status_expected = 0;
+ break;
+ /* something bad happened */
+ case SSL_TLSEXT_ERR_ALERT_FATAL:
+ ret = SSL_TLSEXT_ERR_ALERT_FATAL;
+ al = SSL_AD_INTERNAL_ERROR;
+ goto err;
+ }
+ }
+ else
+ s->tlsext_status_expected = 0;
+
+ err:
+ switch (ret)
+ {
+ case SSL_TLSEXT_ERR_ALERT_FATAL:
+ ssl3_send_alert(s,SSL3_AL_FATAL,al);
+ return -1;
+
+ case SSL_TLSEXT_ERR_ALERT_WARNING:
+ ssl3_send_alert(s,SSL3_AL_WARNING,al);
+ return 1;
+
+ default:
+ return 1;
+ }
+ }
+
int ssl_check_serverhello_tlsext(SSL *s)
{
int ret=SSL_TLSEXT_ERR_NOACK;
Index: ssl/s3_srvr.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/s3_srvr.c,v
retrieving revision 1.171.2.21.2.24.2.11
diff -u -r1.171.2.21.2.24.2.11 s3_srvr.c
--- ssl/s3_srvr.c 6 Jun 2012 12:45:14 -0000 1.171.2.21.2.24.2.11
+++ ssl/s3_srvr.c 19 Jul 2012 15:08:41 -0000
@@ -1418,6 +1418,16 @@
* s->tmp.new_cipher - the new cipher to use.
*/
+ /* Handles TLS extensions that we couldn't check earlier */
+ if (s->version >= SSL3_VERSION)
+ {
+ if (ssl_check_clienthello_tlsext_late(s) <= 0)
+ {
+
SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO,SSL_R_CLIENTHELLO_TLSEXT);
+ goto err;
+ }
+ }
+
if (ret < 0) ret=1;
if (0)
{
Index: ssl/ssl_lib.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/ssl_lib.c,v
retrieving revision 1.176.2.19.2.24.2.7
diff -u -r1.176.2.19.2.24.2.7 ssl_lib.c
--- ssl/ssl_lib.c 3 Jun 2012 22:03:31 -0000 1.176.2.19.2.24.2.7
+++ ssl/ssl_lib.c 19 Jul 2012 15:08:41 -0000
@@ -2320,7 +2320,7 @@
#endif
/* THIS NEEDS CLEANING UP */
-static int ssl_get_server_cert_index(SSL *s)
+static int ssl_get_server_cert_index(const SSL *s)
{
unsigned long alg_k, alg_a;
@@ -2371,7 +2371,7 @@
}
}
-CERT_PKEY *ssl_get_server_send_pkey(SSL *s)
+CERT_PKEY *ssl_get_server_send_pkey(const SSL *s)
{
CERT *c;
int i;
@@ -2840,6 +2840,14 @@
/* Fix this function so that it takes an optional type parameter */
X509 *SSL_get_certificate(const SSL *s)
{
+ if (s->server)
+ {
+ CERT_PKEY *certpkey;
+ certpkey = ssl_get_server_send_pkey(s);
+ if (certpkey && certpkey->x509)
+ return certpkey->x509;
+ }
+
if (s->cert != NULL)
return(s->cert->key->x509);
else
Index: ssl/ssl_locl.h
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/ssl_locl.h,v
retrieving revision 1.100.2.10.2.17.2.11
diff -u -r1.100.2.10.2.17.2.11 ssl_locl.h
--- ssl/ssl_locl.h 29 May 2012 17:27:48 -0000 1.100.2.10.2.17.2.11
+++ ssl/ssl_locl.h 19 Jul 2012 15:08:41 -0000
@@ -866,7 +866,7 @@
int ssl_undefined_function(SSL *s);
int ssl_undefined_void_function(void);
int ssl_undefined_const_function(const SSL *s);
-CERT_PKEY *ssl_get_server_send_pkey(SSL *);
+CERT_PKEY *ssl_get_server_send_pkey(const SSL *s);
unsigned char *ssl_get_authz_data(SSL *s, size_t *authz_length);
EVP_PKEY *ssl_get_sign_pkey(SSL *s,const SSL_CIPHER *c, const EVP_MD **pmd);
int ssl_cert_type(X509 *x,EVP_PKEY *pkey);
@@ -1133,6 +1133,7 @@
unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned
char *limit);
unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned
char *limit);
int ssl_parse_clienthello_tlsext(SSL *s, unsigned char **data, unsigned char
*d, int n);
+int ssl_check_clienthello_tlsext_late(SSL *s);
int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **data, unsigned char
*d, int n);
int ssl_prepare_clienthello_tlsext(SSL *s);
int ssl_prepare_serverhello_tlsext(SSL *s);
Index: ssl/t1_lib.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/t1_lib.c,v
retrieving revision 1.64.2.14.2.32.2.13
diff -u -r1.64.2.14.2.32.2.13 t1_lib.c
--- ssl/t1_lib.c 3 Jul 2012 20:21:35 -0000 1.64.2.14.2.32.2.13
+++ ssl/t1_lib.c 19 Jul 2012 15:08:41 -0000
@@ -123,7 +123,7 @@
static int tls_decrypt_ticket(SSL *s, const unsigned char *tick, int ticklen,
const unsigned char *sess_id, int sesslen,
SSL_SESSION **psess);
-static int ssl_check_clienthello_tlsext(SSL *s);
+static int ssl_check_clienthello_tlsext_early(SSL *s);
int ssl_check_serverhello_tlsext(SSL *s);
#endif
@@ -1846,7 +1846,7 @@
return 0;
}
- if (ssl_check_clienthello_tlsext(s) <= 0)
+ if (ssl_check_clienthello_tlsext_early(s) <= 0)
{
SSLerr(SSL_F_SSL_PARSE_CLIENTHELLO_TLSEXT,SSL_R_CLIENTHELLO_TLSEXT);
return 0;
@@ -2247,7 +2247,7 @@
return 1;
}
-static int ssl_check_clienthello_tlsext(SSL *s)
+static int ssl_check_clienthello_tlsext_early(SSL *s)
{
int ret=SSL_TLSEXT_ERR_NOACK;
int al = SSL_AD_UNRECOGNIZED_NAME;
@@ -2266,42 +2266,11 @@
else if (s->initial_ctx != NULL &&
s->initial_ctx->tlsext_servername_callback != 0)
ret = s->initial_ctx->tlsext_servername_callback(s, &al,
s->initial_ctx->tlsext_servername_arg);
- /* If status request then ask callback what to do.
- * Note: this must be called after servername callbacks in case
- * the certificate has changed.
- */
- if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb)
- {
- int r;
- r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
- switch (r)
- {
- /* We don't want to send a status request response */
- case SSL_TLSEXT_ERR_NOACK:
- s->tlsext_status_expected = 0;
- break;
- /* status request response should be sent */
- case SSL_TLSEXT_ERR_OK:
- if (s->tlsext_ocsp_resp)
- s->tlsext_status_expected = 1;
- else
- s->tlsext_status_expected = 0;
- break;
- /* something bad happened */
- case SSL_TLSEXT_ERR_ALERT_FATAL:
- ret = SSL_TLSEXT_ERR_ALERT_FATAL;
- al = SSL_AD_INTERNAL_ERROR;
- goto err;
- }
- }
- else
- s->tlsext_status_expected = 0;
-
#ifdef TLSEXT_TYPE_opaque_prf_input
{
/* This sort of belongs into ssl_prepare_serverhello_tlsext(),
* but we might be sending an alert in response to the client
hello,
- * so this has to happen here in
ssl_check_clienthello_tlsext(). */
+ * so this has to happen here in
ssl_check_clienthello_tlsext_early(). */
int r = 1;
@@ -2353,8 +2322,8 @@
}
}
-#endif
err:
+#endif
switch (ret)
{
case SSL_TLSEXT_ERR_ALERT_FATAL:
@@ -2372,6 +2341,59 @@
}
}
+int ssl_check_clienthello_tlsext_late(SSL *s)
+ {
+ int ret=SSL_TLSEXT_ERR_OK;
+ int al;
+
+ /* If status request then ask callback what to do.
+ * Note: this must be called after servername callbacks in case
+ * the certificate has changed, and must be called after the cipher
+ * has been chosen because this may influence which certificate is sent
+ */
+ if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb)
+ {
+ int r;
+ r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
+ switch (r)
+ {
+ /* We don't want to send a status request response */
+ case SSL_TLSEXT_ERR_NOACK:
+ s->tlsext_status_expected = 0;
+ break;
+ /* status request response should be sent */
+ case SSL_TLSEXT_ERR_OK:
+ if (s->tlsext_ocsp_resp)
+ s->tlsext_status_expected = 1;
+ else
+ s->tlsext_status_expected = 0;
+ break;
+ /* something bad happened */
+ case SSL_TLSEXT_ERR_ALERT_FATAL:
+ ret = SSL_TLSEXT_ERR_ALERT_FATAL;
+ al = SSL_AD_INTERNAL_ERROR;
+ goto err;
+ }
+ }
+ else
+ s->tlsext_status_expected = 0;
+
+ err:
+ switch (ret)
+ {
+ case SSL_TLSEXT_ERR_ALERT_FATAL:
+ ssl3_send_alert(s,SSL3_AL_FATAL,al);
+ return -1;
+
+ case SSL_TLSEXT_ERR_ALERT_WARNING:
+ ssl3_send_alert(s,SSL3_AL_WARNING,al);
+ return 1;
+
+ default:
+ return 1;
+ }
+ }
+
int ssl_check_serverhello_tlsext(SSL *s)
{
int ret=SSL_TLSEXT_ERR_NOACK;
