-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Changes from all commits
be73188
dca899d
481ca55
c28a745
08b9d63
4304e2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
||
|
@@ -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) | ||
|
@@ -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: | ||
pepoluan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if code in (235, 503): | ||
return (code, resp) | ||
raise SMTPAuthenticationError(code, resp) | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
def _auth_cram_md5(self, arg=None): | ||
if arg is None: | ||
self.push('334 {}'.format(sim_cram_md5_challenge)) | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't use smtplib.SMTP as context manager here ... its |
||
|
||
@hashlib_helper.requires_hashdigest('md5') | ||
def testAUTH_CRAM_MD5(self): | ||
self.serv.add_feature("AUTH CRAM-MD5") | ||
|
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. |
Uh oh!
There was an error while loading. Please reload this page.