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

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=01c803a… Author: Andrei Pelinescu-Onciul &lt;andrei(a)iptel.org&gt; Committer: Andrei Pelinescu-Onciul &lt;andrei(a)iptel.org&gt; 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;