git:andrei/tcp_tls_changes: tls: fix wrong wbio usage - sr-dev

4 Jun 2010

Module: sip-router
Branch: andrei/tcp_tls_changes
Commit: 01c803a6c081ab84ca0b4faf9d16575b23de1b20
URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=01c803a6...
Author: Andrei Pelinescu-Onciul andrei@iptel.org
Committer: Andrei Pelinescu-Onciul andrei@iptel.org
Date:   Fri Jun  4 18:30:48 2010 +0200
tls: fix wrong wbio usage
The openssl library sometimes (after write operations) sets a
buffering bio "over" the wbio that we set. In this case one can no
longer rely on the wbio returned by SSL_get_wbio() (it might be the
buffering bio).
Now the BIOs are set at connection initialization time (and not on
their first use) and are stored inside the tls_extra_data
structure attached to the tcp connection. This way we are sure we
are always controlling our wbio and not something that openssl
might have stacked on top of it.
---
modules/tls/tls_bio.c    |    5 +++--
 modules/tls/tls_server.c |   45 ++++++++++++++++++---------------------------
 modules/tls/tls_server.h |    3 +++
 3 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/modules/tls/tls_bio.c b/modules/tls/tls_bio.c
index c82fe64..3581ef0 100644
--- a/modules/tls/tls_bio.c
+++ b/modules/tls/tls_bio.c
@@ -115,7 +115,7 @@ int tls_BIO_mbuf_set(BIO* b, struct tls_mbuf* rd, struct tls_mbuf* wr)
 {
    struct tls_bio_mbuf_data* d;
    
-	TLS_BIO_DBG("tls_BIO_muf_set called (%p, %p)\n", rd, wr);
+	TLS_BIO_DBG("tls_BIO_mbuf_set called (%p => %p, %p)\n", b, rd, wr);
    if (unlikely(b->ptr == 0)){
    	BUG("null BIO ptr\n");
    	return 0;
@@ -195,7 +195,8 @@ static int tls_bio_mbuf_read(BIO* b, char* dst, int dst_len)
    			   as a shortcut when no data is available =>
    			   simulate EAGIAN/WANT_READ */
    			TLS_BIO_DBG("read (%p, %p, %d) called with null read buffer"
-						" => simulating EAGAIN/WANT_READ\n", b, dst, dst_len);
+						"(%p->%p) => simulating EAGAIN/WANT_READ\n",
+						b, dst, dst_len, d, d->rd);
    			BIO_set_retry_read(b);
    		}
    		return -1;
diff --git a/modules/tls/tls_server.c b/modules/tls/tls_server.c
index c5c36de..981cf41 100644
--- a/modules/tls/tls_server.c
+++ b/modules/tls/tls_server.c
@@ -126,11 +126,16 @@ static int tls_complete_init(struct tcp_connection* c)
    }
    memset(data, '\0', sizeof(struct tls_extra_data));
    data->ssl = SSL_new(dom->ctx[process_no]);
+	data->rwbio = tls_BIO_new_mbuf(0, 0);
    data->cfg = cfg;
    data->state = state;
-	if (data->ssl == 0) {
-		TLS_ERR("Failed to create SSL structure:");
+	if (unlikely(data->ssl == 0 || data->rwbio == 0)) {
+		TLS_ERR("Failed to create SSL or BIO structure:");
+		if (data->ssl)
+			SSL_free(data->ssl);
+		if (data->rwbio)
+			BIO_free(data->rwbio);
    	goto error;
    }
 #ifdef TLS_KSSL_WORKARROUND
@@ -140,6 +145,7 @@ static int tls_complete_init(struct tcp_connection* c)
    	data->ssl->kssl_ctx=0;
    }
 #endif
+	SSL_set_bio(data->ssl, data->rwbio, data->rwbio);
    c->extra_data = data;
    return 0;
@@ -182,25 +188,9 @@ static int tls_set_mbufs(struct tcp_connection *c,
    						struct tls_mbuf* rd,
    						struct tls_mbuf* wr)
 {
-	SSL *ssl;
    BIO *rwbio;
    
-	/* if (unlikely(tls_fix_connection(c) < 0))
-		return -1;
-	*/
-	
-	ssl = ((struct tls_extra_data*)c->extra_data)->ssl;
-	if (unlikely(((rwbio=SSL_get_rbio(ssl))==0) ||
-					((rwbio=SSL_get_wbio(ssl))==0))) {
-		rwbio = tls_BIO_new_mbuf(rd, wr);
-		if (unlikely(rwbio == 0)) {
-			ERR("new mbuf BIO creation failure\n");
-			return -1;
-		}
-		/* use the same bio for both read & write */
-		SSL_set_bio(ssl, rwbio, rwbio);
-		return 0;
-	}
+	rwbio = ((struct tls_extra_data*)c->extra_data)->rwbio;
    if (unlikely(tls_BIO_mbuf_set(rwbio, rd, wr)<=0)) {
    	/* it should be always 1 */
    	ERR("failed to set mbufs");
@@ -875,7 +865,7 @@ redo_read:
    */
    if (unlikely(tls_c->enc_rd_buf)) {
    	/* use queued data */
-		/* safe to use without locks, because only read changes it and 
+		/* safe to use without locks, because only read changes it and
    	   there can't be parallel reads on the same connection */
    	enc_rd_buf = tls_c->enc_rd_buf;
    	tls_c->enc_rd_buf = 0;
@@ -980,8 +970,8 @@ ssl_read_skipped:
    			goto error_send;
    		}
    	}
-		/* quickly catch bugs: segfault if accessed and not set */
-		tls_set_mbufs(c, 0, 0);
+	/* quickly catch bugs: segfault if accessed and not set */
+	tls_set_mbufs(c, 0, 0);
    lock_release(&c->write_lock);
    switch(ssl_error) {
    	case SSL_ERROR_NONE:
@@ -1027,11 +1017,12 @@ ssl_read_skipped:
    	if (unlikely(n < 0))
    		/* here n should always be >= 0 */
    		BUG("unexpected value (n = %d)\n", n);
-		else if (unlikely(n < bytes_free))
-			BUG("read buffer not exhausted (rbio still has %d bytes,"
-					"last SSL_read %d / %d)\n",
-					rd.used - rd.pos, n, bytes_free);
-		else if (n == bytes_free) {
+		else {
+			if (unlikely(n < bytes_free))
+				BUG("read buffer not exhausted (rbio still has %d bytes,"
+						"last SSL_read %d / %d)\n",
+						rd.used - rd.pos, n, bytes_free);
+			/* n <= bytes_free */
    		/*  queue read data if not fully consumed by SSL_read()
    		 * (very unlikely situation)
    		 */
diff --git a/modules/tls/tls_server.h b/modules/tls/tls_server.h
index 66eb962..2a324bd 100644
--- a/modules/tls/tls_server.h
+++ b/modules/tls/tls_server.h
@@ -55,6 +55,9 @@ struct tls_rd_buf {
 struct tls_extra_data {
    tls_domains_cfg_t* cfg; /* Configuration used for this connection */
    SSL* ssl;               /* SSL context used for the connection */
+	BIO* rwbio;             /* bio used for read/write
+							   (openssl code might add buffering BIOs so
+							    it's better to remember our original BIO) */
    tls_ct_q* ct_wq;
    struct tls_rd_buf* enc_rd_buf;
    unsigned int flags;

    