Skip to content

Commit

Permalink
ssl: disallow reading/writing to unstarted SSL socket
Browse files Browse the repository at this point in the history 
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: #9
  • Loading branch information
@rhenium
rhenium committed Oct 24, 2021
1 parent 9b4f761 commit bf78074
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 190 deletions.
231 changes: 92 additions & 139 deletions ext/openssl/ossl_ssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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))
Expand All @@ -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;
}

/*
Expand Down Expand Up @@ -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);
}

/*
Expand Down
63 changes: 12 additions & 51 deletions test/openssl/test_ssl.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit bf78074

Please sign in to comment.