Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl: disallow reading/writing to unstarted SSL socket #469

Merged
merged 1 commit into from
Nov 1, 2021
Merged

ssl: disallow reading/writing to unstarted SSL socket #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 92 additions & 139 deletions ext/openssl/ossl_ssl.c
View file
Open in desktop
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
Open in desktop
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