From e3a6ba4ac4616500124929eb377fca899c0db2b2 Mon Sep 17 00:00:00 2001 From: nick evans Date: Sun, 29 Mar 2026 12:26:48 -0400 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=8D=92=20pick=20257ede0df:=20?= =?UTF-8?q?=F0=9F=A5=85=20Re-raise=20`#starttls`=20error=20from=20receiver?= =?UTF-8?q?=20thread=20[backports=20#395]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backports #395 to `v0.3-stable`. The tests required an additional rescue-and-ignore for the server thread in `starttls_test`, which was already present in all later branches. --------- When `start_tls_session` raises an exception, that's caught in the receiver thread, but not re-raised. Fortunately, `@sock` will now be a permanently broken SSLSocket, so I don't think this can lead to accidentally using an insecure connection. Even so, `#starttls` should disconnect the socket and re-raise the error immediately. Failing test case was provided by @rhenium in #394. Co-authored-by: Kazuki Yamaguchi --- lib/net/imap.rb | 10 +++++++++- test/net/imap/test_imap.rb | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index b9f0a4629..01f066286 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1014,7 +1014,8 @@ def logout # unsolicited untagged response immeditely _after_ #starttls completes. # def starttls(options = {}, verify = true) - send_command("STARTTLS") do |resp| + error = nil + ok = send_command("STARTTLS") do |resp| if resp.kind_of?(TaggedResponse) && resp.name == "OK" begin # for backward compatibility @@ -1024,7 +1025,14 @@ def starttls(options = {}, verify = true) end start_tls_session(options) end + rescue Exception => error + raise # note that the error backtrace is in the receiver_thread end + if error + disconnect + raise error + end + ok end # :call-seq: diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 308c83da4..fc9513a6f 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -75,6 +75,20 @@ def test_imaps_post_connection_check end if defined?(OpenSSL::SSL) + def test_starttls_unknown_ca + imap = nil + ex = nil + starttls_test do |port| + imap = Net::IMAP.new("localhost", port: port) + begin + imap.starttls + rescue => ex + end + imap + end + assert_kind_of(OpenSSL::SSL::SSLError, ex) + end + def test_starttls imap = nil starttls_test do |port| @@ -1004,6 +1018,7 @@ def starttls_test sock.gets sock.print("* BYE terminating connection\r\n") sock.print("RUBY0002 OK LOGOUT completed\r\n") + rescue OpenSSL::SSL::SSLError ensure sock.close server.close From 2068d468e8b6d62ce9c68768cdfda978de99b046 Mon Sep 17 00:00:00 2001 From: nick evans Date: Tue, 14 Feb 2023 00:13:11 -0500 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=8D=92=20pick=20609acd9fa:=20?= =?UTF-8?q?=F0=9F=A5=85=20Add=20new=20`InvalidResponseError`=20exception?= =?UTF-8?q?=20class=20[backport=20#198]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This copies the InvalidResponseError from #198 and the rdoc update to UnknownResponseError. The behavioral changes from that PR have _not_ been copied. --- lib/net/imap/errors.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/net/imap/errors.rb b/lib/net/imap/errors.rb index 52cb936b4..4a222fef2 100644 --- a/lib/net/imap/errors.rb +++ b/lib/net/imap/errors.rb @@ -81,7 +81,27 @@ class BadResponseError < ResponseError class ByeResponseError < ResponseError end + # Error raised when the server sends an invalid response. + # + # This is different from UnknownResponseError: the response has been + # rejected. Although it may be parsable, the server is forbidden from + # sending it in the current context. The client should automatically + # disconnect, abruptly (without logout). + # + # Note that InvalidResponseError does not inherit from ResponseError: it + # can be raised before the response is fully parsed. A related + # ResponseParseError or ResponseError may be the #cause. + class InvalidResponseError < Error + end + # Error raised upon an unknown response from the server. + # + # This is different from InvalidResponseError: the response may be a + # valid extension response and the server may be allowed to send it in + # this context, but Net::IMAP either does not know how to parse it or + # how to handle it. This could result from enabling unknown or + # unhandled extensions. The connection may still be usable, + # but—depending on context—it may be prudent to disconnect. class UnknownResponseError < ResponseError end From f96ab6ca67fb54b0ca239efa44f1b8b76cafa105 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 17:16:25 -0400 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=8D=92=20pick=2046636cae8:=20?= =?UTF-8?q?=E2=9D=8C=F0=9F=94=92=20Add=20failing=20test=20for=20STARTTLS?= =?UTF-8?q?=20stripping=20[backport=20#664]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I'm putting this in its own commit to simplify testing across backports. Also, I'm taking a "belt-and-suspenders" approach, and I'm going to test that either of the two fixes passes the tests. --- test/net/imap/test_imap.rb | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index fc9513a6f..a299c4520 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -113,6 +113,40 @@ def test_starttls_stripping imap end end + + def test_starttls_stripping_ok_sent_before_response + # to coordinate between threads (better than sleep) + server_to_client, client_to_server = Queue.new, Queue.new + imap = nil + server = create_tcp_server + port = server.addr[1] + start_server do + sock = server.accept + begin + sock.print("* OK test server\r\n") + assert_equal :send_malicious_response, client_to_server.pop + sock.print("RUBY0001 OK hahaha, fooled you!\r\n") + server_to_client << :malicious_response_sent + sock.gets + ensure + sock.close + server.close + end + end + begin + imap = Net::IMAP.new("localhost", :port => port) + client_to_server << :send_malicious_response + assert_equal :malicious_response_sent, server_to_client.pop + sleep 0.010 # to be sure the network buffers have flushed, etc + assert_raise(Net::IMAP::InvalidResponseError) do + imap.starttls(:ca_file => CA_FILE) + end + assert imap.disconnected? + ensure + imap.disconnect if imap && !imap.disconnected? + end + assert imap.disconnected? + end end def start_server From d16e994cb552f9613703bec7becb00b7540452f9 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 17:31:11 -0400 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=8D=92=20pick=2062eea6ffe:=20?= =?UTF-8?q?=F0=9F=94=92=F0=9F=A5=85=20Ensure=20STARTTLS=20tagged=20respons?= =?UTF-8?q?e=20was=20handled=20[backport=20#664]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking a "belt-and-suspenders" approach to a STARTTLS stripping attack: This handles `STARTTLS` as a special-case: if the `STARTTLS` handler did not run, for _whatever_ reason, an exception _must_ be raised and the connection dropped. _No_ command should ever receive a tagged `OK` prior to completely sending the command. But `STARTTLS` is security-sensitive enough to warrant this special-case handler. --- lib/net/imap.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 01f066286..4d81d61cb 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1014,9 +1014,11 @@ def logout # unsolicited untagged response immeditely _after_ #starttls completes. # def starttls(options = {}, verify = true) + handled = false error = nil ok = send_command("STARTTLS") do |resp| if resp.kind_of?(TaggedResponse) && resp.name == "OK" + handled = true begin # for backward compatibility certs = options.to_str @@ -1032,6 +1034,13 @@ def starttls(options = {}, verify = true) disconnect raise error end + unless handled + disconnect + raise InvalidResponseError, + "STARTTLS handler was bypassed, although server responded %p" % [ + ok.raw_data.chomp + ] + end ok end From 6b2fda568d20245c85aa0b39b1c0d333ab0ddc1f Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 18:00:09 -0400 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=8D=92=20pick=2024d5c773d:=20?= =?UTF-8?q?=F0=9F=94=92=F0=9F=A5=85=20Handle=20tagged=20"OK"=20to=20incomp?= =?UTF-8?q?lete=20command=20[backport=20#664]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking a "belt-and-suspenders" approach: This is a potential problem for any command which registers a response handler: a malicious server can easily guess what the next tag will be, and send an `OK` response _before_ the client the response handler is attached. `STARTTLS` is an extreme example of this issue: if the `STARTTLS` handler does not run, then `#starttls` will not start the TLS session, and the connection is not secured, _but no error is raised._ We should _also_ attach the response handler before sending the `CRLF`, but that is neither necessary (the response handler will added before the `synchronize` mutex is unlocked) nor sufficient (the fake `OK` can be sent _much_ earlier). On the other hand, it _is_ okay for the server to send an error tagged response (`NO` or `BAD`), before the sending the command has completed. --- lib/net/imap.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 4d81d61cb..6b6a08fad 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -2311,6 +2311,7 @@ def send_command(cmd, *args, &block) put_string(" ") send_data(i, tag) end + guard_against_tagged_response_skipping_handler!(tag) put_string(CRLF) if cmd == "LOGOUT" @logout_command_tag = tag @@ -2326,6 +2327,17 @@ def send_command(cmd, *args, &block) end end end + rescue InvalidResponseError + disconnect + raise + end + + def guard_against_tagged_response_skipping_handler!(tag) + return unless (resp = @tagged_responses[tag])&.name&.upcase == "OK" + raise(InvalidResponseError, + "Server sent tagged 'OK' before command was finished: %p. " \ + "This could indicate a malicious server or client-side " \ + "command injection. Disconnecting." % [resp.raw_data.chomp]) end def generate_tag From ceeb92a23195e4223743575ed7dd9e003b43988b Mon Sep 17 00:00:00 2001 From: nick evans Date: Thu, 23 Apr 2026 15:52:34 -0400 Subject: [PATCH 6/6] =?UTF-8?q?=E2=9C=85=20Block=20incompatible=20rdoc=20v?= =?UTF-8?q?ersion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rdoc 7.2.0 lists >=2.6 as it's required ruby version, but it isn't tested in CI, and it is broken. See ruby/rdoc#1683. --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index b0c045662..35c02c170 100644 --- a/Gemfile +++ b/Gemfile @@ -5,5 +5,5 @@ source "https://rubygems.org" gemspec gem "rake" -gem "rdoc" +gem "rdoc", "<7.2" # incompatible with ruby 2.6 gem "test-unit"