Merge branch 'master' into defer-ehlo-resp-155

pepoluan · pepoluan · commit a3fe96f86eb0 · 2021-01-24T02:39:33.000+07:00
# Conflicts:
#	aiosmtpd/smtp.py
diff --git a/.gitignore b/.gitignore
@@ -33,3 +33,4 @@ diffcov.html
 diffcov-*.html
 .python-version
 .pytype/
+~temp*
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
@@ -2,6 +2,20 @@
  NEWS for aiosmtpd
 ===================
 
+1.2.4 (2021-01-24)
+==================
+
+Added
+-----
+* Optional (default-disabled) logging of ``AUTH`` interaction -- with severe warnings
+
+Fixed/Improved
+--------------
+* ``AUTH`` command line now sanitized before logging (Closes #233)
+* Remove special handling for lone ``=`` during AUTH;
+  it is now treated as simple Base64-encoded ``b""``.
+  This is the correct, strict interpretation of :rfc:`4954` mentions about ``=``
+
 
 1.2.4 (aiosmtpd-next)
 =====================
diff --git a/aiosmtpd/docs/handlers.rst b/aiosmtpd/docs/handlers.rst
@@ -215,7 +215,7 @@ those methods of the handler instance will override the built-in methods.
 
   :boldital:`server` is the instance of the ``SMTP`` class invoking the AUTH hook.
   This allows the AUTH hook implementation to invoke facilities such as the
-  ``push()`` and ``_auth_interact()`` methods.
+  ``push()`` and ``challenge_auth()`` methods.
 
   :boldital:`args` is a list of string split from the string after the ``AUTH`` command.
   ``args[0]`` is always equal to ``MECHANISM``.
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
@@ -9,12 +9,13 @@
 import collections
 import asyncio.sslproto as sslproto
 
-from base64 import b64decode
+from base64 import b64decode, b64encode
 from email._header_value_parser import get_addr_spec, get_angle_addr
 from email.errors import HeaderParseError
 from public import public
 from typing import (
     Any,
+    AnyStr,
     Awaitable,
     Callable,
     Dict,
@@ -30,7 +31,8 @@
 # region #### Custom Data Types #######################################################
 
 class _Missing:
-    pass
+    def __repr__(self):
+        return "MISSING"
 
 
 class _AuthMechAttr(NamedTuple):
@@ -59,7 +61,7 @@ class _DataState(enum.Enum):
     "AuthMechanismType",
     "MISSING",
 ]  # Will be added to by @public
-__version__ = '1.3.0a1'
+__version__ = '1.3.0a2'
 __ident__ = 'Python SMTP {}'.format(__version__)
 log = logging.getLogger('mail.log')
 
@@ -76,6 +78,19 @@ class _DataState(enum.Enum):
 # https://tools.ietf.org/html/rfc3207.html#page-3
 ALLOWED_BEFORE_STARTTLS = {"NOOP", "EHLO", "STARTTLS", "QUIT"}
 
+# Auth hiding regexes
+CLIENT_AUTH_B = re.compile(
+    # Matches "AUTH" <mechanism> <whitespace_but_not_\r_nor_\n>
+    br"(?P<authm>\s*AUTH\s+\S+[^\S\r\n]+)"
+    # Param to AUTH <mechanism>. We only need to sanitize if param is given, which
+    # for some mechanisms contain sensitive info. If no param is given, then we
+    # can skip (match fails)
+    br"(\S+)"
+    # Optional bCRLF at end. Why optional? Because we also want to sanitize the
+    # stripped line. If no bCRLF, then this group will be b""
+    br"(?P<crlf>(?:\r\n)?)", re.IGNORECASE
+)
+
 # endregion
 
 
@@ -162,6 +177,23 @@ class TLSSetupException(Exception):
     pass
 
 
+@public
+def sanitize(text: bytes) -> bytes:
+    m = CLIENT_AUTH_B.match(text)
+    if m:
+        return m.group("authm") + b"********" + m.group("crlf")
+    return text
+
+
+@public
+def sanitized_log(func: Callable, msg: AnyStr, *args, **kwargs):
+    sanitized_args = [
+        sanitize(a) if isinstance(a, bytes) else a
+        for a in args
+    ]
+    func(msg, *sanitized_args, **kwargs)
+
+
 @public
 class SMTP(asyncio.StreamReaderProtocol):
     command_size_limit = 512
@@ -404,10 +436,14 @@ def _set_rset_state(self):
         """Reset all state variables except the greeting."""
         self._set_post_data_state()
 
-    async def push(self, status):
-        response = bytes(
-            status + '\r\n', 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
-        self._writer.write(response)
+    async def push(self, status: AnyStr):
+        if isinstance(status, str):
+            response = bytes(
+                status, 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
+        else:
+            response = status
+        assert isinstance(response, bytes)
+        self._writer.write(response + b"\r\n")
         log.debug("%r << %r", self.session.peer, response)
         await self._writer.drain()
 
@@ -452,10 +488,10 @@ async def _handle_client(self):
                     # send error response and read the next command line.
                     await self.push('500 Command line too long')
                     continue
-                log.debug('_handle_client readline: %r', line)
+                sanitized_log(log.debug, '_handle_client readline: %r', line)
                 # XXX this rstrip may not completely preserve old behavior.
                 line = line.rstrip(b'\r\n')
-                log.info('%r >> %r', self.session.peer, line)
+                sanitized_log(log.info, '%r >> %r', self.session.peer, line)
                 if not line:
                     await self.push('500 Error: bad syntax')
                     continue
@@ -762,18 +798,44 @@ async def smtp_AUTH(self, arg: str) -> None:
             if status is not None:  # pragma: no branch
                 await self.push(status)
 
-    async def _auth_interact(self, server_message) -> _TriStateType:
-        blob: bytes
-        await self.push(server_message)
+    async def challenge_auth(
+            self,
+            challenge: AnyStr,
+            encode_to_b64: bool = True,
+            log_client_response: bool = False,
+    ) -> Union[_Missing, bytes]:
+        """
+        Send challenge during authentication. "334 " will be prefixed, so do NOT
+        put "334 " at start of server_message.
+
+        :param challenge: Challenge to send to client. If str, will be utf8-encoded.
+        :param encode_to_b64: If true, then perform Base64 encoding on challenge
+        :param log_client_response: Perform logging of client's response.
+            WARNING: Might cause leak of sensitive information! Do not turn on
+            unless _absolutely_ necessary!
+        :return: Response from client, or MISSING
+        """
+        challenge = (
+            challenge.encode() if isinstance(challenge, str) else challenge
+        )
+        assert isinstance(challenge, bytes)
+        # Trailing space is MANDATORY even if challenge is empty.
+        # See:
+        #   - https://tools.ietf.org/html/rfc4954#page-4 ¶ 5
+        #   - https://tools.ietf.org/html/rfc4954#page-13 "continue-req"
+        challenge = b"334 " + (b64encode(challenge) if encode_to_b64 else challenge)
+        log.debug("%r << challenge: %r", self.session.peer, challenge)
+        await self.push(challenge)
         line = await self._reader.readline()
-        blob = line.strip()
-        # '=' and '*' handling are in accordance with RFC4954
-        if blob == b"=":
-            log.debug("%r responded with '='", self.session.peer)
-            return None
+        if log_client_response:
+            warn("AUTH interaction logging is enabled!")
+            warn("Sensitive information might be leaked!")
+            log.debug("%r >> %r", self.session.peer, line)
+        blob: bytes = line.strip()
+        # '*' handling in accordance with RFC4954
         if blob == b"*":
-            log.warning("%r aborted with '*'", self.session.peer)
-            await self.push("501 Auth aborted")
+            log.warning("%r aborted AUTH with '*'", self.session.peer)
+            await self.push("501 5.7.0 Auth aborted")
             return MISSING
         try:
             decoded_blob = b64decode(blob, validate=True)
@@ -783,6 +845,22 @@ async def _auth_interact(self, server_message) -> _TriStateType:
             return MISSING
         return decoded_blob
 
+    _334_PREFIX = re.compile(r"^334 ")
+
+    async def _auth_interact(
+            self,
+            server_message: str
+    ) -> Union[_Missing, bytes]:  # pragma: nocover
+        warn(
+            "_auth_interact will be deprecated in version 2.0. "
+            "Please use challenge_auth() instead.",
+            DeprecationWarning
+        )
+        return await self.challenge_auth(
+            challenge=self._334_PREFIX.sub("", server_message),
+            encode_to_b64=False,
+        )
+
     # IMPORTANT NOTES FOR THE auth_* METHODS
     #
     # 1. For internal methods, due to how they are called, we must ignore
@@ -797,64 +875,52 @@ async def _auth_interact(self, server_message) -> _TriStateType:
     #      - 'identity' is not always username, depending on the auth mecha-
     #        nism. Might be a session key, a one-time user ID, or any kind of
     #        object, actually.
-    #      - If the client provides "=" for username during interaction, the
-    #        method MUST return b"" (empty bytes) NOT None, because None has been
-    #        used to indicate error/login failure.
     # 3. Auth credentials checking is performed in the auth_* methods because
     #    more advanced auth mechanism might not return login+password pair
     #    (see #2 above)
 
     async def auth_PLAIN(self, _, args: List[str]):
         login_and_password: _TriStateType
         if len(args) == 1:
-            # Trailing space is MANDATORY
-            # See https://tools.ietf.org/html/rfc4954#page-4 ¶ 5
-            login_and_password = await self._auth_interact("334 ")
+            login_and_password = await self.challenge_auth("")
             if login_and_password is MISSING:
                 return
-        else:
-            blob = args[1]
-            if blob == "=":
-                login_and_password = None
-            else:
-                try:
-                    login_and_password = b64decode(blob, validate=True)
-                except Exception:
-                    await self.push("501 5.5.2 Can't decode base64")
-                    return
-        # Decode login data
-        if login_and_password is None:
-            login = password = None
         else:
             try:
-                _, login, password = login_and_password.split(b"\x00")
-            except ValueError:  # not enough args
-                await self.push("501 5.5.2 Can't split auth value")
+                login_and_password = b64decode(args[1].encode(), validate=True)
+            except Exception:
+                await self.push("501 5.5.2 Can't decode base64")
                 return
+        try:
+            # login data is "{authz_id}\x00{login_id}\x00{password}"
+            # authz_id can be null, and currently ignored
+            # See https://tools.ietf.org/html/rfc4616#page-3
+            _, login, password = login_and_password.split(b"\x00")
+        except ValueError:  # not enough args
+            await self.push("501 5.5.2 Can't split auth value")
+            return
         # Verify login data
+        assert login is not None
+        assert password is not None
         if self._auth_callback("PLAIN", login, password):
-            if login is None:
-                login = EMPTYBYTES
             return login
         else:
             return MISSING
 
     async def auth_LOGIN(self, _, args: List[str]):
         login: _TriStateType
-        # 'User Name\x00'
-        login = await self._auth_interact("334 VXNlciBOYW1lAA==")
+        login = await self.challenge_auth(b"User Name\x00")
         if login is MISSING:
             return
 
         password: _TriStateType
-        # 'Password\x00'
-        password = await self._auth_interact("334 UGFzc3dvcmQA")
+        password = await self.challenge_auth(b"Password\x00")
         if password is MISSING:
             return
 
+        assert login is not None
+        assert password is not None
         if self._auth_callback("LOGIN", login, password):
-            if login is None:  # pragma: no branch
-                login = EMPTYBYTES
             return login
         else:
             return MISSING
diff --git a/aiosmtpd/testing/helpers.py b/aiosmtpd/testing/helpers.py
@@ -65,19 +65,16 @@ def start(plugin):
         warnings.filterwarnings('always', category=ResourceWarning)
 
 
-def assert_auth_success(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 235)
-    testcase.assertEqual(response, b"2.7.0 Authentication successful")
+def assert_auth_success(testcase: TestCase, *response):
+    assert response == (235, b"2.7.0 Authentication successful")
 
 
-def assert_auth_invalid(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 535)
-    testcase.assertEqual(response, b'5.7.8 Authentication credentials invalid')
+def assert_auth_invalid(testcase: TestCase, *response):
+    assert response == (535, b"5.7.8 Authentication credentials invalid")
 
 
-def assert_auth_required(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 530)
-    testcase.assertEqual(response, b'5.7.0 Authentication required')
+def assert_auth_required(testcase: TestCase, *response):
+    assert response == (530, b"5.7.0 Authentication required")
 
 
 SUPPORTED_COMMANDS_TLS: bytes = (
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
