From 25aeb99c0f975237b5c463a3b5ac0d01cc21fe7d Mon Sep 17 00:00:00 2001 From: Maximilian Wirtz Date: Thu, 5 Sep 2024 10:55:34 +0200 Subject: [PATCH] 16218 SEC Fix 2FA bypass via RestAPI CMK-18988 Change-Id: I11d746709c614fb21aee578229b274487f182731 --- .werks/16218 | 21 ++++++++++ cmk/gui/session.py | 8 ++++ cmk/gui/wsgi/applications/rest_api.py | 2 + cmk/gui/wsgi/applications/utils.py | 6 +-- tests/integration/cmk/gui/test_login.py | 54 ++++++++++++++++++++++++- 5 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 .werks/16218 diff --git a/.werks/16218 b/.werks/16218 new file mode 100644 index 00000000000..42bb703b276 --- /dev/null +++ b/.werks/16218 @@ -0,0 +1,21 @@ +Title: Fix 2FA bypass via RestAPI +Class: security +Compatible: compat +Component: wato +Date: 1725874171 +Edition: cre +Level: 1 +Version: 2.2.0p34 + +Previous to this Werk the RestAPI did not properly check if a user that is supposed to authenticated with multiple factors indeed authenticated fully. + +This issue was found during internal review. + +Affected Versions: + +LI: 2.3.0 +LI: 2.2.0 + +Vulnerability Management: + +We have rated the issue with a CVSS Score of 9.2 High (CVSS:4.0/AV:N/AC:H/AT:N/PR:N/UI:N/VC:H/VI:H/VA:H/SC:N/SI:N/SA:N) and assigned CVE-2024-8606. diff --git a/cmk/gui/session.py b/cmk/gui/session.py index 049bb170ac2..0f84e04355d 100644 --- a/cmk/gui/session.py +++ b/cmk/gui/session.py @@ -173,6 +173,14 @@ def persist(self) -> None: def invalidate(self) -> None: self.session_info.logged_out = True + def two_factor_pending(self) -> bool: + if isinstance(self.user, (LoggedInNobody, LoggedInSuperUser)): + return False + return ( + userdb.is_two_factor_login_enabled(self.user.ident) + and not self.session_info.two_factor_completed + ) + class FileBasedSession(SessionInterface): """A "session" which loads its information from a .mk file diff --git a/cmk/gui/wsgi/applications/rest_api.py b/cmk/gui/wsgi/applications/rest_api.py index 02fdc0b9de1..8536a0a9531 100644 --- a/cmk/gui/wsgi/applications/rest_api.py +++ b/cmk/gui/wsgi/applications/rest_api.py @@ -322,6 +322,8 @@ def ensure_authenticated(persist: bool = True) -> typing.Iterator[None]: if session.session.exc: raise session.session.exc raise MKAuthException("You need to be logged in to access this resource.") + if session.session.two_factor_pending(): + raise MKAuthException("Two-factor authentication required.") yield diff --git a/cmk/gui/wsgi/applications/utils.py b/cmk/gui/wsgi/applications/utils.py index bf7c286cbca..145a43b8f8b 100644 --- a/cmk/gui/wsgi/applications/utils.py +++ b/cmk/gui/wsgi/applications/utils.py @@ -68,11 +68,7 @@ def _call_auth() -> Response: "user_webauthn_login_complete", ) - if ( - not two_factor_ok - and userdb.is_two_factor_login_enabled(user_id) - and not session.session_info.two_factor_completed - ): + if not two_factor_ok and session.two_factor_pending(): raise HTTPRedirect( "user_login_two_factor.py?_origtarget=%s" % urlencode(makeuri(request, [])) ) diff --git a/tests/integration/cmk/gui/test_login.py b/tests/integration/cmk/gui/test_login.py index 3c095a2bef9..be59d6828e7 100644 --- a/tests/integration/cmk/gui/test_login.py +++ b/tests/integration/cmk/gui/test_login.py @@ -9,6 +9,8 @@ from tests.testlib import CMKWebSession from tests.testlib.site import Site +from cmk.gui.type_defs import TwoFactorCredentials, WebAuthnCredential + def test_01_login_and_logout(site: Site) -> None: web = CMKWebSession(site) @@ -249,7 +251,11 @@ def test_failed_login_counter_human(site: Site) -> None: # Login form session.post( "login.py", - params={"_username": username, "_password": "wrong_password", "_login": "Login"}, + params={ + "_username": username, + "_password": "wrong_password", + "_login": "Login", + }, allow_redirect_to_login=True, ) @@ -283,3 +289,49 @@ def test_failed_login_counter_automation(site: Site) -> None: expected_code=401, ) assert 0 == _get_failed_logins(site, username) + + +@contextlib.contextmanager +def enable_2fa(site: Site, username: str) -> Iterator[None]: + """This will mimic 2fa. it will not work in the UI and lead to crashes... + In master/2.3 we use totp which is easier to mimic... + + Caution, this overrides any previous 2fa configs""" + + site.write_text_file( + f"var/check_mk/web/{username}/two_factor_credentials.mk", + repr( + TwoFactorCredentials( + webauthn_credentials={ + "foo": WebAuthnCredential( + credential_id="foo", + registered_at=0, + alias="foo", + credential_data=b"foo", + ) + }, + backup_codes=[], + ) + ), + ) + try: + yield + finally: + site.delete_file(f"var/check_mk/web/{username}/two_factor_credentials.mk") + + +def test_rest_api_access_with_enabled_2fa(site: Site) -> None: + """you're not supposed to access the rest api if you have 2fa enabled (except for cookie auth) + + See: CMK-18988""" + username = "cmkadmin" + password = site.admin_password + with enable_2fa(site, "cmkadmin"): + session = CMKWebSession(site) + response = session.get( + f"/{site.id}/check_mk/api/1.0/version", + auth=(username, password), + expected_code=401, + ) + assert not "site" in response.json() + assert not session.is_logged_in()