systemd+dbus: 5-MOAD scan; systemd-0004 CWE-407 seccomp strv O(K*A), dbus-0001 CWE-407 policy optimize O(R^2)
systemd-0004: seccomp_load_syscall_filter_set() in src/shared/seccomp-util.c uses strv_contains(added, name) — O(|added|) linear scan — inside NULSTR_FOREACH over ~537 KNOWN syscalls. On x86_64 (3 arches): ~484,000 string comparisons per service start with SeccompFilter=. Sibling function seccomp_load_syscall_filter_set_raw() already uses hashmap_contains for O(1); this function was left behind. Fix: build Set* from added strv before the NULSTR_FOREACH loop. MEDIUM severity. dbus-0001: bus_client_policy_optimize() in bus/policy.c iterates R rules and for each blanket deny/allow calls remove_rules_by_type_up_to() which scans backward from current position to head — O(R^2) total per new connection creation. At R=100 rules (realistic system bus): ~10,000 comparisons per connect. Fix: single O(R) reverse pass tracking last-seen blanket per rule type. MEDIUM. dbus 5-MOAD summary: MOAD-0001: dbus-0001 DEFECT (policy optimize O(R^2)) MOAD-0002: CLEAN (BusContext is standard daemon context, not a god object) MOAD-0003: CLEAN (single-threaded event loop, no thread-local state) MOAD-0004: CLEAN (_dbus_verbose is no-op in production builds) MOAD-0005: CLEAN (pending_activations hash table coalesces duplicate requests)
This commit is contained in:
parent
a89cc53fed
commit
decacb5dbd
8 changed files with 681 additions and 0 deletions
72
defects/dbus-0001/TICKET.md
Normal file
72
defects/dbus-0001/TICKET.md
Normal file
|
|
@ -0,0 +1,72 @@
|
|||
# dbus-0001: bus_client_policy_optimize O(R^2) rule cleanup per connection
|
||||
|
||||
**MOAD:** MOAD-0001 (CWE-407)
|
||||
**Severity:** MEDIUM
|
||||
**Speedup:** ~50x at R=100 rules (per new connection creation)
|
||||
**Language:** C
|
||||
**File:** bus/policy.c:783-858 (bus_client_policy_optimize)
|
||||
|
||||
## Description
|
||||
|
||||
Every time a new client connects to dbus-daemon, `bus_policy_create_client_policy()`
|
||||
builds a per-connection rule list from default + UID + GID + mandatory policy
|
||||
rules, then calls `bus_client_policy_optimize()` to remove rules that are
|
||||
guaranteed to never fire because a later blanket deny/allow supersedes them.
|
||||
|
||||
The optimization iterates over all R rules and, for each blanket rule, calls
|
||||
`remove_rules_by_type_up_to()` which walks backward from the current position
|
||||
to the start of the list, removing earlier rules of the same type:
|
||||
|
||||
```c
|
||||
void bus_client_policy_optimize(BusClientPolicy *policy) {
|
||||
DBusList *link = _dbus_list_get_first_link(&policy->rules);
|
||||
while (link != NULL) { /* O(R) outer */
|
||||
BusPolicyRule *rule = link->data;
|
||||
...
|
||||
if (remove_preceding)
|
||||
remove_rules_by_type_up_to(policy, /* O(R) inner */
|
||||
rule->type, link);
|
||||
link = next;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`remove_rules_by_type_up_to()` scans from the head of the list to `link`,
|
||||
comparing each rule type. In the worst case (many blanket rules), this is
|
||||
O(R^2) total per connection.
|
||||
|
||||
A system bus with 100 default+uid+gid rules produces ~10,000 comparisons per
|
||||
new connection. A session bus serving a desktop environment with many services
|
||||
(GNOME, KDE) accumulates significant overhead as applications connect and
|
||||
reconnect.
|
||||
|
||||
The code itself notes this as a known issue — the FIXME comment in
|
||||
`bus_matchmaker_disconnected()` acknowledges similar O(N) scan issues.
|
||||
|
||||
## Fix
|
||||
|
||||
The optimize pass can be done in a single O(R) pass by processing rules in
|
||||
reverse order (from tail to head). When a blanket rule is encountered, mark
|
||||
all preceding same-type rules as removable in one forward scan, then do one
|
||||
cleanup pass. Alternatively, group rules by type during construction and apply
|
||||
"last blanket wins" logic without backward scanning.
|
||||
|
||||
A simpler fix: instead of walking backward to remove preceding rules, iterate
|
||||
from the tail of the list forward. Since "last rule wins", the first blanket
|
||||
encountered from the tail supersedes everything before it of the same type —
|
||||
so rules before the blanket can be dropped in O(1) by truncating the list up
|
||||
to that point.
|
||||
|
||||
Complexity drops from O(R^2) to O(R) per connection.
|
||||
|
||||
## Speedup estimate
|
||||
|
||||
At R=100 rules (realistic system bus configuration):
|
||||
- Defect: ~5,000 comparisons worst case
|
||||
- Fix: ~100 comparisons
|
||||
- Speedup: ~50x
|
||||
|
||||
At R=300 rules (large desktop session bus with many UID-specific rules):
|
||||
- Defect: ~45,000 comparisons
|
||||
- Fix: ~300 comparisons
|
||||
- Speedup: ~150x
|
||||
110
defects/dbus-0001/patch/dbus-0001-policy-optimize-o-n2.patch
Normal file
110
defects/dbus-0001/patch/dbus-0001-policy-optimize-o-n2.patch
Normal file
|
|
@ -0,0 +1,110 @@
|
|||
# UNDF: UNDF-PENDING
|
||||
--- a/bus/policy.c
|
||||
+++ b/bus/policy.c
|
||||
@@ -780,38 +780,58 @@ remove_rules_by_type_up_to (BusClientPolicy *policy,
|
||||
void
|
||||
bus_client_policy_optimize (BusClientPolicy *policy)
|
||||
{
|
||||
- DBusList *link;
|
||||
+ /* Replace O(R^2) backward-scan optimization with a single O(R) reverse pass.
|
||||
+ *
|
||||
+ * Original approach: for each blanket rule, call remove_rules_by_type_up_to()
|
||||
+ * which walks from head to the current position — O(R) inner loop, O(R^2) total.
|
||||
+ *
|
||||
+ * Fixed approach: iterate from tail to head. The first (rightmost) blanket rule
|
||||
+ * of each type supersedes all earlier rules of the same type. Track the last
|
||||
+ * seen blanket per type and remove all preceding same-type rules in one pass.
|
||||
+ */
|
||||
+ DBusList *link;
|
||||
+ dbus_bool_t seen_send_blanket = FALSE;
|
||||
+ dbus_bool_t seen_receive_blanket = FALSE;
|
||||
+ dbus_bool_t seen_own_blanket = FALSE;
|
||||
|
||||
_dbus_verbose ("Optimizing policy with %d rules\n",
|
||||
_dbus_list_get_length (&policy->rules));
|
||||
-
|
||||
- link = _dbus_list_get_first_link (&policy->rules);
|
||||
+
|
||||
+ /* Walk from tail to head. Remove any rule that is shadowed by a later
|
||||
+ * blanket rule of the same type. */
|
||||
+ link = _dbus_list_get_last_link (&policy->rules);
|
||||
while (link != NULL)
|
||||
{
|
||||
BusPolicyRule *rule;
|
||||
- DBusList *next;
|
||||
- dbus_bool_t remove_preceding;
|
||||
+ DBusList *prev;
|
||||
+ dbus_bool_t is_blanket;
|
||||
+ dbus_bool_t already_shadowed;
|
||||
|
||||
- next = _dbus_list_get_next_link (&policy->rules, link);
|
||||
+ prev = _dbus_list_get_prev_link (&policy->rules, link);
|
||||
rule = link->data;
|
||||
-
|
||||
- remove_preceding = FALSE;
|
||||
|
||||
- _dbus_assert (rule != NULL);
|
||||
-
|
||||
+ is_blanket = FALSE;
|
||||
+ already_shadowed = FALSE;
|
||||
+
|
||||
switch (rule->type)
|
||||
{
|
||||
case BUS_POLICY_RULE_SEND:
|
||||
- remove_preceding =
|
||||
- rule->d.send.message_type == DBUS_MESSAGE_TYPE_INVALID &&
|
||||
+ is_blanket = (rule->d.send.message_type == DBUS_MESSAGE_TYPE_INVALID &&
|
||||
rule->d.send.path == NULL &&
|
||||
rule->d.send.interface == NULL &&
|
||||
rule->d.send.member == NULL &&
|
||||
rule->d.send.error == NULL &&
|
||||
- rule->d.send.destination == NULL;
|
||||
+ rule->d.send.destination == NULL);
|
||||
+ already_shadowed = seen_send_blanket && !is_blanket;
|
||||
+ if (is_blanket) seen_send_blanket = TRUE;
|
||||
break;
|
||||
case BUS_POLICY_RULE_RECEIVE:
|
||||
- remove_preceding =
|
||||
- rule->d.receive.message_type == DBUS_MESSAGE_TYPE_INVALID &&
|
||||
+ is_blanket = (rule->d.receive.message_type == DBUS_MESSAGE_TYPE_INVALID &&
|
||||
rule->d.receive.path == NULL &&
|
||||
rule->d.receive.interface == NULL &&
|
||||
rule->d.receive.member == NULL &&
|
||||
rule->d.receive.error == NULL &&
|
||||
- rule->d.receive.origin == NULL;
|
||||
+ rule->d.receive.origin == NULL);
|
||||
+ already_shadowed = seen_receive_blanket && !is_blanket;
|
||||
+ if (is_blanket) seen_receive_blanket = TRUE;
|
||||
break;
|
||||
case BUS_POLICY_RULE_OWN:
|
||||
- remove_preceding =
|
||||
- rule->d.own.service_name == NULL;
|
||||
+ is_blanket = (rule->d.own.service_name == NULL);
|
||||
+ already_shadowed = seen_own_blanket && !is_blanket;
|
||||
+ if (is_blanket) seen_own_blanket = TRUE;
|
||||
break;
|
||||
|
||||
- /* The other rule types don't appear in this list */
|
||||
case BUS_POLICY_RULE_USER:
|
||||
case BUS_POLICY_RULE_GROUP:
|
||||
default:
|
||||
_dbus_assert_not_reached ("invalid rule");
|
||||
break;
|
||||
}
|
||||
|
||||
- if (remove_preceding)
|
||||
- remove_rules_by_type_up_to (policy, rule->type,
|
||||
- link);
|
||||
-
|
||||
- link = next;
|
||||
+ if (already_shadowed)
|
||||
+ {
|
||||
+ /* This specific rule can never fire — remove it in O(1). */
|
||||
+ _dbus_list_remove_link (&policy->rules, link);
|
||||
+ bus_policy_rule_unref (rule);
|
||||
+ }
|
||||
+
|
||||
+ link = prev;
|
||||
}
|
||||
|
||||
_dbus_verbose ("After optimization, policy has %d rules\n",
|
||||
173
defects/dbus-0001/test/TestDbusClientPolicyOptimize.java
Normal file
173
defects/dbus-0001/test/TestDbusClientPolicyOptimize.java
Normal file
|
|
@ -0,0 +1,173 @@
|
|||
package defects.dbus_0001;
|
||||
|
||||
import org.junit.Test;
|
||||
import static org.junit.Assert.*;
|
||||
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
* Unit test demonstrating dbus-0001 CWE-407:
|
||||
* bus_client_policy_optimize() in bus/policy.c uses O(R^2) nested list scan
|
||||
* to remove shadowed policy rules during connection creation.
|
||||
*
|
||||
* For each blanket-deny/allow rule, remove_rules_by_type_up_to() walks
|
||||
* backward through the rule list, producing O(R^2) total operations.
|
||||
*
|
||||
* Fix: single O(R) reverse pass, tracking last blanket per type.
|
||||
*/
|
||||
public class TestDbusClientPolicyOptimize {
|
||||
|
||||
enum RuleType { SEND, RECEIVE, OWN }
|
||||
|
||||
static class PolicyRule {
|
||||
RuleType type;
|
||||
boolean isBlanket; // true = no field constraints = blanket deny/allow
|
||||
|
||||
PolicyRule(RuleType type, boolean isBlanket) {
|
||||
this.type = type;
|
||||
this.isBlanket = isBlanket;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Simulates defect: O(R^2) nested scan.
|
||||
* Returns total comparisons performed.
|
||||
*/
|
||||
static long defect_optimize(LinkedList<PolicyRule> rules) {
|
||||
long comparisons = 0;
|
||||
ListIterator<PolicyRule> it = rules.listIterator();
|
||||
while (it.hasNext()) {
|
||||
PolicyRule rule = it.next();
|
||||
int pos = it.previousIndex();
|
||||
if (rule.isBlanket) {
|
||||
// remove_rules_by_type_up_to: walk from head to current pos
|
||||
ListIterator<PolicyRule> back = rules.listIterator();
|
||||
int idx = 0;
|
||||
while (back.hasNext() && idx < pos) {
|
||||
PolicyRule r = back.next();
|
||||
comparisons++; // type comparison
|
||||
if (r.type == rule.type) {
|
||||
back.remove();
|
||||
it = rules.listIterator(pos); // reset iterator after removal
|
||||
pos--;
|
||||
}
|
||||
idx++;
|
||||
}
|
||||
}
|
||||
}
|
||||
return comparisons;
|
||||
}
|
||||
|
||||
/**
|
||||
* Simulates fix: O(R) reverse pass.
|
||||
* Returns total comparisons performed.
|
||||
*/
|
||||
static long fix_optimize(LinkedList<PolicyRule> rules) {
|
||||
long comparisons = 0;
|
||||
// seen_blanket per type
|
||||
boolean[] seenBlanket = new boolean[RuleType.values().length];
|
||||
|
||||
ListIterator<PolicyRule> it = rules.listIterator(rules.size());
|
||||
while (it.hasPrevious()) {
|
||||
PolicyRule rule = it.previous();
|
||||
comparisons++; // one type check per rule
|
||||
|
||||
int typeIdx = rule.type.ordinal();
|
||||
if (rule.isBlanket) {
|
||||
seenBlanket[typeIdx] = true;
|
||||
} else if (seenBlanket[typeIdx]) {
|
||||
// This rule is shadowed by a later blanket — remove O(1)
|
||||
it.remove();
|
||||
}
|
||||
}
|
||||
return comparisons;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a realistic policy rule list: interleaved specific rules and
|
||||
* blanket denies of each type.
|
||||
*/
|
||||
static LinkedList<PolicyRule> buildRules(int rulesPerType, int blanketsPerType) {
|
||||
LinkedList<PolicyRule> rules = new LinkedList<>();
|
||||
for (RuleType type : RuleType.values()) {
|
||||
for (int i = 0; i < rulesPerType; i++) {
|
||||
rules.add(new PolicyRule(type, false)); // specific rule
|
||||
if (i % (rulesPerType / Math.max(blanketsPerType, 1)) == 0) {
|
||||
rules.add(new PolicyRule(type, true)); // blanket rule
|
||||
}
|
||||
}
|
||||
}
|
||||
return rules;
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testRealisticSystemBus() {
|
||||
// ~100 rules across 3 types, with ~5 blanket rules each
|
||||
LinkedList<PolicyRule> defect_rules = buildRules(30, 5);
|
||||
LinkedList<PolicyRule> fix_rules = new LinkedList<>(defect_rules);
|
||||
|
||||
long defect_ops = defect_optimize(defect_rules);
|
||||
long fix_ops = fix_optimize(fix_rules);
|
||||
|
||||
System.out.printf("Realistic (R~%d): defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
defect_rules.size() + fix_rules.size(),
|
||||
defect_ops, fix_ops, (double) defect_ops / fix_ops);
|
||||
|
||||
assertTrue("Fix should be faster than defect", fix_ops <= defect_ops);
|
||||
assertTrue("Fix should have fewer ops at realistic scale", fix_ops * 5 < defect_ops);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testLargeDesktopSessionBus() {
|
||||
// ~300 rules (GNOME/KDE session bus with many UID-specific rules)
|
||||
LinkedList<PolicyRule> defect_rules = buildRules(90, 10);
|
||||
LinkedList<PolicyRule> fix_rules = new LinkedList<>(defect_rules);
|
||||
|
||||
long defect_ops = defect_optimize(defect_rules);
|
||||
long fix_ops = fix_optimize(fix_rules);
|
||||
|
||||
double speedup = (double) defect_ops / fix_ops;
|
||||
System.out.printf("Large desktop (R~%d): defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
defect_rules.size(),
|
||||
defect_ops, fix_ops, speedup);
|
||||
|
||||
assertTrue("Speedup should be > 10x at R=300", speedup > 10.0);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testWorstCaseAllBlankets() {
|
||||
// Worst case: alternating specific and blanket rules of same type
|
||||
LinkedList<PolicyRule> defect_rules = new LinkedList<>();
|
||||
LinkedList<PolicyRule> fix_rules = new LinkedList<>();
|
||||
for (int i = 0; i < 50; i++) {
|
||||
PolicyRule specific = new PolicyRule(RuleType.SEND, false);
|
||||
PolicyRule blanket = new PolicyRule(RuleType.SEND, true);
|
||||
defect_rules.add(specific); defect_rules.add(blanket);
|
||||
fix_rules.add(specific); fix_rules.add(blanket);
|
||||
}
|
||||
|
||||
long defect_ops = defect_optimize(defect_rules);
|
||||
long fix_ops = fix_optimize(fix_rules);
|
||||
|
||||
double speedup = (double) defect_ops / Math.max(fix_ops, 1);
|
||||
System.out.printf("Worst case (R=100, all-blankets): defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
100, defect_ops, fix_ops, speedup);
|
||||
|
||||
assertTrue("Defect must be more expensive in worst case", defect_ops > fix_ops);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSingleConnection() {
|
||||
// Minimum case: even 1 blanket + 1 specific triggers the defect
|
||||
LinkedList<PolicyRule> rules = new LinkedList<>();
|
||||
rules.add(new PolicyRule(RuleType.SEND, false));
|
||||
rules.add(new PolicyRule(RuleType.SEND, true));
|
||||
|
||||
LinkedList<PolicyRule> fix_rules = new LinkedList<>(rules);
|
||||
long defect_ops = defect_optimize(rules);
|
||||
long fix_ops = fix_optimize(fix_rules);
|
||||
|
||||
// Defect does 1 comparison; fix does 2 checks (same for tiny case)
|
||||
assertTrue("Both paths complete without error", defect_ops >= 0 && fix_ops >= 0);
|
||||
}
|
||||
}
|
||||
55
defects/dbus/patch/SCAN.md
Normal file
55
defects/dbus/patch/SCAN.md
Normal file
|
|
@ -0,0 +1,55 @@
|
|||
# dbus 5-MOAD scan — 2026-04-03
|
||||
|
||||
Source: https://gitlab.freedesktop.org/dbus/dbus (depth=1)
|
||||
Focus: bus/ (message dispatch, activation, policy), dbus/ (auth, protocol)
|
||||
|
||||
## MOAD-0001 (CWE-407): DEFECT — dbus-0001
|
||||
|
||||
`bus/policy.c` `bus_client_policy_optimize()` runs on every new connection to
|
||||
eliminate shadowed policy rules. For each blanket-deny/allow rule (one with no
|
||||
field constraints), it calls `remove_rules_by_type_up_to()` which walks the
|
||||
full rules list from the beginning up to the current position. Total complexity
|
||||
is O(R^2) where R = total rules in the client policy.
|
||||
|
||||
A system bus configuration with 100 rules per connection produces ~10,000
|
||||
comparisons per new connection. Under a workload that creates and destroys many
|
||||
connections (container start/stop, session bus churn), this accumulates.
|
||||
|
||||
See `dbus-0001-policy-optimize-o-n2.patch` and `dbus-0001-TICKET.md` for full
|
||||
analysis.
|
||||
|
||||
Other CWE-407 candidates reviewed:
|
||||
- `bus/signals.c` match rule dispatch: uses hash table indexed by
|
||||
(message_type, interface) — pre-filtered, O(1) dispatch. CLEAN.
|
||||
- `bus/activation.c` service activation dedup: uses `pending_activations`
|
||||
hash table to coalesce duplicate requests. CLEAN.
|
||||
- `bus/config-parser.c` service_dirs_find_dir: O(D^2) during config load but
|
||||
D is small (< 20 dirs). LOW severity, startup only. CLEAN.
|
||||
- `bus/services.c` owner list scan: O(N) per ownership transfer. N = number
|
||||
of owners of a single service name, always very small. CLEAN.
|
||||
|
||||
## MOAD-0002 (Intertangle): CLEAN
|
||||
|
||||
`BusContext` in `bus/bus.h` is a well-scoped daemon context object holding
|
||||
all bus state. This is the standard single-daemon pattern, not a god object
|
||||
spanning independent subsystems. The sub-objects (BusPolicy, BusActivation,
|
||||
BusRegistry, BusMatchmaker, BusConnections) have clean interfaces. CLEAN.
|
||||
|
||||
## MOAD-0003 (Leaked Context): CLEAN
|
||||
|
||||
dbus-daemon is single-threaded (main loop in `bus/main.c` using a custom
|
||||
event loop). No `pthread_getspecific`, `thread_local`, or `__thread` in
|
||||
`bus/`. CLEAN.
|
||||
|
||||
## MOAD-0004 (CWE-312 Logged Secret): CLEAN
|
||||
|
||||
All SASL auth logging (`dbus-auth.c`) uses `_dbus_verbose()` which compiles
|
||||
to a no-op unless `DBUS_ENABLE_VERBOSE_MODE` is set at build time. Production
|
||||
dbus binaries do not enable this flag. The `dbus-daemon-launch-helper` and
|
||||
activation paths log only service names and error codes. CLEAN.
|
||||
|
||||
## MOAD-0005 (Thundering Herd): CLEAN
|
||||
|
||||
dbus-daemon is single-threaded. Service activation uses `pending_activations`
|
||||
hash table to coalesce multiple concurrent activation requests for the same
|
||||
service into a single launch, preventing herd behavior. CLEAN.
|
||||
72
defects/systemd-0004/TICKET.md
Normal file
72
defects/systemd-0004/TICKET.md
Normal file
|
|
@ -0,0 +1,72 @@
|
|||
# systemd-0004: seccomp-util.c strv_contains in KNOWN syscall scan O(K*A)
|
||||
|
||||
**MOAD:** MOAD-0001 (CWE-407)
|
||||
**Severity:** MEDIUM
|
||||
**Speedup:** ~150x at K=537, A=300 (per architecture per service start)
|
||||
**Language:** C
|
||||
**File:** src/shared/seccomp-util.c:1195-1210
|
||||
|
||||
## Description
|
||||
|
||||
`seccomp_load_syscall_filter_set()` builds a `char **added` string vector (strv)
|
||||
containing every syscall name that was already handled by the requested filter
|
||||
set. After processing the filter set it iterates over all ~537 syscalls in the
|
||||
`@known` set and, for each one, calls `strv_contains(added, name)` to check
|
||||
whether that syscall was already covered:
|
||||
|
||||
```c
|
||||
SECCOMP_FOREACH_LOCAL_ARCH(arch) {
|
||||
_cleanup_strv_free_ char **added = NULL;
|
||||
|
||||
r = add_syscall_filter_set(seccomp, set, action, NULL, log_missing, &added);
|
||||
...
|
||||
NULSTR_FOREACH(name, syscall_filter_sets[SYSCALL_FILTER_SET_KNOWN].value) {
|
||||
...
|
||||
if (strv_contains(added, name)) /* O(|added|) linear scan */
|
||||
continue;
|
||||
...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`strv_contains()` performs a linear scan. The `added` strv can hold 200-400
|
||||
entries depending on which filter set is loaded. On x86\_64 systemd iterates 3
|
||||
architectures (x86, x32, x86\_64). Total comparisons:
|
||||
|
||||
```
|
||||
3 arches * 537 KNOWN * ~300 added = ~484,000 string comparisons per service start
|
||||
```
|
||||
|
||||
The sibling function `seccomp_load_syscall_filter_set_raw()` (line 1246)
|
||||
already uses `hashmap_contains(filter, ...)` for O(1) lookup, confirming that
|
||||
O(1) lookup is the intended pattern. This function was never updated to match.
|
||||
|
||||
Every systemd service unit that uses `SystemCallFilter=` triggers this path
|
||||
on service activation. Container environments starting many services
|
||||
simultaneously amplify the impact.
|
||||
|
||||
## Fix
|
||||
|
||||
Before the `NULSTR_FOREACH` loop, build a `Set *added_set` from `added` strv
|
||||
entries. Use `set_contains(added_set, name)` instead of `strv_contains(added,
|
||||
name)`. This mirrors the `hashmap_contains` pattern already used in the raw
|
||||
variant.
|
||||
|
||||
```c
|
||||
/* Build O(1) lookup set from added strv */
|
||||
_cleanup_set_free_ Set *added_set = NULL;
|
||||
STRV_FOREACH(n, added) {
|
||||
r = set_put_strdup(&added_set, *n);
|
||||
if (r < 0)
|
||||
return log_oom();
|
||||
}
|
||||
|
||||
NULSTR_FOREACH(name, syscall_filter_sets[SYSCALL_FILTER_SET_KNOWN].value) {
|
||||
...
|
||||
if (set_contains(added_set, name)) /* O(1) */
|
||||
continue;
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
Complexity drops from O(K * A) to O(K + A) per architecture.
|
||||
38
defects/systemd-0004/patch/SCAN.md
Normal file
38
defects/systemd-0004/patch/SCAN.md
Normal file
|
|
@ -0,0 +1,38 @@
|
|||
# systemd deeper scan — 2026-04-03
|
||||
|
||||
Source: https://github.com/systemd/systemd (depth=1)
|
||||
Focus: src/shared/seccomp-util.c (missed in 2026-03-31 scan)
|
||||
|
||||
Prior defects:
|
||||
- systemd-0001: strv_extend_strv dedup O(N^2) (src/basic/strv.c) — HIGH
|
||||
- systemd-0002: unit_file_get_list states filter O(U*S) (src/shared/install.c) — MEDIUM
|
||||
- systemd-0003: dbus-cgroup BPF filter strv dedup O(N^2) (src/core/dbus-cgroup.c) — LOW-MEDIUM
|
||||
|
||||
## MOAD-0001 (CWE-407): DEFECT — systemd-0004
|
||||
|
||||
`src/shared/seccomp-util.c` `seccomp_load_syscall_filter_set()`:
|
||||
|
||||
After processing the requested filter set (building `char **added` strv of
|
||||
covered syscall names), the function iterates all ~537 KNOWN syscalls and for
|
||||
each calls `strv_contains(added, name)` which is an O(|added|) linear scan.
|
||||
|
||||
This produces O(K * A) comparisons where:
|
||||
- K = 537 KNOWN syscalls on x86\_64
|
||||
- A = filter set size (200-400 for common sets like @default, @system-service)
|
||||
|
||||
On x86\_64 systemd processes 3 architectures (x86, x32, x86\_64):
|
||||
```
|
||||
3 * 537 * ~300 = ~484,000 string comparisons per service activation with SeccompFilter=
|
||||
```
|
||||
|
||||
The sibling function `seccomp_load_syscall_filter_set_raw()` already uses
|
||||
`hashmap_contains(filter, id)` for O(1) lookup. This function was left behind
|
||||
when the raw variant was optimized.
|
||||
|
||||
**Fix:** build a `Set *added_set` from the `added` strv before the
|
||||
`NULSTR_FOREACH` loop, then use `set_contains(added_set, name)` for O(1).
|
||||
Complexity drops from O(K*A) to O(K+A) per architecture.
|
||||
|
||||
See `systemd-0004-seccomp-strv-to-set.patch` and `TICKET.md`.
|
||||
|
||||
## MOADs 0002-0005: carried from 2026-03-31 scan (all CLEAN)
|
||||
|
|
@ -0,0 +1,44 @@
|
|||
# UNDF: UNDF-PENDING
|
||||
--- a/src/shared/seccomp-util.c
|
||||
+++ b/src/shared/seccomp-util.c
|
||||
@@ -1178,7 +1178,7 @@ int seccomp_load_syscall_filter_set(uint32_t default_action, const SyscallFilter
|
||||
|
||||
SECCOMP_FOREACH_LOCAL_ARCH(arch) {
|
||||
_cleanup_(seccomp_releasep) scmp_filter_ctx seccomp = NULL;
|
||||
- _cleanup_strv_free_ char **added = NULL;
|
||||
+ _cleanup_strv_free_ char **added = NULL;
|
||||
+ _cleanup_set_free_ Set *added_set = NULL;
|
||||
|
||||
log_trace("Operating on architecture: %s", seccomp_arch_to_string(arch));
|
||||
|
||||
@@ -1188,10 +1188,20 @@ int seccomp_load_syscall_filter_set(uint32_t default_action, const SyscallFilter
|
||||
if (r < 0)
|
||||
return log_debug_errno(r, "Failed to add filter set: %m");
|
||||
|
||||
+ /* Build O(1) lookup set from the added strv to avoid O(K*A) quadratic
|
||||
+ * scan in the NULSTR_FOREACH loop below. The sibling function
|
||||
+ * seccomp_load_syscall_filter_set_raw() already uses hashmap_contains
|
||||
+ * for O(1); this brings the named-set path in line with it. */
|
||||
+ if (default_action != default_action_override) {
|
||||
+ char **n;
|
||||
+ STRV_FOREACH(n, added) {
|
||||
+ r = set_put_strdup(&added_set, *n);
|
||||
+ if (r < 0)
|
||||
+ return log_oom();
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
if (default_action != default_action_override)
|
||||
NULSTR_FOREACH(name, syscall_filter_sets[SYSCALL_FILTER_SET_KNOWN].value) {
|
||||
int id;
|
||||
|
||||
id = sym_seccomp_syscall_resolve_name(name);
|
||||
if (id < 0)
|
||||
continue;
|
||||
|
||||
/* Ignore the syscall if it was already handled above */
|
||||
- if (strv_contains(added, name))
|
||||
+ if (set_contains(added_set, name))
|
||||
continue;
|
||||
|
||||
r = sym_seccomp_rule_add_exact(seccomp, default_action, id, 0);
|
||||
117
defects/systemd-0004/test/TestSeccompStrvToSet.java
Normal file
117
defects/systemd-0004/test/TestSeccompStrvToSet.java
Normal file
|
|
@ -0,0 +1,117 @@
|
|||
package defects.systemd_0004;
|
||||
|
||||
import org.junit.Test;
|
||||
import static org.junit.Assert.*;
|
||||
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
* Unit test demonstrating systemd-0004 CWE-407:
|
||||
* seccomp_load_syscall_filter_set() uses strv_contains (O(N) linear scan)
|
||||
* inside NULSTR_FOREACH over ~537 KNOWN syscalls, producing O(K*A) behavior.
|
||||
*
|
||||
* The sibling function already uses hashmap/set for O(1) lookup.
|
||||
* This test proves O(K*A) vs O(K+A) operation count at realistic scales.
|
||||
*/
|
||||
public class TestSeccompStrvToSet {
|
||||
|
||||
/** Simulates strv_contains: linear scan of a list for a name. */
|
||||
static int strv_contains(List<String> added, String name) {
|
||||
for (String s : added) {
|
||||
if (s.equals(name)) return 1; // found
|
||||
}
|
||||
return 0; // not found
|
||||
}
|
||||
|
||||
/** Simulates set_contains: O(1) hash lookup. */
|
||||
static int set_contains(Set<String> added_set, String name) {
|
||||
return added_set.contains(name) ? 1 : 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Measures operation count for the O(K*A) defect vs O(K+A) fix.
|
||||
*
|
||||
* @param K number of KNOWN syscalls (e.g. 537 on x86_64)
|
||||
* @param A number of syscalls already added to filter (e.g. 300 for @default)
|
||||
*/
|
||||
static long[] measureOps(int K, int A) {
|
||||
// Build KNOWN syscall list (K entries)
|
||||
List<String> known = new ArrayList<>();
|
||||
for (int i = 0; i < K; i++) {
|
||||
known.add("sys_" + i);
|
||||
}
|
||||
|
||||
// Build added strv: first A syscalls already covered
|
||||
List<String> added_strv = new ArrayList<>();
|
||||
Set<String> added_set = new HashSet<>();
|
||||
for (int i = 0; i < A; i++) {
|
||||
added_strv.add("sys_" + i);
|
||||
added_set.add("sys_" + i);
|
||||
}
|
||||
|
||||
// Defect: O(K*A) - strv_contains inside loop over KNOWN
|
||||
long ops_defect = 0;
|
||||
for (String name : known) {
|
||||
// strv_contains does up to A comparisons
|
||||
for (String s : added_strv) {
|
||||
ops_defect++;
|
||||
if (s.equals(name)) break;
|
||||
}
|
||||
}
|
||||
|
||||
// Fix: O(K+A) - set_contains inside loop over KNOWN
|
||||
// Build set: A ops
|
||||
long ops_fix = A; // set construction
|
||||
// Loop: K * O(1) lookups
|
||||
for (String name : known) {
|
||||
ops_fix++; // one hash lookup per iteration
|
||||
if (added_set.contains(name)) continue;
|
||||
}
|
||||
|
||||
return new long[]{ops_defect, ops_fix};
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testRealisticScale() {
|
||||
// K=537 (x86_64 KNOWN syscalls), A=300 (@default filter set size)
|
||||
int K = 537, A = 300;
|
||||
long[] ops = measureOps(K, A);
|
||||
long defect = ops[0], fix = ops[1];
|
||||
|
||||
System.out.printf("K=%d, A=%d: defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
K, A, defect, fix, (double) defect / fix);
|
||||
|
||||
// The defect should be dramatically more expensive
|
||||
assertTrue("Defect should have > 10x more ops than fix at K=537, A=300",
|
||||
defect > fix * 10);
|
||||
|
||||
// Compute expected speedup
|
||||
double speedup = (double) defect / fix;
|
||||
assertTrue("Speedup should be > 50x at K=537, A=300", speedup > 50.0);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSmallScale() {
|
||||
// Even at small scale (K=100, A=50) the defect shows clearly
|
||||
long[] ops = measureOps(100, 50);
|
||||
long defect = ops[0], fix = ops[1];
|
||||
System.out.printf("K=100, A=50: defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
100, 50, defect, fix, (double) defect / fix);
|
||||
assertTrue("Defect always more expensive than fix", defect > fix);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testMultipleArches() {
|
||||
// x86_64 runs 3 architectures (x86, x32, x86_64)
|
||||
int arches = 3, K = 537, A = 300;
|
||||
long total_defect = 0, total_fix = 0;
|
||||
for (int i = 0; i < arches; i++) {
|
||||
long[] ops = measureOps(K, A);
|
||||
total_defect += ops[0];
|
||||
total_fix += ops[1];
|
||||
}
|
||||
System.out.printf("3 arches: defect=%,d ops, fix=%,d ops, speedup=%.1fx%n",
|
||||
total_defect, total_fix, (double) total_defect / total_fix);
|
||||
assertTrue("3-arch total: defect > 100x fix", total_defect > total_fix * 100);
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue