From 6c5e6b3ba0363ca496ea0b464edd1f2a235e8bf2 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Fri, 22 Sep 2017 18:05:20 +0900 Subject: [PATCH 1/2] test/test_pair: replace sleep with IO.select The sleep was to ensure that the SSLSocket#read_nonblock will get close_notify alert. A simple IO.select will suffice. --- test/test_pair.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_pair.rb b/test/test_pair.rb index 7daa92881..cbb985dda 100644 --- a/test/test_pair.rb +++ b/test/test_pair.rb @@ -218,7 +218,7 @@ def test_read_nonblock assert_nothing_raised("[ruby-core:20298]") { ret = s2.read_nonblock(10) } assert_equal("def\n", ret) s1.close - sleep 0.1 + IO.select([s2]) assert_raise(EOFError) { s2.read_nonblock(10) } } end @@ -234,7 +234,7 @@ def test_read_nonblock_no_exception assert_nothing_raised("[ruby-core:20298]") { ret = s2.read_nonblock(10, exception: false) } assert_equal("def\n", ret) s1.close - sleep 0.1 + IO.select([s2]) assert_equal(nil, s2.read_nonblock(10, exception: false)) } end From 6ff7844ea13ded27241fed9c641a20081b8ff402 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 23 Sep 2017 03:04:48 +0900 Subject: [PATCH 2/2] ssl: prevent SSLSocket#sysread* from leaking uninitialized data Set the length of the buffer string to 0 first, and adjust to the size successfully read by the SSL_read() call later. This is needed because the buffer string may be provided by the caller. --- ext/openssl/ossl_ssl.c | 22 +++++++++++++--------- test/test_pair.rb | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/ext/openssl/ossl_ssl.c b/ext/openssl/ossl_ssl.c index bf40c5b12..aa2dbbc8f 100644 --- a/ext/openssl/ossl_ssl.c +++ b/ext/openssl/ossl_ssl.c @@ -1688,20 +1688,26 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) } ilen = NUM2INT(len); - if(NIL_P(str)) str = rb_str_new(0, ilen); - else{ - StringValue(str); - rb_str_modify(str); - rb_str_resize(str, ilen); + if (NIL_P(str)) + str = rb_str_new(0, ilen); + else { + StringValue(str); + if (RSTRING_LEN(str) >= ilen) + rb_str_modify(str); + else + rb_str_modify_expand(str, ilen - RSTRING_LEN(str)); } - if(ilen == 0) return str; + OBJ_TAINT(str); + rb_str_set_len(str, 0); + if (ilen == 0) + return str; GetSSL(self, ssl); io = rb_attr_get(self, id_i_io); GetOpenFile(io, fptr); if (ssl_started(ssl)) { for (;;){ - nread = SSL_read(ssl, RSTRING_PTR(str), RSTRING_LENINT(str)); + nread = SSL_read(ssl, RSTRING_PTR(str), ilen); switch(ssl_get_error(ssl, nread)){ case SSL_ERROR_NONE: goto end; @@ -1751,8 +1757,6 @@ ossl_ssl_read_internal(int argc, VALUE *argv, VALUE self, int nonblock) end: rb_str_set_len(str, nread); - OBJ_TAINT(str); - return str; } diff --git a/test/test_pair.rb b/test/test_pair.rb index cbb985dda..ea5f0dcf9 100644 --- a/test/test_pair.rb +++ b/test/test_pair.rb @@ -239,6 +239,30 @@ def test_read_nonblock_no_exception } end + def test_read_with_outbuf + ssl_pair { |s1, s2| + s1.write("abc\n") + buf = "" + ret = s2.read(2, buf) + assert_same ret, buf + assert_equal "ab", ret + + buf = "garbage" + ret = s2.read(2, buf) + assert_same ret, buf + assert_equal "c\n", ret + + buf = "garbage" + assert_equal :wait_readable, s2.read_nonblock(100, buf, exception: false) + assert_equal "", buf + + s1.close + buf = "garbage" + assert_equal nil, s2.read(100, buf) + assert_equal "", buf + } + end + def test_write_nonblock ssl_pair {|s1, s2| assert_equal 3, s1.write_nonblock("foo")