Skip to content

Commit be0e4ae

Browse files
jwag956jwag956
committed
Fix GHSA-97r5-pg8x-p63p - possible to 'verify' an account using a different users oauth credentials.
The fix is simply to verify that the email address from the oauth response matches the current user. 2 other hardening improvements: 1) make sure that the field used for identity (usually email) is configured in flask_security as a IDENTITY_ATTRIBUTE 2) use the IDENTITY_ATTRIBUTE mapper on the received value - for the common case this means the email address with be normalized and validated (using email_validator package). While it seems unlikely that an oauth provider would return a non-normalized email -its seems a good idea to treat all emails the same in the system. close #1215
1 parent 92af5c5 commit be0e4ae

3 files changed

Lines changed: 132 additions & 19 deletions

File tree

‎CHANGES.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ Features & Improvements
1515
Fixes
1616
+++++
1717
- (:issue:`1212`) Newly introduced :py:meth:`.UserMixin.is_locked` logic is inverted.
18+
- (:issue:`1215`) Fix GHSA-97r5-pg8x-p63p - possible to verify an account using a different
19+
users oauth credentials.
1820

1921
Backwards Compatibility Concerns
2022
+++++++++++++++++++++++++++++++++

‎flask_security/oauth_glue.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
config_value as cv,
4141
do_flash,
4242
login_user,
43+
get_identity_attribute,
4344
get_message,
4445
get_post_action_redirect,
4546
get_url,
@@ -104,6 +105,14 @@ def _oauth_response_common(name: str) -> tuple[t.Any, UserMixin | None] | t.NoRe
104105
field_name, field_value = oauth_provider.fetch_identity_cb(
105106
_security.oauthglue.oauth_app, token
106107
)
108+
# we only allow configured IDENTITY_ATTRIBUTES and run
109+
# the mapper (which for email will normalize it)
110+
if not (uia := get_identity_attribute(field_name)):
111+
raise OAuthError(
112+
error="Config error",
113+
description=f"Invalid identity attribute: {field_name}",
114+
)
115+
field_value = uia["mapper"](field_value)
107116
user = _security.datastore.find_user(**{field_name: field_value})
108117
return field_value, user
109118

@@ -199,7 +208,7 @@ def oauth_verify_response(name: str) -> ResponseValue:
199208
assert oauth_provider is not None
200209
return oauth_provider.oauth_response_failure("VERIFY_ERROR_VIEW", e)
201210
next_loc = session.pop("fs_oauth_next", None)
202-
if user:
211+
if user and user.email == current_user.email:
203212
# verified - so set freshness time.
204213
session["fs_paa"] = time.time()
205214
if cv("REDIRECT_BEHAVIOR") == "spa":

‎tests/test_oauthglue.py‎

Lines changed: 120 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ def authorize_redirect(self, uri):
7070
return redirect(urllib.parse.quote(redirect_url))
7171

7272

73-
class MockOAuth:
73+
class MockAuthlibFlaskClient:
7474
def __init__(self):
7575
pass
7676

@@ -83,7 +83,9 @@ def register(self, name, **kwargs):
8383
def test_github(app, sqlalchemy_datastore, get_message):
8484
CSRFProtect(app)
8585
init_app_with_options(
86-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
86+
app,
87+
sqlalchemy_datastore,
88+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
8789
)
8890
client = app.test_client()
8991
response = client.get("/login")
@@ -115,7 +117,9 @@ def test_github_nocsrf(app, sqlalchemy_datastore, get_message):
115117
# Test if ignore_unauth_endpoints is true - doesn't require CSRF
116118
CSRFProtect(app)
117119
init_app_with_options(
118-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
120+
app,
121+
sqlalchemy_datastore,
122+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
119123
)
120124
client = app.test_client()
121125
response = client.get("/login")
@@ -131,7 +135,7 @@ def myoauth_fetch_identity(oauth, token):
131135
profile = resp.json()
132136
return "email", profile["email"]
133137

