Store verify_result with sessions to avoid potential security hole.

diff --git a/CHANGES b/CHANGES
index 4ccab5772fda86bb3462333b83e78145ed8dd109..289342b9876486354449ae860b135e628d44bba8 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,17 @@
 
  Changes between 0.9.4 and 0.9.5  [xx XXX 1999]
 
+  *) For servers, store verify_result in SSL_SESSION data structure
+     (and add it to external session representation).
+     This is needed when client certificate verifications fails,
+     but an application-provided verification callback (set by
+     SSL_CTX_set_cert_verify_callback) allows accepting the session
+     anyway (i.e. leaves x509_store_ctx->error != X509_V_OK
+     but returns 1): When the session is reused, we have to set
+     ssl->verify_result to the appropriate error code to avoid
+     security holes.
+     [Bodo Moeller, problem pointed out by Lutz Jaenicke]
+
   *) Fix a bug in the new PKCS#7 code: it didn't consider the
      case in PKCS7_dataInit() where the signed PKCS7 structure
      didn't contain any existing data because it was being created.

diff --git a/crypto/x509/x509_vfy.h b/crypto/x509/x509_vfy.h
index ecfd4cf9edac31cc03faf5e12ca0543dab71804f..39fa056c1ae2872536f996e480f4e62b19b8acaa 100644 (file)
--- a/crypto/x509/x509_vfy.h
+++ b/crypto/x509/x509_vfy.h
@@ -234,6 +234,7 @@ struct x509_store_state_st      /* X509_STORE_CTX */
                X509_LOOKUP_ctrl((x),X509_L_ADD_DIR,(name),(long)(type),NULL)
 
 #define                X509_V_OK                                       0
+/* illegal error (for uninitialized values, to avoid X509_V_OK): 1 */
 
 #define                X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT            2
 #define                X509_V_ERR_UNABLE_TO_GET_CRL                    3

diff --git a/ssl/s2_clnt.c b/ssl/s2_clnt.c
index 1fe8bd627dbe2e12ef98104ac5c4944376a1236a..956c0280c99b24c0dde1632fec6d31726d3a1890 100644 (file)
--- a/ssl/s2_clnt.c
+++ b/ssl/s2_clnt.c
@@ -445,7 +445,7 @@ static int get_server_hello(SSL *s)
        CRYPTO_w_unlock(CRYPTO_LOCK_X509);
 #else
        s->session->peer = s->session->sess_cert->peer_key->x509;
-    /* peer_key->x509 has been set by ssl2_set_certificate. */
+       /* peer_key->x509 has been set by ssl2_set_certificate. */
        CRYPTO_add(&s->session->peer->references, 1, CRYPTO_LOCK_X509);
 #endif
 

diff --git a/ssl/s2_srvr.c b/ssl/s2_srvr.c
index 9aeedef55f62dc5188c47005beda1870ce0612d1..f8f1ba76d0d77220715f2adb4591b4e4f99667a8 100644 (file)
--- a/ssl/s2_srvr.c
+++ b/ssl/s2_srvr.c
@@ -921,6 +921,7 @@ static int request_certificate(SSL *s)
                                X509_free(s->session->peer);
                        s->session->peer=x509;
                        CRYPTO_add(&x509->references,1,CRYPTO_LOCK_X509);
+                       s->session->verify_result = s->verify_result;
                        ret=1;
                        goto end;
                        }

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 961a2ca10c241eb2af138d49df4309dfa86d9e3b..dd3b149a892d022a6e0ac32c73d7c17b7c5b0eb7 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1627,6 +1627,7 @@ static int ssl3_get_client_certificate(SSL *s)
        if (s->session->peer != NULL) /* This should not be needed */
                X509_free(s->session->peer);
        s->session->peer=sk_X509_shift(sk);
+       s->session->verify_result = s->verify_result;
 
        /* With the current implementation, sess_cert will always be NULL
         * when we arrive here. */

diff --git a/ssl/ssl.h b/ssl/ssl.h
index 3f7cdd10ddd647331bda23700c3e2d29c85cf6db..f888530625aa3539f9272d3b7f4c57d51c2ea63e 100644 (file)
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -215,7 +215,8 @@ typedef struct ssl_method_st
  *     Timeout [ 2 ] EXPLICIT  INTEGER,        -- optional Timeout ins seconds
  *     Peer [ 3 ] EXPLICIT     X509,           -- optional Peer Certificate
  *     Session_ID_context [ 4 ] EXPLICIT OCTET_STRING,   -- the Session ID context
