16218 SEC Fix 2FA bypass via RestAPI

CMK-18988

Change-Id: I11d746709c614fb21aee578229b274487f182731
(cherry picked from commit 371de1d)

.werks/16218.md

@@ -0,0 +1,25 @@
+[//]: # (werk v2)
+# Fix 2FA bypass via RestAPI
+
+key        | value
+---------- | ---
+date       | 2024-09-09T09:29:31+00:00
+version    | 2.3.0p16
+class      | security
+edition    | cre
+component  | wato
+level      | 1
+compatible | yes
+
+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*:
+
+* 2.3.0
+* 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`.

cmk/gui/session.py

@@ -241,6 +241,15 @@ def _flash_message(self, msg_type: MSG_TYPET, message: str) -> None:
         if tuple_to_add not in self.session_info.flashes:
             self.session_info.flashes.append(tuple_to_add)
 
+    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

cmk/gui/wsgi/applications/rest_api.py

@@ -368,6 +368,8 @@ def _ensure_authenticated() -> 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.")
 
 
 class CheckmkRESTAPI(AbstractWSGIApp):

cmk/gui/wsgi/applications/utils.py

@@ -73,11 +73,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, []))
                 )

tests/code_quality/test_werks.py

@@ -111,7 +111,7 @@ def test_werk_versions_after_tagged(precompiled_werks: None) -> None:
 
         # Some werks were added after the version was released. Mostly they were forgotten by
         # the developer. Consider it a hall of shame ;)
-        if werk_id in {10062, 10063, 10064, 10125, 12836, 16794}:
+        if werk_id in {10062, 10063, 10064, 10125, 12836, 16794, 16218}:
             continue
 
         tag_name = "v%s" % werk.version

tests/integration/cmk/gui/test_login.py

@@ -10,6 +10,8 @@
 from tests.testlib.pytest_helpers.marks import skip_if_saas_edition
 from tests.testlib.site import Site
 
+from cmk.gui.type_defs import TotpCredential, TwoFactorCredentials
+
 
 @skip_if_saas_edition
 def test_login_and_logout(site: Site) -> None:
@@ -259,7 +261,11 @@ def test_failed_login_counter_human(site: Site) -> None:
 
         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,
         )
 
@@ -342,3 +348,80 @@ def test_local_secret_permissions(site: Site) -> None:
     )
     assert response.status_code == 200
     assert isinstance(response.json()["lifetime_in_months"], int)
+
+
+@contextlib.contextmanager
+def enable_2fa(site: Site, username: str) -> Iterator[None]:
+    """enables totp as second factor for username
+
+    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={},
+                backup_codes=[],
+                totp_credentials={
+                    "foo": TotpCredential(
+                        credential_id="foo",
+                        secret=b"\0",
+                        version=1,
+                        registered_at=0,
+                        alias="alias",
+                    )
+                },
+            )
+        ),
+    )
+    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()
+
+
+def test_rest_api_access_by_cookie_2fa(site: Site) -> None:
+    """login via the gui but do not complete the 2fa, the cookie must not allow you access to the
+    rest api
+
+    See: CMK-18988"""
+
+    username = "cmkadmin"
+    password = site.admin_password
+
+    with enable_2fa(site, "cmkadmin"):
+        session = CMKWebSession(site)
+        response = session.post(
+            "login.py",
+            data={
+                "filled_in": "login",
+                "_username": username,
+                "_password": password,
+                "_login": "Login",
+            },
+        )
+        assert "Enter the six-digit code from your authenticator app to log in." in response.text
+
+        response = session.get(
+            f"/{site.id}/check_mk/api/1.0/version",
+            expected_code=401,
+        )
+        assert not "site" in response.json()
+        assert not session.is_logged_in()