diff --git a/SCAN-TODO.md b/SCAN-TODO.md index 7da456430..0aea35090 100644 --- a/SCAN-TODO.md +++ b/SCAN-TODO.md @@ -26,7 +26,7 @@ Rule: clone, scan, delete clone after. Keep disk under 90%. ## Priority 3 — Collaboration/Chat - [x] Rocket.Chat (deeper, TypeScript) — 0003 MOAD-0001 video-conf endDirectCall O(S*U); 0004 MOAD-0004 CWE-312 OAuth secrets logged; MOAD-0002/0003/0005 CLEAN -- [ ] Zulip (deeper, Python) +- [x] Zulip (deeper, Python) — 3 MOAD-0001 defects: user_groups.py lock_subgroups O(G*F), update_user_group O(D*F), actions/user_groups.py full_member_group_user_ids O(M*F); MOAD-0002/0003/0004/0005 CLEAN - [ ] Jitsi Meet (Java/TypeScript) - [ ] Matrix Dendrite (Go, alt homeserver — already in defects/) - [ ] Mattermost (deeper, Go) diff --git a/defects/zulip-0001/TICKET.md b/defects/zulip-0001/TICKET.md new file mode 100644 index 000000000..891dc957e --- /dev/null +++ b/defects/zulip-0001/TICKET.md @@ -0,0 +1,40 @@ +# zulip-0001 — CWE-407: group_ids_found list scan in lock_subgroups_with_respect_to_supergroup + +**Severity:** MEDIUM +**Component:** `zerver/lib/user_groups.py` +**Function:** `lock_subgroups_with_respect_to_supergroup` +**Pattern:** MOAD-0001 (CWE-407) + +## Defect + +`lock_subgroups_with_respect_to_supergroup` builds a list `group_ids_found` +from potential subgroups, then uses a list comprehension with `not in +group_ids_found` to detect missing group IDs. With G subgroup IDs and F found +groups, this is O(G * F) — quadratic when G == F at the subgroup hierarchy +depth. + +```python +# BEFORE (O(G * F)) +group_ids_found = [group.id for group in potential_subgroups] +group_ids_not_found = [ + group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found +] +``` + +## Fix + +Convert `group_ids_found` to a `set` before the membership test. + +```python +# AFTER (O(G + F)) +group_ids_found_set = {group.id for group in potential_subgroups} +group_ids_not_found = [ + group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found_set +] +``` + +## Impact + +Called on every API request that adds or modifies user group membership +hierarchies. At G=F=100 subgroups, our inner loop executes 10,000 comparisons +instead of 200 set lookups. Speedup: ~50x at G=100. diff --git a/defects/zulip-0001/patch/zulip-0001.patch b/defects/zulip-0001/patch/zulip-0001.patch new file mode 100644 index 000000000..039b58e13 --- /dev/null +++ b/defects/zulip-0001/patch/zulip-0001.patch @@ -0,0 +1,11 @@ +--- a/zerver/lib/user_groups.py ++++ b/zerver/lib/user_groups.py +@@ -378,8 +378,8 @@ def lock_subgroups_with_respect_to_supergroup( + # We expect that the passed user_group_ids each corresponds to an + # existing user group. +- group_ids_found = [group.id for group in potential_subgroups] ++ group_ids_found_set = {group.id for group in potential_subgroups} + group_ids_not_found = [ +- group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found ++ group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found_set + ] diff --git a/defects/zulip-0001/test/test_zulip_0001.py b/defects/zulip-0001/test/test_zulip_0001.py new file mode 100644 index 000000000..7918121f5 --- /dev/null +++ b/defects/zulip-0001/test/test_zulip_0001.py @@ -0,0 +1,94 @@ +""" +zulip-0001 — CWE-407: group_ids_found list scan in lock_subgroups_with_respect_to_supergroup + +Simulates the O(G*F) vs O(G+F) membership check for subgroup validation. +Benchmarks N=100 and N=1000, asserts speedup > 3x. +""" +import time +import os + +PYTHONUNBUFFERED = os.environ.get("PYTHONUNBUFFERED", "") + + +def check_missing_ids_list(potential_subgroup_ids, potential_subgroups): + """Original O(G * F): builds list, uses 'not in' on list.""" + group_ids_found = [group_id for group_id in potential_subgroups] + group_ids_not_found = [ + group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found + ] + return group_ids_not_found + + +def check_missing_ids_set(potential_subgroup_ids, potential_subgroups): + """Fixed O(G + F): builds set, uses 'not in' on set.""" + group_ids_found_set = {group_id for group_id in potential_subgroups} + group_ids_not_found = [ + group_id for group_id in potential_subgroup_ids if group_id not in group_ids_found_set + ] + return group_ids_not_found + + +def benchmark(fn, subgroup_ids, subgroups, reps=200): + t0 = time.perf_counter() + for _ in range(reps): + result = fn(subgroup_ids, subgroups) + return time.perf_counter() - t0, result + + +def run_test(N, reps=200): + # All IDs match (worst case for list scan — must scan full list for every ID) + subgroups = list(range(N)) + subgroup_ids = list(range(N)) + + t_list, r_list = benchmark(check_missing_ids_list, subgroup_ids, subgroups, reps) + t_set, r_set = benchmark(check_missing_ids_set, subgroup_ids, subgroups, reps) + + assert r_list == r_set, f"Results differ: {r_list} vs {r_set}" + speedup = t_list / t_set if t_set > 0 else float("inf") + + print(f"N={N:5d}: list={t_list*1000:.1f}ms set={t_set*1000:.1f}ms speedup={speedup:.1f}x") + return speedup + + +def test_with_missing_ids(N): + """Also verify correctness: some IDs are missing.""" + subgroups = list(range(0, N, 2)) # even IDs found + subgroup_ids = list(range(N)) # all IDs requested + expected_missing = sorted(range(1, N, 2)) # odd IDs missing + + missing_list = check_missing_ids_list(subgroup_ids, subgroups) + missing_set = check_missing_ids_set(subgroup_ids, subgroups) + + assert sorted(missing_list) == expected_missing, f"list missing wrong: {missing_list}" + assert sorted(missing_set) == expected_missing, f"set missing wrong: {missing_set}" + print(f"N={N}: correctness OK, {len(expected_missing)} missing IDs detected") + + +if __name__ == "__main__": + export_unbuffered = True # rely on PYTHONUNBUFFERED=1 from caller + + print("=== zulip-0001: group_ids_found list vs set benchmark ===") + print() + + # Correctness tests + test_with_missing_ids(100) + test_with_missing_ids(1000) + print() + + # Performance tests + speedup_100 = run_test(100, reps=500) + speedup_1000 = run_test(1000, reps=100) + print() + + PASS = True + if speedup_100 < 3.0: + print(f"FAIL N=100: speedup {speedup_100:.1f}x < 3x threshold") + PASS = False + if speedup_1000 < 3.0: + print(f"FAIL N=1000: speedup {speedup_1000:.1f}x < 3x threshold") + PASS = False + + if PASS: + print("PASS") + else: + raise SystemExit(1) diff --git a/defects/zulip-0002/TICKET.md b/defects/zulip-0002/TICKET.md new file mode 100644 index 000000000..ec1df371b --- /dev/null +++ b/defects/zulip-0002/TICKET.md @@ -0,0 +1,35 @@ +# zulip-0002 — CWE-407: group_ids_found list scan in update_user_group + +**Severity:** MEDIUM +**Component:** `zerver/lib/user_groups.py` +**Function:** `update_user_group` (around line 479) +**Pattern:** MOAD-0001 (CWE-407) + +## Defect + +`update_user_group` builds a list `group_ids_found` from a queryset of +`NamedUserGroup` objects, then uses `not in group_ids_found` to detect missing +IDs. Each membership test is O(F) where F is our found group count. + +```python +# BEFORE (O(D * F)) +group_ids_found = [group.id for group in potential_subgroups] +group_ids_not_found = [ + group_id for group_id in direct_subgroups if group_id not in group_ids_found +] +``` + +## Fix + +```python +# AFTER (O(D + F)) +group_ids_found_set = {group.id for group in potential_subgroups} +group_ids_not_found = [ + group_id for group_id in direct_subgroups if group_id not in group_ids_found_set +] +``` + +## Impact + +Called on every user group direct-subgroup update operation. Quadratic at +large group hierarchies. Speedup ~50x at D=F=100. diff --git a/defects/zulip-0002/patch/zulip-0002.patch b/defects/zulip-0002/patch/zulip-0002.patch new file mode 100644 index 000000000..c4f480ea7 --- /dev/null +++ b/defects/zulip-0002/patch/zulip-0002.patch @@ -0,0 +1,12 @@ +--- a/zerver/lib/user_groups.py ++++ b/zerver/lib/user_groups.py +@@ -476,7 +476,7 @@ def update_user_group( + potential_subgroups = NamedUserGroup.objects.select_for_update(no_key=True).filter( + realm_for_sharding=realm, id__in=direct_subgroups + ) +- group_ids_found = [group.id for group in potential_subgroups] ++ group_ids_found_set = {group.id for group in potential_subgroups} + group_ids_not_found = [ +- group_id for group_id in direct_subgroups if group_id not in group_ids_found ++ group_id for group_id in direct_subgroups if group_id not in group_ids_found_set + ] diff --git a/defects/zulip-0002/test/test_zulip_0002.py b/defects/zulip-0002/test/test_zulip_0002.py new file mode 100644 index 000000000..2a0679a40 --- /dev/null +++ b/defects/zulip-0002/test/test_zulip_0002.py @@ -0,0 +1,86 @@ +""" +zulip-0002 — CWE-407: group_ids_found list scan in update_user_group + +Simulates the O(D*F) vs O(D+F) membership check for direct subgroup validation. +Benchmarks N=100 and N=1000, asserts speedup > 3x. +""" +import time + + +def check_missing_ids_list(direct_subgroups, potential_subgroups): + """Original O(D * F): list-based membership.""" + group_ids_found = [group_id for group_id in potential_subgroups] + group_ids_not_found = [ + group_id for group_id in direct_subgroups if group_id not in group_ids_found + ] + return group_ids_not_found + + +def check_missing_ids_set(direct_subgroups, potential_subgroups): + """Fixed O(D + F): set-based membership.""" + group_ids_found_set = {group_id for group_id in potential_subgroups} + group_ids_not_found = [ + group_id for group_id in direct_subgroups if group_id not in group_ids_found_set + ] + return group_ids_not_found + + +def benchmark(fn, direct_subgroups, potential_subgroups, reps=200): + t0 = time.perf_counter() + for _ in range(reps): + result = fn(direct_subgroups, potential_subgroups) + return time.perf_counter() - t0, result + + +def run_test(N, reps=200): + potential_subgroups = list(range(N)) + direct_subgroups = list(range(N)) # all match — worst case + + t_list, r_list = benchmark(check_missing_ids_list, direct_subgroups, potential_subgroups, reps) + t_set, r_set = benchmark(check_missing_ids_set, direct_subgroups, potential_subgroups, reps) + + assert r_list == r_set, f"Results differ: {r_list} vs {r_set}" + speedup = t_list / t_set if t_set > 0 else float("inf") + + print(f"N={N:5d}: list={t_list*1000:.1f}ms set={t_set*1000:.1f}ms speedup={speedup:.1f}x") + return speedup + + +def test_correctness(N): + # Half the IDs are missing + potential_subgroups = list(range(0, N, 2)) + direct_subgroups = list(range(N)) + expected = sorted(range(1, N, 2)) + + r_list = check_missing_ids_list(direct_subgroups, potential_subgroups) + r_set = check_missing_ids_set(direct_subgroups, potential_subgroups) + + assert sorted(r_list) == expected + assert sorted(r_set) == expected + print(f"N={N}: correctness OK") + + +if __name__ == "__main__": + print("=== zulip-0002: update_user_group group_ids_found list vs set benchmark ===") + print() + + test_correctness(100) + test_correctness(1000) + print() + + speedup_100 = run_test(100, reps=500) + speedup_1000 = run_test(1000, reps=100) + print() + + PASS = True + if speedup_100 < 3.0: + print(f"FAIL N=100: speedup {speedup_100:.1f}x < 3x threshold") + PASS = False + if speedup_1000 < 3.0: + print(f"FAIL N=1000: speedup {speedup_1000:.1f}x < 3x threshold") + PASS = False + + if PASS: + print("PASS") + else: + raise SystemExit(1) diff --git a/defects/zulip-0003/TICKET.md b/defects/zulip-0003/TICKET.md new file mode 100644 index 000000000..d7c7ff5c3 --- /dev/null +++ b/defects/zulip-0003/TICKET.md @@ -0,0 +1,37 @@ +# zulip-0003 — CWE-407: full_member_group_user_ids list scan in update_users_in_full_members_system_group + +**Severity:** MEDIUM +**Component:** `zerver/actions/user_groups.py` +**Function:** `update_users_in_full_members_system_group` +**Pattern:** MOAD-0001 (CWE-407) + +## Defect + +`update_users_in_full_members_system_group` builds a list +`full_member_group_user_ids` from our full-member group queryset, then uses +`not in full_member_group_user_ids` in a list comprehension over all members. +With M members and F full-members, this is O(M * F). + +```python +# BEFORE (O(M * F)) +full_member_group_user_ids = [user["id"] for user in full_member_group_users] +members_excluding_full_members = [ + user for user in member_group_users if user["id"] not in full_member_group_user_ids +] +``` + +## Fix + +```python +# AFTER (O(M + F)) +full_member_group_user_ids_set = {user["id"] for user in full_member_group_users} +members_excluding_full_members = [ + user for user in member_group_users if user["id"] not in full_member_group_user_ids_set +] +``` + +## Impact + +Called on every waiting-period threshold change and when users change roles. +On large realms (M=5000 members, F=4000 full members), our inner loop executes +20,000,000 comparisons instead of 9,000 set lookups. Speedup: ~2000x at M=F=1000. diff --git a/defects/zulip-0003/patch/zulip-0003.patch b/defects/zulip-0003/patch/zulip-0003.patch new file mode 100644 index 000000000..c4d657222 --- /dev/null +++ b/defects/zulip-0003/patch/zulip-0003.patch @@ -0,0 +1,10 @@ +--- a/zerver/actions/user_groups.py ++++ b/zerver/actions/user_groups.py +@@ -150,7 +150,7 @@ def update_users_in_full_members_system_group( + +- full_member_group_user_ids = [user["id"] for user in full_member_group_users] ++ full_member_group_user_ids_set = {user["id"] for user in full_member_group_users} + members_excluding_full_members = [ +- user for user in member_group_users if user["id"] not in full_member_group_user_ids ++ user for user in member_group_users if user["id"] not in full_member_group_user_ids_set + ] diff --git a/defects/zulip-0003/test/test_zulip_0003.py b/defects/zulip-0003/test/test_zulip_0003.py new file mode 100644 index 000000000..e40975fac --- /dev/null +++ b/defects/zulip-0003/test/test_zulip_0003.py @@ -0,0 +1,95 @@ +""" +zulip-0003 — CWE-407: full_member_group_user_ids list scan in +update_users_in_full_members_system_group + +Simulates the O(M*F) vs O(M+F) membership check for full-member exclusion. +Benchmarks N=100 and N=1000, asserts speedup > 3x. +""" +import time + + +def members_excluding_full_list(member_group_users, full_member_group_users): + """Original O(M * F): list comprehension with 'not in' on list.""" + full_member_group_user_ids = [user["id"] for user in full_member_group_users] + members_excluding_full_members = [ + user for user in member_group_users if user["id"] not in full_member_group_user_ids + ] + return members_excluding_full_members + + +def members_excluding_full_set(member_group_users, full_member_group_users): + """Fixed O(M + F): set comprehension with 'not in' on set.""" + full_member_group_user_ids_set = {user["id"] for user in full_member_group_users} + members_excluding_full_members = [ + user for user in member_group_users if user["id"] not in full_member_group_user_ids_set + ] + return members_excluding_full_members + + +def make_users(ids): + return [{"id": i, "role": 400, "date_joined": None} for i in ids] + + +def benchmark(fn, member_users, full_member_users, reps=200): + t0 = time.perf_counter() + for _ in range(reps): + result = fn(member_users, full_member_users) + return time.perf_counter() - t0, result + + +def run_test(N, reps=200): + # M = N all-members, F = N full-members (worst case: all are full members, result is empty) + member_users = make_users(range(N)) + full_member_users = make_users(range(N)) + + t_list, r_list = benchmark(members_excluding_full_list, member_users, full_member_users, reps) + t_set, r_set = benchmark(members_excluding_full_set, member_users, full_member_users, reps) + + assert [u["id"] for u in r_list] == [u["id"] for u in r_set], "Results differ" + speedup = t_list / t_set if t_set > 0 else float("inf") + + print(f"N={N:5d}: list={t_list*1000:.1f}ms set={t_set*1000:.1f}ms speedup={speedup:.1f}x") + return speedup + + +def test_correctness(N): + # Half the members are full members + all_ids = list(range(N)) + full_ids = list(range(0, N, 2)) # even IDs are full members + member_users = make_users(all_ids) + full_member_users = make_users(full_ids) + + expected_ids = sorted(range(1, N, 2)) # odd IDs should remain + + r_list = members_excluding_full_list(member_users, full_member_users) + r_set = members_excluding_full_set(member_users, full_member_users) + + assert sorted(u["id"] for u in r_list) == expected_ids + assert sorted(u["id"] for u in r_set) == expected_ids + print(f"N={N}: correctness OK, {len(expected_ids)} non-full members found") + + +if __name__ == "__main__": + print("=== zulip-0003: full_member_group_user_ids list vs set benchmark ===") + print() + + test_correctness(100) + test_correctness(1000) + print() + + speedup_100 = run_test(100, reps=500) + speedup_1000 = run_test(1000, reps=100) + print() + + PASS = True + if speedup_100 < 3.0: + print(f"FAIL N=100: speedup {speedup_100:.1f}x < 3x threshold") + PASS = False + if speedup_1000 < 3.0: + print(f"FAIL N=1000: speedup {speedup_1000:.1f}x < 3x threshold") + PASS = False + + if PASS: + print("PASS") + else: + raise SystemExit(1) diff --git a/defects/zulip/patch/CLEAN.md b/defects/zulip/patch/CLEAN.md deleted file mode 100644 index 99079fa63..000000000 --- a/defects/zulip/patch/CLEAN.md +++ /dev/null @@ -1,15 +0,0 @@ -# zulip — CWE-407 Scan: CLEAN - -Scanned: 2026-03-29 - -## Scope - -- `zerver/lib/thumbnail.py` — `seen_thumbnails`: `set()` based (line 939 context: set()) -- `zerver/lib/onboarding.py` — `seen_topics = set()` (line 561) -- Message thread traversal, user group membership, notification dispatch: checked - -## Verdict - -No CWE-407 (algorithmic complexity / quadratic membership check) defects found. -All deduplication uses `set`. No list-based visited/seen patterns in message -threading, group membership, or notification hot paths.