- *     Compression [5] IMPLICIT ASN1_OBJECT    -- compression OID XXXXX
+ *     Verify_result [ 5 ] EXPLICIT INTEGER    -- X509_V_... code for `Peer'
+ *     Compression [6] IMPLICIT ASN1_OBJECT    -- compression OID XXXXX
  *     }
  * Look in ssl/ssl_asn1.c for more details
  * I'm using EXPLICIT tags so I can read the damn things using asn1parse :-).
@@ -249,6 +250,9 @@ typedef struct ssl_session_st
         * (the latter is not enough as sess_cert is not retained
         * in the external representation of sessions, see ssl_asn1.c). */
        X509 *peer;
+       /* when app_verify_callback accepts a session where the peer's certificate
+        * is not ok, we must remember the error for session reuse: */
+       long verify_result; /* only for servers */
 
        int references;
        long timeout;

diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index 0f6a0884e4afb6af36b956024ceeacd3aa5c27b2..d77b0f26dbd08f51e438b615a9629f545b2dbf4e 100644 (file)
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -60,6 +60,7 @@
 #include <stdlib.h>
 #include <openssl/asn1_mac.h>
 #include <openssl/objects.h>
+#include <openssl/x509.h>
 #include "ssl_locl.h"
 
 typedef struct ssl_session_asn1_st
@@ -73,14 +74,15 @@ typedef struct ssl_session_asn1_st
        ASN1_OCTET_STRING key_arg;
        ASN1_INTEGER time;
        ASN1_INTEGER timeout;
+       ASN1_INTEGER verify_result;
        } SSL_SESSION_ASN1;
 
 int i2d_SSL_SESSION(SSL_SESSION *in, unsigned char **pp)
        {
 #define LSIZE2 (sizeof(long)*2)
-       int v1=0,v2=0,v3=0,v4=0;
+       int v1=0,v2=0,v3=0,v4=0,v5=0;
        unsigned char buf[4],ibuf1[LSIZE2],ibuf2[LSIZE2];
-       unsigned char ibuf3[LSIZE2],ibuf4[LSIZE2];
+       unsigned char ibuf3[LSIZE2],ibuf4[LSIZE2],ibuf5[LSIZE2];
        long l;
        SSL_SESSION_ASN1 a;
        M_ASN1_I2D_vars(in);
@@ -156,6 +158,14 @@ int i2d_SSL_SESSION(SSL_SESSION *in, unsigned char **pp)
                ASN1_INTEGER_set(&(a.timeout),in->timeout);
                }
 
+       if (in->verify_result != X509_V_OK)
+               {
+               a.verify_result.length=LSIZE2;
+               a.verify_result.type=V_ASN1_INTEGER;
+               a.verify_result.data=ibuf5;
+               ASN1_INTEGER_set(&a.verify_result,in->verify_result);
+               }
+
        M_ASN1_I2D_len(&(a.version),            i2d_ASN1_INTEGER);
        M_ASN1_I2D_len(&(a.ssl_version),        i2d_ASN1_INTEGER);
        M_ASN1_I2D_len(&(a.cipher),             i2d_ASN1_OCTET_STRING);
@@ -170,6 +180,8 @@ int i2d_SSL_SESSION(SSL_SESSION *in, unsigned char **pp)
        if (in->peer != NULL)
                M_ASN1_I2D_len_EXP_opt(in->peer,i2d_X509,3,v3);
        M_ASN1_I2D_len_EXP_opt(&a.session_id_context,i2d_ASN1_OCTET_STRING,4,v4);
+       if (in->verify_result != X509_V_OK)
+               M_ASN1_I2D_len_EXP_opt(&(a.verify_result),i2d_ASN1_INTEGER,5,v5);
 
        M_ASN1_I2D_seq_total();
 
@@ -188,7 +200,8 @@ int i2d_SSL_SESSION(SSL_SESSION *in, unsigned char **pp)
                M_ASN1_I2D_put_EXP_opt(in->peer,i2d_X509,3,v3);
        M_ASN1_I2D_put_EXP_opt(&a.session_id_context,i2d_ASN1_OCTET_STRING,4,
                               v4);
-
+       if (in->verify_result != X509_V_OK)
+               M_ASN1_I2D_put_EXP_opt(&a.verify_result,i2d_ASN1_INTEGER,5,v5);
        M_ASN1_I2D_finish();
        }
 
@@ -322,6 +335,15 @@ SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, unsigned char **pp,
        else
            ret->sid_ctx_length=0;
 
+       ai.length=0;
+       M_ASN1_D2I_get_EXP_opt(aip,d2i_ASN1_INTEGER,5);
+       if (ai.data != NULL)
+               {
+               ret->verify_result=ASN1_INTEGER_get(aip);
+               Free(ai.data); ai.data=NULL; ai.length=0;
+               }
+       else
+               ret->verify_result=X509_V_OK;
+
        M_ASN1_D2I_Finish(a,SSL_SESSION_free,SSL_F_D2I_SSL_SESSION);
        }
-

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 4dddf627cd62e33eb68f068e53a1539c1257fd2a..57ee7eb3c518774c33ea3665dc5ebbba7d218b1e 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -112,6 +112,7 @@ SSL_SESSION *SSL_SESSION_new(void)
                }
        memset(ss,0,sizeof(SSL_SESSION));
 
+       ss->verify_result = 1; /* avoid 0 (= X509_V_OK) just in case */
        ss->references=1;
        ss->timeout=60*5+4; /* 5 minute timeout by default */
        ss->time=time(NULL);
@@ -190,6 +191,7 @@ int ssl_get_new_session(SSL *s, int session)
        ss->sid_ctx_length=s->sid_ctx_length;
        s->session=ss;
        ss->ssl_version=s->version;
+       ss->verify_result = X509_V_OK;
 
        return(1);
        }
@@ -320,6 +322,7 @@ int ssl_get_prev_session(SSL *s, unsigned char *session_id, int len)
        if (s->session != NULL)
                SSL_SESSION_free(s->session);
        s->session=ret;
+       s->verify_result = s->session->verify_result;
        return(1);
 
  err:

diff --git a/ssl/ssltest.c b/ssl/ssltest.c
index 7d3c31dfaef618e9de0472b4c2a4177f614165ff..2e373a8f379c40336139e1884736299f7b088456 100644 (file)
--- a/ssl/ssltest.c
+++ b/ssl/ssltest.c
@@ -398,6 +398,11 @@ bad:
                SSL_CTX_set_verify(c_ctx,SSL_VERIFY_PEER,
                        verify_callback);
                }
+       
+       {
+               int session_id_context = 0;
+               SSL_CTX_set_session_id_context(s_ctx, (void *)&session_id_context, sizeof session_id_context);
+       }
 
        c_ssl=SSL_new(c_ctx);
        s_ssl=SSL_new(s_ctx);