From 2086be061fcbe99c657719e9347449e6ae1ccf65 Mon Sep 17 00:00:00 2001 From: nick evans Date: Tue, 15 Nov 2022 19:36:46 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Verify=20SASL=20authenticators?= =?UTF-8?q?=20are=20done?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The protocol client is responsible for raising an error if the command completes successfully but "done?" returns false. --- lib/net/imap.rb | 11 +++-- lib/net/imap/sasl.rb | 40 ++++++++++++++++++- lib/net/imap/sasl/anonymous_authenticator.rb | 9 +++++ lib/net/imap/sasl/cram_md5_authenticator.rb | 5 +++ lib/net/imap/sasl/digest_md5_authenticator.rb | 7 +++- lib/net/imap/sasl/external_authenticator.rb | 9 +++++ lib/net/imap/sasl/login_authenticator.rb | 8 +++- lib/net/imap/sasl/plain_authenticator.rb | 9 +++++ lib/net/imap/sasl/xoauth2_authenticator.rb | 9 +++++ test/net/imap/test_imap.rb | 37 ++++++++++++++++- 10 files changed, 133 insertions(+), 11 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 2c5d81720..81ba115b8 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1238,7 +1238,7 @@ def authenticate(mechanism, *creds, sasl_ir: true, **props, &callback) SASL.initial_response?(authenticator) cmdargs << [authenticator.process(nil)].pack("m0") end - send_command(*cmdargs) do |resp| + result = send_command(*cmdargs) do |resp| if resp.instance_of?(ContinuationRequest) challenge = resp.data.text.unpack1("m") response = authenticator.process(challenge) @@ -1246,9 +1246,12 @@ def authenticate(mechanism, *creds, sasl_ir: true, **props, &callback) put_string(response + CRLF) end end - .tap { @capabilities = capabilities_from_resp_code _1 } - # NOTE: If any Net::IMAP::SASL mechanism ever supports security layer - # negotiation, capabilities sent during the "OK" response MUST be ignored. + unless SASL.done?(authenticator) + logout! + raise SASL::AuthenticationFailed, "authentication ended prematurely" + end + @capabilities = capabilities_from_resp_code result + result end # Sends a {LOGIN command [IMAP4rev1 ยง6.2.3]}[https://www.rfc-editor.org/rfc/rfc3501#section-6.2.3] diff --git a/lib/net/imap/sasl.rb b/lib/net/imap/sasl.rb index 3bcffb6b0..4f379d458 100644 --- a/lib/net/imap/sasl.rb +++ b/lib/net/imap/sasl.rb @@ -77,6 +77,31 @@ class IMAP # Using a deprecated mechanism will print a warning. # module SASL + # Exception class for any client error detected during the authentication + # exchange. + # + # When the _server_ reports an authentication failure, it will respond + # with a protocol specific error instead, e.g: +BAD+ or +NO+ in IMAP. + # + # When the client encounters any error, it *must* consider the + # authentication exchange to be unsuccessful and it might need to drop the + # connection. For example, if the server reports that the authentication + # exchange was successful or the protocol does not allow additional + # authentication attempts. + Error = Class.new(StandardError) + + # Indicates an authentication exchange that will be or has been cancelled + # by the client, not due to any error or failure during processing. + AuthenticationCancelled = Class.new(Error) + + # Indicates an error when processing a server challenge, e.g: an invalid + # or unparsable challenge. An underlying exception may be available as + # the exception's #cause. + AuthenticationError = Class.new(Error) + + # Indicates that authentication cannot proceed because the server has + # not passed integrity checks. + AuthenticationFailed = Class.new(Error) # autoloading to avoid loading all of the regexps when they aren't used. sasl_stringprep_rb = File.expand_path("sasl/stringprep", __dir__) @@ -118,13 +143,24 @@ def saslprep(string, **opts) Net::IMAP::StringPrep::SASLprep.saslprep(string, **opts) end - # Returns whether the authenticator is client-first and supports sending - # an "initial response". + # Returns whether +authenticator+ is client-first and supports sending an + # "initial response". def initial_response?(authenticator) authenticator.respond_to?(:initial_response?) && authenticator.initial_response? end + # Returns whether +authenticator+ considers the authentication exchange to + # be complete. + # + # The authentication should not succeed if this returns false, but + # returning true does *not* indicate success. Authentication succeeds + # when this method returns true and the server responds with a + # protocol-specific success. + def done?(authenticator) + !authenticator.respond_to?(:done?) || authenticator.done? + end + end end end diff --git a/lib/net/imap/sasl/anonymous_authenticator.rb b/lib/net/imap/sasl/anonymous_authenticator.rb index fbcfeeac8..ed7a46efe 100644 --- a/lib/net/imap/sasl/anonymous_authenticator.rb +++ b/lib/net/imap/sasl/anonymous_authenticator.rb @@ -40,6 +40,7 @@ def initialize(anon_msg = nil, anonymous_message: nil, **) raise ArgumentError, "anonymous_message is too long. (%d codepoints)" % [size] end + @done = false end # :call-seq: @@ -51,8 +52,16 @@ def initial_response?; true end # Returns #anonymous_message. def process(_server_challenge_string) anonymous_message + ensure + @done = true end + # Returns true when the initial client response was sent. + # + # The authentication should not succeed unless this returns true, but it + # does *not* indicate success. + def done?; @done end + end end end diff --git a/lib/net/imap/sasl/cram_md5_authenticator.rb b/lib/net/imap/sasl/cram_md5_authenticator.rb index 42935d3a0..6a6242eb4 100644 --- a/lib/net/imap/sasl/cram_md5_authenticator.rb +++ b/lib/net/imap/sasl/cram_md5_authenticator.rb @@ -21,13 +21,18 @@ def initialize(user, password, warn_deprecation: true, **_ignored) require "digest/md5" @user = user @password = password + @done = false end def process(challenge) digest = hmac_md5(challenge, @password) return @user + " " + digest + ensure + @done = true end + def done?; @done end + private def hmac_md5(text, key) diff --git a/lib/net/imap/sasl/digest_md5_authenticator.rb b/lib/net/imap/sasl/digest_md5_authenticator.rb index d1576ba5f..d77afdadb 100644 --- a/lib/net/imap/sasl/digest_md5_authenticator.rb +++ b/lib/net/imap/sasl/digest_md5_authenticator.rb @@ -11,7 +11,8 @@ class Net::IMAP::SASL::DigestMD5Authenticator STAGE_ONE = :stage_one STAGE_TWO = :stage_two - private_constant :STAGE_ONE, :STAGE_TWO + STAGE_DONE = :stage_done + private_constant :STAGE_ONE, :STAGE_TWO, :STAGE_DONE # Authentication identity: the identity that matches the #password. # @@ -126,7 +127,7 @@ def process(challenge) return response.keys.map {|key| qdval(key.to_s, response[key]) }.join(',') when STAGE_TWO - @stage = nil + @stage = STAGE_DONE # if at the second stage, return an empty string if challenge =~ /rspauth=/ return '' @@ -138,6 +139,8 @@ def process(challenge) end end + def done?; @stage == STAGE_DONE end + private def nc(nonce) diff --git a/lib/net/imap/sasl/external_authenticator.rb b/lib/net/imap/sasl/external_authenticator.rb index 921b2ce6a..f229c63d7 100644 --- a/lib/net/imap/sasl/external_authenticator.rb +++ b/lib/net/imap/sasl/external_authenticator.rb @@ -34,6 +34,7 @@ def initialize(authzid: nil, **) if @authzid&.match?(/\u0000/u) # also validates UTF8 encoding raise ArgumentError, "contains NULL" end + @done = false end # :call-seq: @@ -45,8 +46,16 @@ def initial_response?; true end # Returns #authzid, or an empty string if there is no authzid. def process(_) authzid || "" + ensure + @done = true end + # Returns true when the initial client response was sent. + # + # The authentication should not succeed unless this returns true, but it + # does *not* indicate success. + def done?; @done end + end end end diff --git a/lib/net/imap/sasl/login_authenticator.rb b/lib/net/imap/sasl/login_authenticator.rb index 11d508df5..f13cac6aa 100644 --- a/lib/net/imap/sasl/login_authenticator.rb +++ b/lib/net/imap/sasl/login_authenticator.rb @@ -20,7 +20,8 @@ class Net::IMAP::SASL::LoginAuthenticator STATE_USER = :USER STATE_PASSWORD = :PASSWORD - private_constant :STATE_USER, :STATE_PASSWORD + STATE_DONE = :DONE + private_constant :STATE_USER, :STATE_PASSWORD, :STATE_DONE def initialize(user, password, warn_deprecation: true, **_ignored) if warn_deprecation @@ -37,7 +38,12 @@ def process(data) @state = STATE_PASSWORD return @user when STATE_PASSWORD + @state = STATE_DONE return @password + when STATE_DONE + raise ResponseParseError, data end end + + def done?; @state == STATE_DONE end end diff --git a/lib/net/imap/sasl/plain_authenticator.rb b/lib/net/imap/sasl/plain_authenticator.rb index 8b04ab7f9..4d5978d35 100644 --- a/lib/net/imap/sasl/plain_authenticator.rb +++ b/lib/net/imap/sasl/plain_authenticator.rb @@ -61,6 +61,7 @@ def initialize(user = nil, pass = nil, @username = username @password = password @authzid = authzid + @done = false end # :call-seq: @@ -72,6 +73,14 @@ def initial_response?; true end # Responds with the client's credentials. def process(data) return "#@authzid\0#@username\0#@password" + ensure + @done = true end + # Returns true when the initial client response was sent. + # + # The authentication should not succeed unless this returns true, but it + # does *not* indicate success. + def done?; @done end + end diff --git a/lib/net/imap/sasl/xoauth2_authenticator.rb b/lib/net/imap/sasl/xoauth2_authenticator.rb index 98debbc0b..164afa0d1 100644 --- a/lib/net/imap/sasl/xoauth2_authenticator.rb +++ b/lib/net/imap/sasl/xoauth2_authenticator.rb @@ -56,6 +56,7 @@ def initialize(user = nil, token = nil, username: nil, oauth2_token: nil, **) raise ArgumentError, "conflicting values for username" [oauth2_token, token].compact.count == 1 or raise ArgumentError, "conflicting values for oauth2_token" + @done = false end # :call-seq: @@ -68,8 +69,16 @@ def initial_response?; true end # with the +oauth2_token+. def process(_data) build_oauth2_string(@username, @oauth2_token) + ensure + @done = true end + # Returns true when the initial client response was sent. + # + # The authentication should not succeed unless this returns true, but it + # does *not* indicate success. + def done?; @done end + private def build_oauth2_string(username, oauth2_token) diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 8baf67b44..d67889f74 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -911,18 +911,51 @@ def test_id ].pack("m0") ) state.commands << {continuation: response_b64} + response_b64 = cmd.request_continuation(["rspauth="].pack("m0")) + state.commands << {continuation: response_b64} server.state.authenticate(server.config.user) cmd.done_ok end imap.authenticate("DIGEST-MD5", "test_user", "test-password", warn_deprecation: false) - cmd, cont = 2.times.map { server.commands.pop } + cmd, cont1, cont2 = 3.times.map { server.commands.pop } assert_equal %w[AUTHENTICATE DIGEST-MD5], [cmd.name, *cmd.args] - assert_match(%r{\A[a-z0-9+/]+=*\z}i, cont[:continuation].strip) + assert_match(%r{\A[a-z0-9+/]+=*\z}i, cont1[:continuation].strip) + assert_empty cont2[:continuation].strip assert_empty server.commands end end + test("#authenticate disconnects and raises SASL::AuthenticationFailed " \ + "when the server succeeds prematurely") do + with_fake_server( + preauth: false, cleartext_auth: true, + sasl_ir: true, sasl_mechanisms: %i[DIGEST-MD5] + ) do |server, imap| + server.on "AUTHENTICATE" do |cmd| + response_b64 = cmd.request_continuation( + [ + %w[ + realm="somerealm" + nonce="OA6MG9tEQGm2hh" + qop="auth" + charset=utf-8 + algorithm=md5-sess + ].join(",") + ].pack("m0") + ) + state.commands << {continuation: response_b64} + server.state.authenticate(server.config.user) + cmd.done_ok + end + assert_raise(Net::IMAP::SASL::AuthenticationFailed) do + imap.authenticate("DIGEST-MD5", "test_user", "test-password", + warn_deprecation: false) + end + assert imap.disconnected? + end + end + def test_uidplus_uid_expunge with_fake_server(select: "INBOX", extensions: %i[UIDPLUS]) do |server, imap|