From c9a6f28f8794cb991613d378b3920e5719062e29 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 17:16:25 -0400 Subject: [PATCH 1/3] =?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 | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 3eb726c2..79e6deaf 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -158,6 +158,43 @@ def test_starttls_stripping assert_equal(CA_FILE, imap.ssl_ctx.ca_file) assert_equal(OpenSSL::SSL::VERIFY_PEER, imap.ssl_ctx.verify_mode) 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_equal false, imap.tls_verified? + assert_equal({ca_file: CA_FILE}, imap.ssl_ctx_params) + assert_equal(CA_FILE, imap.ssl_ctx.ca_file) + assert_equal(OpenSSL::SSL::VERIFY_PEER, imap.ssl_ctx.verify_mode) + end end def start_server From 705aa59faa28083f496dce50c0ee1eccca287506 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 17:31:11 -0400 Subject: [PATCH 2/3] =?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 d7c2014d..e1f5e553 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1312,9 +1312,11 @@ def logout! # def starttls(**options) @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options) + handled = false error = nil ok = send_command("STARTTLS") do |resp| if resp.kind_of?(TaggedResponse) && resp.name == "OK" + handled = true clear_cached_capabilities clear_responses start_tls_session @@ -1326,6 +1328,13 @@ def starttls(**options) 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 038ae35d5ecbb2b85f77f4fa35e46604154dc8c4 Mon Sep 17 00:00:00 2001 From: nick evans Date: Fri, 27 Mar 2026 18:00:09 -0400 Subject: [PATCH 3/3] =?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 e1f5e553..ee3bc672 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -3061,6 +3061,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 @@ -3076,6 +3077,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