Skip to content

bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP #24118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions Lib/smtplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
CRLF = "\r\n"
bCRLF = b"\r\n"
_MAXLINE = 8192 # more than 8 times larger than RFC 821, 4.5.3
_MAXCHALLENGE = 5 # Maximum number of AUTH challenges sent

OLDSTYLE_AUTH = re.compile(r"auth=(.*)", re.I)

Expand Down Expand Up @@ -248,6 +249,7 @@ def __init__(self, host='', port=0, local_hostname=None,
self.esmtp_features = {}
self.command_encoding = 'ascii'
self.source_address = source_address
self._auth_challenge_count = 0

if host:
(code, msg) = self.connect(host, port)
Expand Down Expand Up @@ -633,14 +635,23 @@ def auth(self, mechanism, authobject, *, initial_response_ok=True):
if initial_response is not None:
response = encode_base64(initial_response.encode('ascii'), eol='')
(code, resp) = self.docmd("AUTH", mechanism + " " + response)
self._auth_challenge_count = 1
else:
(code, resp) = self.docmd("AUTH", mechanism)
self._auth_challenge_count = 0
# If server responds with a challenge, send the response.
if code == 334:
while code == 334:
self._auth_challenge_count += 1
challenge = base64.decodebytes(resp)
response = encode_base64(
authobject(challenge).encode('ascii'), eol='')
(code, resp) = self.docmd(response)
# If server keeps sending challenges, something is wrong.
if self._auth_challenge_count > _MAXCHALLENGE:
raise SMTPException(
"Server AUTH mechanism infinite loop. Last response: "
+ repr((code, resp))
)
Comment on lines +649 to +654
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guards against broken/buggy server that traps us in the AUTH state.

At this point, state is indeterminate, and we should let user handle things. That's why it simply raises. (The only "guaranteed safe" thing to do is close(), actually.)

if code in (235, 503):
return (code, resp)
raise SMTPAuthenticationError(code, resp)
Expand All @@ -662,7 +673,7 @@ def auth_plain(self, challenge=None):
def auth_login(self, challenge=None):
""" Authobject to use with LOGIN authentication. Requires self.user and
self.password to be set."""
if challenge is None:
if challenge is None or self._auth_challenge_count < 2:
Copy link
Contributor Author

@pepoluan pepoluan Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The draft standard for AUTH LOGIN specifies that clients SHOULD ignore the actual contents of the challenge, and respond only based on how many challenges have been received.

return self.user
else:
return self.password
Expand Down
45 changes: 44 additions & 1 deletion Lib/test/test_smtplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ def found_terminator(self):
except ResponseException as e:
self.smtp_state = self.COMMAND
self.push('%s %s' % (e.smtp_code, e.smtp_error))
return
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mis-indented return caused the test for smtpd state to "fall through" to the next test, resulting ultimately in "Internal Confusion" error.

super().found_terminator()


Expand Down Expand Up @@ -851,6 +851,11 @@ def _auth_login(self, arg=None):
self._authenticated(self._auth_login_user, password == sim_auth[1])
del self._auth_login_user

def _auth_buggy(self, arg=None):
# This AUTH mechanism will 'trap' client in a neverending 334
# base64 encoded 'BuGgYbUgGy'
self.push('334 QnVHZ1liVWdHeQ==')
Comment on lines +854 to +857
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally made-up AUTH mechanism whose purpose is to 'trap' the client in an infinite loop of 334 (AUTH Challenge)


def _auth_cram_md5(self, arg=None):
if arg is None:
self.push('334 {}'.format(sim_cram_md5_challenge))
Expand Down Expand Up @@ -1069,6 +1074,44 @@ def testAUTH_LOGIN(self):
self.assertEqual(resp, (235, b'Authentication Succeeded'))
smtp.close()

def testAUTH_LOGIN_initial_response_ok(self):
self.serv.add_feature("AUTH LOGIN")
with smtplib.SMTP(HOST, self.port, local_hostname='localhost',
timeout=support.LOOPBACK_TIMEOUT) as smtp:
smtp.user, smtp.password = sim_auth
smtp.ehlo("test_auth_login")
resp = smtp.auth("LOGIN", smtp.auth_login, initial_response_ok=True)
self.assertEqual(resp, (235, b'Authentication Succeeded'))

def testAUTH_LOGIN_initial_response_notok(self):
self.serv.add_feature("AUTH LOGIN")
with smtplib.SMTP(HOST, self.port, local_hostname='localhost',
timeout=support.LOOPBACK_TIMEOUT) as smtp:
smtp.user, smtp.password = sim_auth
smtp.ehlo("test_auth_login")
resp = smtp.auth("LOGIN", smtp.auth_login, initial_response_ok=False)
self.assertEqual(resp, (235, b'Authentication Succeeded'))

def testAUTH_BUGGY(self):
self.serv.add_feature("AUTH BUGGY")

def auth_buggy(challenge=None):
self.assertEqual(b"BuGgYbUgGy", challenge)
return "\0"

smtp = smtplib.SMTP(
HOST, self.port, local_hostname='localhost',
timeout=support.LOOPBACK_TIMEOUT
)
try:
smtp.user, smtp.password = sim_auth
smtp.ehlo("test_auth_buggy")
expect = r"^Server AUTH mechanism infinite loop.*"
with self.assertRaisesRegex(smtplib.SMTPException, expect) as cm:
smtp.auth("BUGGY", auth_buggy, initial_response_ok=False)
finally:
smtp.close()
Comment on lines +1102 to +1113
Copy link
Contributor Author

@pepoluan pepoluan Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use smtplib.SMTP as context manager here ... its __exit__ causes a different exception to be raised in addition. So the test uses a decidedly "lower level" mechanism to close the connection.


@hashlib_helper.requires_hashdigest('md5')
def testAUTH_CRAM_MD5(self):
self.serv.add_feature("AUTH CRAM-MD5")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Fixed long-standing bug of smtplib.SMTP where doing AUTH LOGIN with
initial_response_ok=False will fail.

The cause is that SMTP.auth_login _always_ returns a password if provided
with a challenge string, thus non-compliant with the standard for AUTH
LOGIN.

Also fixes bug with the test for smtpd.