🔒 Verify SASL authenticators are done

The protocol client is responsible for raising an error if the command
completes successfully but "done?" returns false.

Author: nevans

lib/net/imap.rb

@@ -1088,7 +1088,7 @@ def logout
     def logout!
       logout unless disconnected?
     rescue => ex
-      warn "%s during <Net::IMAP %s:%s>logout!: %s" % [
+      warn "%s during <Net::IMAP %s:%s> logout!: %s" % [
         ex.class, host, port, ex
       ]
     ensure
@@ -1238,17 +1238,20 @@ 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)
           response  = [response].pack("m0")
           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]

lib/net/imap/sasl.rb

@@ -77,6 +77,31 @@ class IMAP
     # <em>Using a deprecated mechanism will print a warning.</em>
     #
     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 canceled
+      # by the client, not due to any error or failure during processing.
+      AuthenticationCanceled = 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 one of the server's
+      # messages 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

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

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)

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)

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

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

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

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)

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|