1berry/ruby
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[ruby/openssl] ssl: disallow reading/writing to unstarted SSL socket

Browse Source 
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

https://github.com/ruby/openssl/commit/bf780748b3
This commit is contained in:
Kazuki Yamaguchi 2021-10-25 00:09:24 +09:00
parent f0226f9a01
commit 1ac7f23bb8
2 changed files with 102 additions and 188 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

227
ext/openssl/ossl_ssl.c
View File

@ -1704,8 +1704,7 @@ ossl_start_ssl(VALUE self, int (*func)(), const char *funcname, VALUE opts)
* call-seq: * call-seq:
* ssl.connect => self * ssl.connect => self
* *
* Initiates an SSL/TLS handshake with a server. The handshake may be started * Initiates an SSL/TLS handshake with a server.
* after unencrypted data has been sent over the socket.
*/ */
static VALUE static VALUE
ossl_ssl_connect(VALUE self) ossl_ssl_connect(VALUE self)
@ -1752,8 +1751,7 @@ ossl_ssl_connect_nonblock(int argc, VALUE *argv, VALUE self)
* call-seq: * call-seq:
* ssl.accept => self * ssl.accept => self
* *
* Waits for a SSL/TLS client to initiate a handshake. The handshake may be * Waits for a SSL/TLS client to initiate a handshake.
* started after unencrypted data has been sent over the socket.
*/ */
static VALUE static VALUE
ossl_ssl_accept(VALUE self) ossl_ssl_accept(VALUE self)
@ -1800,7 +1798,7 @@ static VALUE
ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock)
{ {
SSL *ssl; SSL *ssl;
int ilen, nread = 0; int ilen;
VALUE len, str; VALUE len, str;
rb_io_t *fptr; rb_io_t *fptr;
VALUE io, opts = Qnil; VALUE io, opts = Qnil;
@ -1810,6 +1808,9 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock)
} else { } else {
rb_scan_args(argc, argv, "11", &len, &str); 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); ilen = NUM2INT(len);
if (NIL_P(str)) if (NIL_P(str))
@ -1825,85 +1826,60 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock)
if (ilen == 0) if (ilen == 0)
return str; return str;
GetSSL(self, ssl);
io = rb_attr_get(self, id_i_io); io = rb_attr_get(self, id_i_io);
GetOpenFile(io, fptr); GetOpenFile(io, fptr);
if (ssl_started(ssl)) {
rb_str_locktmp(str); rb_str_locktmp(str);
for (;;) { for (;;) {
nread = SSL_read(ssl, RSTRING_PTR(str), ilen); int nread = SSL_read(ssl, RSTRING_PTR(str), ilen);
switch(ssl_get_error(ssl, nread)){ switch (ssl_get_error(ssl, nread)) {
case SSL_ERROR_NONE: 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); rb_str_unlocktmp(str);
goto end; if (no_exception_p(opts)) { return sym_wait_writable; }
case SSL_ERROR_ZERO_RETURN: write_would_block(nonblock);
}
io_wait_writable(fptr);
continue;
case SSL_ERROR_WANT_READ:
if (nonblock) {
rb_str_unlocktmp(str); rb_str_unlocktmp(str);
if (no_exception_p(opts)) { return Qnil; } if (no_exception_p(opts)) { return sym_wait_readable; }
rb_eof_error(); read_would_block(nonblock);
case SSL_ERROR_WANT_WRITE: }
if (nonblock) { io_wait_readable(fptr);
rb_str_unlocktmp(str); continue;
if (no_exception_p(opts)) { return sym_wait_writable; } case SSL_ERROR_SYSCALL:
write_would_block(nonblock); 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();
} }
io_wait_writable(fptr); }
continue; /* fall through */
case SSL_ERROR_WANT_READ: default:
if (nonblock) { rb_str_unlocktmp(str);
rb_str_unlocktmp(str); ossl_raise(eSSLError, "SSL_read");
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:
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);
}
#endif
else
return rb_funcall(io, meth, 2, len, str);
}
end:
rb_str_set_len(str, nread);
return str;
} }
/* /*
@ -1943,78 +1919,55 @@ static VALUE
ossl_ssl_write_internal(VALUE self, VALUE str, VALUE opts) ossl_ssl_write_internal(VALUE self, VALUE str, VALUE opts)
{ {
SSL *ssl; SSL *ssl;
int nwrite = 0;
rb_io_t *fptr; rb_io_t *fptr;
int nonblock = opts != Qfalse; int num, nonblock = opts != Qfalse;
VALUE tmp, io; VALUE tmp, io;
tmp = rb_str_new_frozen(StringValue(str));
GetSSL(self, ssl); 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); io = rb_attr_get(self, id_i_io);
GetOpenFile(io, fptr); GetOpenFile(io, fptr);
if (ssl_started(ssl)) {
for (;;) {
int num = RSTRING_LENINT(tmp);
/* SSL_write(3ssl) manpage states num == 0 is undefined */ /* SSL_write(3ssl) manpage states num == 0 is undefined */
if (num == 0) num = RSTRING_LENINT(tmp);
goto end; if (num == 0)
return INT2FIX(0);
nwrite = SSL_write(ssl, RSTRING_PTR(tmp), num); for (;;) {
switch(ssl_get_error(ssl, nwrite)){ int nwritten = SSL_write(ssl, RSTRING_PTR(tmp), num);
case SSL_ERROR_NONE: switch (ssl_get_error(ssl, nwritten)) {
goto end; case SSL_ERROR_NONE:
case SSL_ERROR_WANT_WRITE: return INT2NUM(nwritten);
if (no_exception_p(opts)) { return sym_wait_writable; } case SSL_ERROR_WANT_WRITE:
write_would_block(nonblock); if (no_exception_p(opts)) { return sym_wait_writable; }
io_wait_writable(fptr); write_would_block(nonblock);
continue; io_wait_writable(fptr);
case SSL_ERROR_WANT_READ: continue;
if (no_exception_p(opts)) { return sym_wait_readable; } case SSL_ERROR_WANT_READ:
read_would_block(nonblock); if (no_exception_p(opts)) { return sym_wait_readable; }
io_wait_readable(fptr); read_would_block(nonblock);
continue; io_wait_readable(fptr);
case SSL_ERROR_SYSCALL: continue;
case SSL_ERROR_SYSCALL:
#ifdef __APPLE__ #ifdef __APPLE__
/* /*
* It appears that send syscall can return EPROTOTYPE if the * It appears that send syscall can return EPROTOTYPE if the
* socket is being torn down. Retry to get a proper errno to * socket is being torn down. Retry to get a proper errno to
* make the error handling in line with the socket library. * make the error handling in line with the socket library.
* [Bug #14713] https://bugs.ruby-lang.org/issues/14713 * [Bug #14713] https://bugs.ruby-lang.org/issues/14713
*/ */
if (errno == EPROTOTYPE) if (errno == EPROTOTYPE)
continue; continue;
#endif #endif
if (errno) rb_sys_fail(0); if (errno) rb_sys_fail(0);
/* fallthrough */ /* fallthrough */
default: default:
ossl_raise(eSSLError, "SSL_write"); 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);
} }
/* /*

63
test/openssl/test_ssl.rb
View File

@ -373,59 +373,20 @@ class OpenSSL::TestSSL < OpenSSL::SSLTestCase
} }
end end
def test_read_nonblock_without_session def test_unstarted_session
EnvUtil.suppress_warning do start_server do |port|
start_server(start_immediately: false) { |port| sock = TCPSocket.new("127.0.0.1", port)
sock = TCPSocket.new("127.0.0.1", port) ssl = OpenSSL::SSL::SSLSocket.new(sock)
ssl = OpenSSL::SSL::SSLSocket.new(sock)
ssl.sync_close = true
assert_equal :wait_readable, ssl.read_nonblock(100, exception: false) assert_raise(OpenSSL::SSL::SSLError) { ssl.syswrite("data") }
ssl.write("abc\n") assert_raise(OpenSSL::SSL::SSLError) { ssl.sysread(1) }
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 ssl.connect
server_proc = -> (ctx, ssl) { ssl.puts "abc"
while line = ssl.gets assert_equal "abc\n", ssl.gets
if line =~ /^STARTTLS$/ ensure
ssl.write("x") ssl&.close
ssl.flush sock&.close
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
ssl.puts("STARTTLS")
ssl.read(1)
ssl.connect
ssl.puts "over-tls"
assert_equal "over-tls\n", ssl.gets
ensure
ssl&.close
sock&.close
end
}
end end
end end