Skip to content

Commit ede4e37

Browse files
committed
auth: Correctly parse check_authentication response
This previously used substring search, which is incorrect, although unlikely to be a vulnerability because there are no free-form text fields allowed in this response format.
1 parent 1c178be commit ede4e37

2 files changed

Lines changed: 24 additions & 11 deletions

File tree

tornado/auth.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ async def get(self):
7373
import binascii
7474
import hashlib
7575
import hmac
76+
import re
7677
import time
7778
import urllib.parse
7879
import uuid
@@ -217,7 +218,7 @@ def _on_authentication_verified(
217218
self, response: httpclient.HTTPResponse
218219
) -> Dict[str, Any]:
219220
handler = cast(RequestHandler, self)
220-
if b"is_valid:true" not in response.body:
221+
if re.search(rb"(?m)^is_valid:true$", response.body) is None:
221222
raise AuthError("Invalid OpenID response: %r" % response.body)
222223

223224
# Make sure we got back at least an email from attribute exchange

tornado/test/auth_test.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,20 @@ def get(self):
4646

4747

4848
class OpenIdServerAuthenticateHandler(RequestHandler):
49+
flip_flop = False
50+
4951
def post(self):
5052
if self.get_argument("openid.mode") != "check_authentication":
5153
raise Exception("incorrect openid.mode %r")
52-
self.write("is_valid:true")
54+
# Cover both orderings of the response parameters if we call this handler twice.
55+
# (the flip_flop side effect is simpler than plumbing parameters around).
56+
# We check both orderings to catch mistaken uses of re.match instead of re.search
57+
# or incorrect matching of the newline characters.
58+
if type(self).flip_flop:
59+
self.write("is_valid:true\nns:http://specs.openid.net/auth/2.0\n")
60+
else:
61+
self.write("ns:http://specs.openid.net/auth/2.0\nis_valid:true\n")
62+
type(self).flip_flop = not type(self).flip_flop
5363

5464

5565
class OAuth1ClientLoginHandler(RequestHandler, OAuthMixin):
@@ -344,15 +354,17 @@ def test_openid_redirect(self):
344354
self.assertIn("/openid/server/authenticate?", response.headers["Location"])
345355

346356
def test_openid_get_user(self):
347-
response = self.fetch(
348-
"/openid/client/login?openid.mode=blah"
349-
"&openid.ns.ax=http://openid.net/srv/ax/1.0"
350-
"&openid.ax.type.email=http://axschema.org/contact/email"
351-
"&openid.ax.value.email=foo@example.com"
352-
)
353-
response.rethrow()
354-
parsed = json_decode(response.body)
355-
self.assertEqual(parsed["email"], "foo@example.com")
357+
for i in range(2):
358+
with self.subTest(i=i):
359+
response = self.fetch(
360+
"/openid/client/login?openid.mode=blah"
361+
"&openid.ns.ax=http://openid.net/srv/ax/1.0"
362+
"&openid.ax.type.email=http://axschema.org/contact/email"
363+
"&openid.ax.value.email=foo@example.com"
364+
)
365+
response.rethrow()
366+
parsed = json_decode(response.body)
367+
self.assertEqual(parsed["email"], "foo@example.com")
356368

357369
def test_oauth10_redirect(self):
358370
response = self.fetch("/oauth10/client/login", follow_redirects=False)

0 commit comments

Comments
 (0)