134-
authlib_oauth = MockOAuth()
138+
authlib_oauth = MockAuthlibFlaskClient()
135139
authlib_oauth.register("myoauth")
136140
init_app_with_options(
137141
app, sqlalchemy_datastore, **{"security_args": {"oauth": authlib_oauth}}
@@ -157,7 +161,9 @@ def myoauth_fetch_identity(oauth, token):
157161
@pytest.mark.settings(oauth_enable=True)
158162
def test_bad_api(app, sqlalchemy_datastore, get_message):
159163
init_app_with_options(
160-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
164+
app,
165+
sqlalchemy_datastore,
166+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
161167
)
162168
client = app.test_client()
163169

@@ -186,7 +192,9 @@ def test_bad_api(app, sqlalchemy_datastore, get_message):
186192
@pytest.mark.settings(oauth_enable=True)
187193
def test_unknown_user(app, sqlalchemy_datastore, get_message):
188194
init_app_with_options(
189-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
195+
app,
196+
sqlalchemy_datastore,
197+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
190198
)
191199
client = app.test_client()
192200
oauth_app = app.security.oauthglue.oauth_app
@@ -198,7 +206,9 @@ def test_unknown_user(app, sqlalchemy_datastore, get_message):
198206
@pytest.mark.settings(oauth_enable=True)
199207
def test_disabled_user(app, sqlalchemy_datastore, get_message):
200208
init_app_with_options(
201-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
209+
app,
210+
sqlalchemy_datastore,
211+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
202212
)
203213
client = app.test_client()
204214
oauth_app = app.security.oauthglue.oauth_app
@@ -208,6 +218,44 @@ def test_disabled_user(app, sqlalchemy_datastore, get_message):
208218
assert get_message("DISABLED_ACCOUNT") in response.data # should be in flash
209219

210220

221+
@pytest.mark.settings(oauth_enable=True)
222+
def test_unicode_email(app, sqlalchemy_datastore, get_message):
223+
# Test that we normalize email response from oauth provider
224+
init_app_with_options(
225+
app,
226+
sqlalchemy_datastore,
227+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
228+
)
229+
client = app.test_client()
230+
oauth_app = app.security.oauthglue.oauth_app
231+
oauth_app.github.set_identity(
232+
"matt@\N{FULLWIDTH LATIN SMALL LETTER L}\N{FULLWIDTH LATIN SMALL LETTER P}.com"
233+
)
234+
client.get("/login/oauthresponse/github", follow_redirects=True)
235+
assert is_authenticated(client, get_message)
236+
237+
238+
@pytest.mark.settings(
239+
oauth_enable=True,
240+
USER_IDENTITY_ATTRIBUTES=[
241+
{"username": {"mapper": lambda x: x}},
242+
],
243+
)
244+
def test_email_not_identity(app, sqlalchemy_datastore, get_message):
245+
# Test that if email isn't an identity attribute - can't use oauth to log in
246+
init_app_with_options(
247+
app,
248+
sqlalchemy_datastore,
249+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
250+
)
251+
client = app.test_client()
252+
response = client.get("/login/oauthresponse/github", follow_redirects=True)
253+
assert (
254+
b": (Config error - Invalid identity attribute: email). Please try again."
255+
in response.data
256+
)
257+
258+
211259
def _locked(self, form_error):
212260
if self.email == "gal@lp.com":
213261
form_error.append("You are not allowed to do that")
@@ -219,7 +267,9 @@ def _locked(self, form_error):
219267
@pytest.mark.settings(oauth_enable=True)
220268
def test_override_user_locked(app, sqlalchemy_datastore, get_message):
221269
init_app_with_options(
222-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
270+
app,
271+
sqlalchemy_datastore,
272+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
223273
)
224274
client = app.test_client()
225275
oauth_app = app.security.oauthglue.oauth_app
@@ -233,7 +283,9 @@ def test_override_user_locked(app, sqlalchemy_datastore, get_message):
233283
@pytest.mark.settings(oauth_enable=True)
234284
def test_tf(app, sqlalchemy_datastore, get_message):
235285
init_app_with_options(
236-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
286+
app,
287+
sqlalchemy_datastore,
288+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
237289
)
238290
client = app.test_client()
239291
authenticate(client)
@@ -273,7 +325,9 @@ def test_spa(app, sqlalchemy_datastore, get_message):
273325
headers = {"Accept": "application/json", "Content-Type": "application/json"}
274326

275327
init_app_with_options(
276-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
328+
app,
329+
sqlalchemy_datastore,
330+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
277331
)
278332
client = app.test_client()
279333
csrf_token = get_csrf_token(client)
@@ -325,7 +379,9 @@ def test_spa(app, sqlalchemy_datastore, get_message):
325379
def test_already_auth(app, sqlalchemy_datastore, get_message):
326380
headers = {"Accept": "application/json", "Content-Type": "application/json"}
327381
init_app_with_options(
328-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
382+
app,
383+
sqlalchemy_datastore,
384+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
329385
)
330386
client = app.test_client()
331387
authenticate(client)
@@ -346,7 +402,9 @@ def test_simple_next(app, sqlalchemy_datastore, get_message):
346402
# For oauth we stash 'next' in the session since we can't really
347403
# send it all around the oauth providers.
348404
init_app_with_options(
349-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
405+
app,
406+
sqlalchemy_datastore,
407+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
350408
)
351409
client = app.test_client()
352410
response = client.get("/profile", follow_redirects=True)
@@ -379,7 +437,9 @@ def oauth_response_failure(self, error_view, e):
379437
return redirect("/uh-oh")
380438

381439
init_app_with_options(
382-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
440+
app,
441+
sqlalchemy_datastore,
442+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
383443
)
384444
# Have to register with Oauthglue.
385445
app.security.oauthglue.register_provider_ext(MyOauthProvider("myoauth"))
@@ -410,7 +470,9 @@ def oauth_response_failure(self, error_view, e):
410470
@pytest.mark.settings(oauth_enable=True, post_verify_view="/post_verify")
411471
def test_verify(app, sqlalchemy_datastore, get_message):
412472
init_app_with_options(
413-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
473+
app,
474+
sqlalchemy_datastore,
475+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
414476
)
415477
client = app.test_client()
416478

@@ -444,7 +506,9 @@ def test_verify(app, sqlalchemy_datastore, get_message):
444506
@pytest.mark.settings(oauth_enable=True)
445507
def test_verify_unknown_user(app, sqlalchemy_datastore, get_message):
446508
init_app_with_options(
447-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
509+
app,
510+
sqlalchemy_datastore,
511+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
448512
)
449513
client = app.test_client()
450514
authenticate(client)
@@ -469,6 +533,38 @@ def test_verify_unknown_user(app, sqlalchemy_datastore, get_message):
469533
assert "fs_oauth_next" not in session
470534

471535

536+
@pytest.mark.settings(oauth_enable=True)
537+
def test_verify_evil_user(app, sqlalchemy_datastore, get_message):
538+
# Ensure a valid (evil) user can't use their oauth credentials to 'verify'
539+
# another user.
540+
init_app_with_options(
541+
app,
542+
sqlalchemy_datastore,
543+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
544+
)
545+
client = app.test_client()
546+
authenticate(client) # authenticate as matt@lp.com
547+
oauth_app = app.security.oauthglue.oauth_app
548+
oauth_app.github.set_identity("joe@lp.com") # a valid user
549+
550+
reset_fresh(client, app.config["SECURITY_FRESHNESS"])
551+
response = client.get("/fresh", follow_redirects=True)
552+
github_url = get_form_action(response, 1)
553+
response = client.post(github_url, follow_redirects=False)
554+
555+
with capture_flashes() as flashes:
556+
redirect_url = urllib.parse.urlsplit(urllib.parse.unquote(response.location))
557+
local_redirect = urllib.parse.parse_qs(redirect_url.query)["redirect_uri"][0]
558+
response = client.get(local_redirect, follow_redirects=True)
559+
assert flashes[0]["category"] == "error"
560+
assert flashes[0]["message"].encode("utf-8") == get_message(
561+
"IDENTITY_NOT_REGISTERED", id="joe@lp.com"
562+
)
563+
assert b"Reauthenticate" in response.data
564+
session = get_session(response)
565+
assert "fs_oauth_next" not in session
566+
567+
472568
@pytest.mark.settings(
473569
oauth_enable=True,
474570
oauth_builtin_providers=["github"],
@@ -479,7 +575,9 @@ def test_verify_unknown_user(app, sqlalchemy_datastore, get_message):
479575
def test_verify_spa(app, sqlalchemy_datastore, get_message):
480576
headers = {"Accept": "application/json", "Content-Type": "application/json"}
481577
init_app_with_options(
482-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
578+
app,
579+
sqlalchemy_datastore,
580+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
483581
)
484582
client = app.test_client()
485583
json_authenticate(client)
@@ -506,7 +604,9 @@ def test_verify_spa(app, sqlalchemy_datastore, get_message):
506604
)
507605
def test_verify_unknown_user_spa(app, sqlalchemy_datastore, get_message):
508606
init_app_with_options(
509-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
607+
app,
608+
sqlalchemy_datastore,
609+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
510610
)
511611
client = app.test_client()
512612
json_authenticate(client)
@@ -526,7 +626,9 @@ def test_verify_unknown_user_spa(app, sqlalchemy_datastore, get_message):
526626
)
527627
def test_verify_spa_exc(app, sqlalchemy_datastore, get_message):
528628
init_app_with_options(
529-
app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}}
629+
app,
630+
sqlalchemy_datastore,
631+
**{"security_args": {"oauth": MockAuthlibFlaskClient()}},
530632
)
531633
client = app.test_client()
532634
json_authenticate(client)

0 commit comments

Comments
 (0)