From bf780748b30f3f2498d0791a3605bf4946b6def1 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Mon, 25 Oct 2021 00:09:24 +0900 Subject: [PATCH] ssl: disallow reading/writing to unstarted SSL socket OpenSSL::SSL::SSLSocket allowed #read and #write to be called before an SSL/TLS handshake is completed. They passed unencrypted data to the underlying socket. This behavior is very odd to have in this library. A verbose mode warning "SSL session is not started yet" was emitted whenever this happened. It also didn't behave well with OpenSSL::Buffering. Let's just get rid of it. Fixes: https://github.com/ruby/openssl/issues/9 --- ext/openssl/ossl_ssl.c | 231 ++++++++++++++++----------------------- test/openssl/test_ssl.rb | 63 ++--------- 2 files changed, 104 insertions(+), 190 deletions(-) diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c index 1de0f9892..8f68b5d42 100644 --- a/ext/openssl/ossl_ssl.c +++ b/ext/openssl/ossl_ssl.c @@ -1697,8 +1697,7 @@ ossl_start_ssl(VALUE self, int (*func)(), const char *funcname, VALUE opts) * call-seq: * ssl.connect => self * - * Initiates an SSL/TLS handshake with a server. The handshake may be started - * after unencrypted data has been sent over the socket. + * Initiates an SSL/TLS handshake with a server. */ static VALUE ossl_ssl_connect(VALUE self) @@ -1745,8 +1744,7 @@ ossl_ssl_connect_nonblock(int argc, VALUE *argv, VALUE self) * call-seq: * ssl.accept => self * - * Waits for a SSL/TLS client to initiate a handshake. The handshake may be - * started after unencrypted data has been sent over the socket. + * Waits for a SSL/TLS client to initiate a handshake. */ static VALUE ossl_ssl_accept(VALUE self) @@ -1793,7 +1791,7 @@ static VALUE ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) { SSL *ssl; - int ilen, nread = 0; + int ilen; VALUE len, str; rb_io_t *fptr; VALUE io, opts = Qnil; @@ -1803,6 +1801,9 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) } else { rb_scan_args(argc, argv, "11", &len, &str); } + GetSSL(self, ssl); + if (!ssl_started(ssl)) + rb_raise(eSSLError, "SSL session is not started yet"); ilen = NUM2INT(len); if (NIL_P(str)) @@ -1818,85 +1819,60 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) if (ilen == 0) return str; - GetSSL(self, ssl); io = rb_attr_get(self, id_i_io); GetOpenFile(io, fptr); - if (ssl_started(ssl)) { - rb_str_locktmp(str); - for (;;) { - nread = SSL_read(ssl, RSTRING_PTR(str), ilen); - switch(ssl_get_error(ssl, nread)){ - case SSL_ERROR_NONE: + + rb_str_locktmp(str); + for (;;) { + int nread = SSL_read(ssl, RSTRING_PTR(str), ilen); + switch (ssl_get_error(ssl, nread)) { + case SSL_ERROR_NONE: + rb_str_unlocktmp(str); + rb_str_set_len(str, nread); + return str; + case SSL_ERROR_ZERO_RETURN: + rb_str_unlocktmp(str); + if (no_exception_p(opts)) { return Qnil; } + rb_eof_error(); + case SSL_ERROR_WANT_WRITE: + if (nonblock) { rb_str_unlocktmp(str); - goto end; - case SSL_ERROR_ZERO_RETURN: + if (no_exception_p(opts)) { return sym_wait_writable; } + write_would_block(nonblock); + } + io_wait_writable(fptr); + continue; + case SSL_ERROR_WANT_READ: + if (nonblock) { rb_str_unlocktmp(str); - if (no_exception_p(opts)) { return Qnil; } - rb_eof_error(); - case SSL_ERROR_WANT_WRITE: - if (nonblock) { - rb_str_unlocktmp(str); - if (no_exception_p(opts)) { return sym_wait_writable; } - write_would_block(nonblock); - } - io_wait_writable(fptr); - continue; - case SSL_ERROR_WANT_READ: - if (nonblock) { - rb_str_unlocktmp(str); - if (no_exception_p(opts)) { return sym_wait_readable; } - read_would_block(nonblock); - } - io_wait_readable(fptr); - continue; - case SSL_ERROR_SYSCALL: - if (!ERR_peek_error()) { - rb_str_unlocktmp(str); - if (errno) - rb_sys_fail(0); - else { - /* - * The underlying BIO returned 0. This is actually a - * protocol error. But unfortunately, not all - * implementations cleanly shutdown the TLS connection - * but just shutdown/close the TCP connection. So report - * EOF for now... - */ - if (no_exception_p(opts)) { return Qnil; } - rb_eof_error(); - } - } - /* fall through */ - default: + if (no_exception_p(opts)) { return sym_wait_readable; } + read_would_block(nonblock); + } + io_wait_readable(fptr); + continue; + case SSL_ERROR_SYSCALL: + if (!ERR_peek_error()) { rb_str_unlocktmp(str); - ossl_raise(eSSLError, "SSL_read"); - } - } - } - else { - ID meth = nonblock ? rb_intern("read_nonblock") : rb_intern("sysread"); - - rb_warning("SSL session is not started yet."); -#if defined(RB_PASS_KEYWORDS) - if (nonblock) { - VALUE argv[3]; - argv[0] = len; - argv[1] = str; - argv[2] = opts; - return rb_funcallv_kw(io, meth, 3, argv, RB_PASS_KEYWORDS); - } -#else - if (nonblock) { - return rb_funcall(io, meth, 3, len, str, opts); + if (errno) + rb_sys_fail(0); + else { + /* + * The underlying BIO returned 0. This is actually a + * protocol error. But unfortunately, not all + * implementations cleanly shutdown the TLS connection + * but just shutdown/close the TCP connection. So report + * EOF for now... + */ + if (no_exception_p(opts)) { return Qnil; } + rb_eof_error(); + } + } + /* fall through */ + default: + rb_str_unlocktmp(str); + ossl_raise(eSSLError, "SSL_read"); } -#endif - else - return rb_funcall(io, meth, 2, len, str); } - - end: - rb_str_set_len(str, nread); - return str; } /* @@ -1936,78 +1912,55 @@ static VALUE ossl_ssl_write_internal(VALUE self, VALUE str, VALUE opts) { SSL *ssl; - int nwrite = 0; rb_io_t *fptr; - int nonblock = opts != Qfalse; + int num, nonblock = opts != Qfalse; VALUE tmp, io; - tmp = rb_str_new_frozen(StringValue(str)); GetSSL(self, ssl); + if (!ssl_started(ssl)) + rb_raise(eSSLError, "SSL session is not started yet"); + + tmp = rb_str_new_frozen(StringValue(str)); io = rb_attr_get(self, id_i_io); GetOpenFile(io, fptr); - if (ssl_started(ssl)) { - for (;;) { - int num = RSTRING_LENINT(tmp); - - /* SSL_write(3ssl) manpage states num == 0 is undefined */ - if (num == 0) - goto end; - - nwrite = SSL_write(ssl, RSTRING_PTR(tmp), num); - switch(ssl_get_error(ssl, nwrite)){ - case SSL_ERROR_NONE: - goto end; - case SSL_ERROR_WANT_WRITE: - if (no_exception_p(opts)) { return sym_wait_writable; } - write_would_block(nonblock); - io_wait_writable(fptr); - continue; - case SSL_ERROR_WANT_READ: - if (no_exception_p(opts)) { return sym_wait_readable; } - read_would_block(nonblock); - io_wait_readable(fptr); - continue; - case SSL_ERROR_SYSCALL: + + /* SSL_write(3ssl) manpage states num == 0 is undefined */ + num = RSTRING_LENINT(tmp); + if (num == 0) + return INT2FIX(0); + + for (;;) { + int nwritten = SSL_write(ssl, RSTRING_PTR(tmp), num); + switch (ssl_get_error(ssl, nwritten)) { + case SSL_ERROR_NONE: + return INT2NUM(nwritten); + case SSL_ERROR_WANT_WRITE: + if (no_exception_p(opts)) { return sym_wait_writable; } + write_would_block(nonblock); + io_wait_writable(fptr); + continue; + case SSL_ERROR_WANT_READ: + if (no_exception_p(opts)) { return sym_wait_readable; } + read_would_block(nonblock); + io_wait_readable(fptr); + continue; + case SSL_ERROR_SYSCALL: #ifdef __APPLE__ - /* - * It appears that send syscall can return EPROTOTYPE if the - * socket is being torn down. Retry to get a proper errno to - * make the error handling in line with the socket library. - * [Bug #14713] https://bugs.ruby-lang.org/issues/14713 - */ - if (errno == EPROTOTYPE) - continue; + /* + * It appears that send syscall can return EPROTOTYPE if the + * socket is being torn down. Retry to get a proper errno to + * make the error handling in line with the socket library. + * [Bug #14713] https://bugs.ruby-lang.org/issues/14713 + */ + if (errno == EPROTOTYPE) + continue; #endif - if (errno) rb_sys_fail(0); - /* fallthrough */ - default: - ossl_raise(eSSLError, "SSL_write"); - } + if (errno) rb_sys_fail(0); + /* fallthrough */ + default: + ossl_raise(eSSLError, "SSL_write"); } } - else { - ID meth = nonblock ? - rb_intern("write_nonblock") : rb_intern("syswrite"); - - rb_warning("SSL session is not started yet."); -#if defined(RB_PASS_KEYWORDS) - if (nonblock) { - VALUE argv[2]; - argv[0] = str; - argv[1] = opts; - return rb_funcallv_kw(io, meth, 2, argv, RB_PASS_KEYWORDS); - } -#else - if (nonblock) { - return rb_funcall(io, meth, 2, str, opts); - } -#endif - else - return rb_funcall(io, meth, 1, str); - } - - end: - return INT2NUM(nwrite); } /* diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb index 65dbe7ac2..70c46a934 100644 --- a/test/openssl/test_ssl.rb +++ b/test/openssl/test_ssl.rb @@ -373,59 +373,20 @@ def test_client_ca } end - def test_read_nonblock_without_session - EnvUtil.suppress_warning do - start_server(start_immediately: false) { |port| - sock = TCPSocket.new("127.0.0.1", port) - ssl = OpenSSL::SSL::SSLSocket.new(sock) - ssl.sync_close = true - - assert_equal :wait_readable, ssl.read_nonblock(100, exception: false) - ssl.write("abc\n") - IO.select [ssl] - assert_equal('a', ssl.read_nonblock(1)) - assert_equal("bc\n", ssl.read_nonblock(100)) - assert_equal :wait_readable, ssl.read_nonblock(100, exception: false) - ssl.close - } - end - end - - def test_starttls - server_proc = -> (ctx, ssl) { - while line = ssl.gets - if line =~ /^STARTTLS$/ - ssl.write("x") - ssl.flush - ssl.accept - break - end - ssl.write(line) - end - readwrite_loop(ctx, ssl) - } - - EnvUtil.suppress_warning do # read/write on not started session - start_server(start_immediately: false, - server_proc: server_proc) { |port| - begin - sock = TCPSocket.new("127.0.0.1", port) - ssl = OpenSSL::SSL::SSLSocket.new(sock) - - ssl.puts "plaintext" - assert_equal "plaintext\n", ssl.gets + def test_unstarted_session + start_server do |port| + sock = TCPSocket.new("127.0.0.1", port) + ssl = OpenSSL::SSL::SSLSocket.new(sock) - ssl.puts("STARTTLS") - ssl.read(1) - ssl.connect + assert_raise(OpenSSL::SSL::SSLError) { ssl.syswrite("data") } + assert_raise(OpenSSL::SSL::SSLError) { ssl.sysread(1) } - ssl.puts "over-tls" - assert_equal "over-tls\n", ssl.gets - ensure - ssl&.close - sock&.close - end - } + ssl.connect + ssl.puts "abc" + assert_equal "abc\n", ssl.gets + ensure + ssl&.close + sock&.close end end