CVE-2021-3967: Other (NVD-CWE-Other)
value: Improper Access Control in GitHub repository zulip/zulip prior to 4.10.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 8.8
Published: 2022-02-26T23:15Z
Repository: https://github.com/zulip/zulip
Language: Python
Files changed: static/js/settings_account.js, zerver/tests/test_settings.py, zproject/urls.py
Commit date: 2022-02-25 14:00:52 -0800
Commit message: CVE-2021-3967: Only regenerate the API key by authing with the old key.

Diff: diff --git a/static/js/settings_account.js b/static/js/settings_account.js
index b4a6c7fe265..c65f7c54d9f 100644
--- a/static/js/settings_account.js
+++ b/static/js/settings_account.js
@@ -357,12 +357,19 @@ export function set_up() {
                 }
             });
         }
 
         $("#regenerate_api_key").on("click", (e) => {
+            const email = page_params.delivery_email;
+            const api_key = $("#api_key_value").text();
+            const authorization_header = "Basic " + btoa(`${email}:${api_key}`);
+
             channel.post({
-                url: "/json/users/me/api_key/regenerate",
+                // This endpoint is only accessible with the previous API key,
+                // via our usual HTTP Basic auth mechanism.
+                url: "/api/v1/users/me/api_key/regenerate",
+                headers: {Authorization: authorization_header},
                 success(data) {
                     $("#api_key_value").text(data.api_key);
                 },
                 error(xhr) {
                     $("#user_api_key_error").text(JSON.parse(xhr.responseText).msg).show();
diff --git a/zerver/tests/test_settings.py b/zerver/tests/test_settings.py
index 9478bdd9993..853f966e6c7 100644
--- a/zerver/tests/test_settings.py
+++ b/zerver/tests/test_settings.py
@@ -493,11 +493,21 @@ class UserChangesTest(ZulipTestCase):
         # Ensure the old API keys are in the authentication cache, so
         # that the below logic can test whether we have a cache-flushing bug.
         for api_key in old_api_keys:
             self.assertEqual(get_user_profile_by_api_key(api_key).email, email)
 
+        # First verify this endpoint is not registered in the /json/... path
+        # to prevent access with only a session.
         result = self.client_post("/json/users/me/api_key/regenerate")
+        self.assertEqual(result.status_code, 404)
+
+        # A logged-in session doesn't allow access to an /api/v1/ endpoint
+        # of course.
+        result = self.client_post("/api/v1/users/me/api_key/regenerate")
+        self.assertEqual(result.status_code, 401)
+
+        result = self.api_post(user, "/api/v1/users/me/api_key/regenerate")
         self.assert_json_success(result)
         new_api_key = result.json()["api_key"]
         self.assertNotIn(new_api_key, old_api_keys)
         user = self.example_user("hamlet")
         current_api_keys = get_all_api_keys(user)
diff --git a/zproject/urls.py b/zproject/urls.py
index 63ce28b9fe4..6a3a56b152b 100644
--- a/zproject/urls.py
+++ b/zproject/urls.py
@@ -370,11 +370,10 @@ v1_api_and_json_patterns = [
     rest_path("user_groups", GET=get_user_group),
     rest_path("user_groups/create", POST=add_user_group),
     rest_path("user_groups/<int:user_group_id>", PATCH=edit_user_group, DELETE=delete_user_group),
     rest_path("user_groups/<int:user_group_id>/members", POST=update_user_group_backend),
     # users/me -> zerver.views.user_settings
-    rest_path("users/me/api_key/regenerate", POST=regenerate_api_key),
     rest_path("users/me/avatar", POST=set_avatar_backend, DELETE=delete_avatar_backend),
     # users/me/hotspots -> zerver.views.hotspots
     rest_path(
         "users/me/hotspots",
         POST=(
@@ -722,10 +721,15 @@ v1_api_mobile_patterns = [
     # server-specific compatibility.
     path("server_settings", api_get_server_settings),
     # This json format view used by the mobile apps accepts a username
     # password/pair and returns an API key.
     path("fetch_api_key", api_fetch_api_key),
+    # The endpoint for regenerating and obtaining a new API key
+    # should only be available by authenticating with the current
+    # API key - as we consider access to the API key sensitive
+    # and just having a logged-in session should be insufficient.
+    rest_path("users/me/api_key/regenerate", POST=regenerate_api_key),
 ]
 
 # View for uploading messages from email mirror
 urls += [
     path("email_mirror_message", email_mirror_message),

Vulnerable code:                 url: "/json/users/me/api_key/regenerate",
    rest_path("users/me/api_key/regenerate", POST=regenerate_api_key),
Fixed code:             const email = page_params.delivery_email;
            const api_key = $("#api_key_value").text();
            const authorization_header = "Basic " + btoa(`${email}:${api_key}`);

                // This endpoint is only accessible with the previous API key,
                // via our usual HTTP Basic auth mechanism.
                url: "/api/v1/users/me/api_key/regenerate",
                headers: {Authorization: authorization_header},
        # First verify this endpoint is not registered in the /json/... path
        # to prevent access with only a session.
        self.assertEqual(result.status_code, 404)

        # A logged-in session doesn't allow access to an /api/v1/ endpoint
        # of course.
        result = self.client_post("/api/v1/users/me/api_key/regenerate")
        self.assertEqual(result.status_code, 401)

        result = self.api_post(user, "/api/v1/users/me/api_key/regenerate")
    # The endpoint for regenerating and obtaining a new API key
    # should only be available by authenticating with the current
    # API key - as we consider access to the API key sensitive
    # and just having a logged-in session should be insufficient.
    rest_path("users/me/api_key/regenerate", POST=regenerate_api_key),