90 lines
4 KiB
Diff
90 lines
4 KiB
Diff
From 0000000 Mon Sep 17 00:00:00 2001
|
|
Subject: [PATCH] acl: replace selector->patterns/channels lists with dicts for O(1) dedup
|
|
|
|
CWE-407: ACLSetSelector performs O(P) listSearchKey to deduplicate key
|
|
patterns when adding each ~<pattern> rule. When N rules are applied in
|
|
sequence (e.g., ACL SETUSER user ~p1 ~p2 ... ~pN), total cost is
|
|
O(1+2+...+N) = O(N²). Same defect applies to &<channel> rules using
|
|
selector->channels.
|
|
|
|
Trigger: ACL SETUSER myuser ~key:1 ~key:2 ... ~key:N
|
|
ACL SETUSER myuser &ch:1 &ch:2 ... &ch:N
|
|
ACL file load with many per-selector key/channel rules
|
|
|
|
Severity: MEDIUM
|
|
- Directly controllable by any client with ACL SETUSER permission
|
|
- N=500 key patterns → 125,000 comparisons per SETUSER call
|
|
- N=1000 → 500,000 comparisons
|
|
|
|
Affected files: src/acl.c:1103 (listSearchKey selector->patterns)
|
|
src/acl.c:1122 (listSearchKey selector->channels)
|
|
|
|
Fix: replace selector->patterns and selector->channels lists with dicts
|
|
(hash tables) during the dedup phase of ACLSetSelector. The existing
|
|
list encoding is preserved for iteration / serialization by converting
|
|
back to a list for ACLDescribeUser and related consumers.
|
|
|
|
Alternatively (smaller diff): replace listSearchKey with dictFind using
|
|
a sds-keyed dict maintained in parallel.
|
|
|
|
Simpler targeted fix shown below: build a temporary hash set during
|
|
ACLSetSelector and fall back to O(1) lookup.
|
|
|
|
--- a/src/acl.c
|
|
+++ b/src/acl.c
|
|
@@ -349,6 +349,10 @@ aclSelector *ACLCreateSelector(int flags) {
|
|
selector->patterns = listCreate();
|
|
selector->channels = listCreate();
|
|
|
|
+ /* CWE-407 fix: parallel hash sets for O(1) dedup during ACLSetSelector */
|
|
+ selector->patterns_ht = dictCreate(&sdsReplyDictType);
|
|
+ selector->channels_ht = dictCreate(&sdsReplyDictType);
|
|
+
|
|
listSetMatchMethod(selector->patterns,ACLListMatchKeyPattern);
|
|
listSetFreeMethod(selector->patterns,ACLListFreeKeyPattern);
|
|
listSetDupMethod(selector->patterns,ACLListDupKeyPattern);
|
|
@@ -366,6 +370,8 @@ void ACLFreeSelector(aclSelector *selector) {
|
|
listRelease(selector->patterns);
|
|
listRelease(selector->channels);
|
|
|
|
+ dictRelease(selector->patterns_ht);
|
|
+ dictRelease(selector->channels_ht);
|
|
+
|
|
zfree(selector);
|
|
}
|
|
|
|
@@ -1099,10 +1103,12 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
|
|
keyPattern *newpat = ACLKeyPatternCreate(sdsnewlen(op+offset,oplen-offset), flags);
|
|
- listNode *ln = listSearchKey(selector->patterns,newpat);
|
|
- /* Avoid re-adding the same key pattern multiple times. */
|
|
- if (ln == NULL) {
|
|
+ /* CWE-407 fix: O(1) dict lookup instead of O(P) listSearchKey */
|
|
+ dictEntry *de = dictFind(selector->patterns_ht, newpat->pattern);
|
|
+ if (de == NULL) {
|
|
listAddNodeTail(selector->patterns,newpat);
|
|
+ dictAdd(selector->patterns_ht, sdsdup(newpat->pattern), newpat);
|
|
} else {
|
|
- ((keyPattern *)listNodeValue(ln))->flags |= flags;
|
|
+ ((keyPattern *)dictGetVal(de))->flags |= flags;
|
|
ACLKeyPatternFree(newpat);
|
|
}
|
|
selector->flags &= ~SELECTOR_FLAG_ALLKEYS;
|
|
@@ -1117,10 +1123,12 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
|
|
sds newpat = sdsnewlen(op+1,oplen-1);
|
|
- listNode *ln = listSearchKey(selector->channels,newpat);
|
|
- /* Avoid re-adding the same channel pattern multiple times. */
|
|
- if (ln == NULL)
|
|
+ /* CWE-407 fix: O(1) dict lookup instead of O(C) listSearchKey */
|
|
+ if (dictFind(selector->channels_ht, newpat) == NULL) {
|
|
listAddNodeTail(selector->channels,newpat);
|
|
- else
|
|
+ dictAdd(selector->channels_ht, sdsdup(newpat), NULL);
|
|
+ } else {
|
|
sdsfree(newpat);
|
|
+ }
|
|
selector->flags &= ~SELECTOR_FLAG_ALLCHANNELS;
|
|
|
|
Speedup: O(N²) → O(N). N=500 patterns: 125,000 → 500 comparisons (250x).
|
|
N=1000 patterns: 500,000 → 1,000 comparisons (500x).
|
|
|
|
Also applies to Valkey (same code, src/acl.c:1217 and :1236).
|