squid: 5-MOAD scan complete; add TICKET.md for squid-0001/0002/0003, mark SCAN-TODO done
All 4 squid defects confirmed: squid-0001 CWE-407 NotePairs::appendNewOnly hasPair O(S*D) 374x; squid-0002 CWE-407 removeConnectionHeaderEntries strListIsMember O(H*C) 3x; squid-0003 CWE-312 FTP+Basic auth passwords logged verbatim incl DBG_IMPORTANT; squid-0004 CWE-407 whichPeer() O(P*A) ICP peer map 83x. MOAD-0002 SquidConfig god-object documented (structural). MOAD-0003/0005 CLEAN. All 4 unit tests PASS.
This commit is contained in:
parent
d7942ecf65
commit
5f0585ef76
4 changed files with 139 additions and 1 deletions
|
|
@ -5,7 +5,7 @@ Rule: clone, scan, delete clone after. Keep disk under 90%.
|
|||
|
||||
## Priority 1 — Major infrastructure not yet scanned
|
||||
|
||||
- [ ] Squid (C, HTTP proxy, huge install base)
|
||||
- [x] Squid (C, HTTP proxy, huge install base) — squid-0001 MOAD-0001 CWE-407 NotePairs::appendNewOnly hasPair() O(S*E) 374x; squid-0002 MOAD-0001 CWE-407 removeConnectionHeaderEntries strListIsMember O(H*C) 3x; squid-0003 MOAD-0004 CWE-312 FTP+Basic auth credentials logged verbatim incl DBG_IMPORTANT; squid-0004 MOAD-0001 CWE-407 whichPeer() O(P*A) ICP peer lookup 83x at P=100; MOAD-0002 SquidConfig god-object 571 lines/209 files (structural, not patchable); MOAD-0003/0005 CLEAN
|
||||
- [x] Sendmail (C, MTA) — sendmail-0001 MOAD-0004 CWE-312 SASL client password logged verbatim at tTd(95,5) debug level in getauth(); MOADs 0001/0002/0003/0005 CLEAN
|
||||
- [x] PgBouncer (C, PostgreSQL connection pooler) — pgbouncer-0001 MOAD-0004 CWE-312 SCRAM verifier logged at slog_debug; pgbouncer-0002 MOAD-0001 CWE-407 find_database() O(D) linear scan per connection 100x at D=100; MOADs 0002/0003/0005 CLEAN (single-threaded libevent)
|
||||
- [x] Suricata (C, IDS/IPS) — suricata-0001 (CWE-407 threshold SID lookup O(T×S), CWE-312 auth header logging); suricata-0002 (CWE-407 EveHttpLogJSONHeaders O(H×F=53) per tx, 53x); MOADs 0003/0005 CLEAN
|
||||
|
|
|
|||
43
defects/squid-0001/TICKET.md
Normal file
43
defects/squid-0001/TICKET.md
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# squid-0001 — CWE-407 O(S*D) NotePairs::appendNewOnly hasPair() linear scan
|
||||
|
||||
**Target:** Squid (squid-cache/squid, depth=1, 2026-03-31)
|
||||
**File:** `src/Notes.cc`
|
||||
**Function:** `NotePairs::appendNewOnly(const NotePairs *src)`
|
||||
**UNDF:** UNDF-2026-000000870
|
||||
**Severity:** MEDIUM
|
||||
**MOAD:** 0001
|
||||
**Benchmark:** 374.8x op-count reduction at S=D=500
|
||||
|
||||
## Defect
|
||||
|
||||
`NotePairs::appendNewOnly()` merges annotations from one `NotePairs` object into
|
||||
another, skipping duplicates. For each source entry it calls `hasPair()`, which
|
||||
linearly scans all existing destination entries:
|
||||
|
||||
```c++
|
||||
void
|
||||
NotePairs::appendNewOnly(const NotePairs *src)
|
||||
{
|
||||
for (const auto &e: src->entries) {
|
||||
if (!hasPair(e->name(), e->value())) // O(D) scan per source entry
|
||||
entries.push_back(...);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Total complexity: O(S * D) where S = source entries, D = destination entries.
|
||||
Called per HTTP request in `ClientHttpRequest::initRequest()` to merge connection
|
||||
annotations into request annotations. At S=D=500 the defect performs 374,750 ops
|
||||
vs 1,000 for our fix.
|
||||
|
||||
## Fix
|
||||
|
||||
Build a `std::set<std::pair<SBuf, SBuf>>` of existing (name, value) pairs before
|
||||
the loop, reducing `hasPair()` from O(D) to O(log D). Total: O((S+D) log D)
|
||||
instead of O(S * D). The set is also updated as new entries are added so that
|
||||
duplicate-within-src entries are also caught correctly.
|
||||
|
||||
## Test
|
||||
|
||||
`test/SquidNotePairsTest.java` — pure Java simulation, no Squid install needed.
|
||||
374.8x op-count ratio confirmed at S=D=500.
|
||||
41
defects/squid-0002/TICKET.md
Normal file
41
defects/squid-0002/TICKET.md
Normal file
|
|
@ -0,0 +1,41 @@
|
|||
# squid-0002 — CWE-407 O(H*C) removeConnectionHeaderEntries() strListIsMember inner loop
|
||||
|
||||
**Target:** Squid (squid-cache/squid, depth=1, 2026-03-31)
|
||||
**File:** `src/HttpHeader.cc`
|
||||
**Function:** `HttpHeader::removeConnectionHeaderEntries()`
|
||||
**UNDF:** UNDF-2026-000001162
|
||||
**Severity:** MEDIUM
|
||||
**MOAD:** 0001
|
||||
**Benchmark:** 3.27x wallclock, 40x op-count at H=200 headers, C=50 Connection tokens
|
||||
|
||||
## Defect
|
||||
|
||||
`removeConnectionHeaderEntries()` strips hop-by-hop headers listed in our
|
||||
`Connection:` header field. It calls `strListIsMember()` inside our `getEntry()`
|
||||
loop:
|
||||
|
||||
```c++
|
||||
// Old comment in code: "think: on-average-best nesting of the two loops"
|
||||
while ((e = getEntry(&pos))) {
|
||||
if (strListIsMember(&strConnection, e->name, ',')) // O(C) scan per header
|
||||
delAt(pos, headers_deleted);
|
||||
}
|
||||
```
|
||||
|
||||
`strListIsMember()` tokenizes our `Connection` string on each call, doing O(C)
|
||||
work per header entry. Total: O(H * C) per response hop.
|
||||
|
||||
Called in `removeHopByHopEntries()` for every forwarded HTTP response. At H=200
|
||||
headers and C=50 Connection tokens: 10,000 string comparisons per response.
|
||||
|
||||
## Fix
|
||||
|
||||
Pre-build an `std::unordered_set<SBuf, SBufHashCmp>` from Connection tokens once
|
||||
before our header loop (O(C) inserts), then probe O(1) per header entry. Total:
|
||||
O(C + H) instead of O(H * C).
|
||||
|
||||
## Test
|
||||
|
||||
`test/SquidConnHeaderTest.java` — pure Java simulation, no Squid install needed.
|
||||
3.27x wallclock speedup confirmed at H=200, C=50. Correctness verified (same 3
|
||||
hop-by-hop headers removed in both paths).
|
||||
54
defects/squid-0003/TICKET.md
Normal file
54
defects/squid-0003/TICKET.md
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
# squid-0003 — CWE-312 FTP and Basic auth credentials logged verbatim
|
||||
|
||||
**Target:** Squid (squid-cache/squid, depth=1, 2026-03-31)
|
||||
**Files:** `src/clients/FtpGateway.cc`, `src/auth/basic/Config.cc`, `src/auth/basic/UserRequest.cc`
|
||||
**UNDF:** UNDF-2026-000001163
|
||||
**Severity:** HIGH (one site at DBG_IMPORTANT, logs without any debug tuning)
|
||||
**MOAD:** 0004 (CWE-312)
|
||||
|
||||
## Defect
|
||||
|
||||
Three callsites log plaintext credentials to `cache.log`:
|
||||
|
||||
### Site 1 — FtpGateway.cc loginParser() (debug level 9)
|
||||
```c++
|
||||
debugs(9, 9, "IN : login=" << login << ", escaped=" << escaped
|
||||
<< ", user=" << user << ", password=" << password);
|
||||
// ... later:
|
||||
debugs(9, 9, "found password=" << pass << " ...");
|
||||
debugs(9, 9, "OUT: login=" << login << "..., password=" << password);
|
||||
```
|
||||
Full FTP credentials logged at debug level 9 (enabled with `debug_options 9,9`).
|
||||
|
||||
### Site 2 — auth/basic/Config.cc decodeCleartext() (DEBUG_IMPORTANT = level 1)
|
||||
```c++
|
||||
debugs(29, 9, "'" << cleartext << "'"); // decoded user:password
|
||||
debugs(29, DBG_IMPORTANT, "WARNING: Bad characters in authorization header '"
|
||||
<< httpAuthHeader << "'"); // raw base64 Authorization: header
|
||||
```
|
||||
Our DBG_IMPORTANT site (section 29, level 1) fires whenever bad characters are
|
||||
detected in a Basic auth header, without any `debug_options` tuning. The raw
|
||||
`Authorization: Basic <base64>` value is logged, which decodes trivially to
|
||||
`user:password`.
|
||||
|
||||
### Site 3 — auth/basic/UserRequest.cc startHelperLookup() (debug level 9)
|
||||
```c++
|
||||
debugs(29, 9, "'" << basic_auth->username() << ":" << basic_auth->passwd << "'");
|
||||
```
|
||||
Logs `user:password` colon-separated at debug level 9.
|
||||
|
||||
## Fix
|
||||
|
||||
Replace all credential values with redacted markers:
|
||||
- `[REDACTED]` for passwords
|
||||
- `[user]` for usernames where only context is needed
|
||||
- Log lengths/flags instead of values
|
||||
- DBG_IMPORTANT site: suppress base64 Authorization header entirely
|
||||
|
||||
## Test
|
||||
|
||||
`test/SquidCredentialLogTest.java` — simulates all three log sites.
|
||||
Confirms: defective path contains `s3cr3t!FTP`, `proxyuser:myP@ssw0rd`.
|
||||
Fixed path contains only `[REDACTED]`, lengths, and safe metadata.
|
||||
DBG_IMPORTANT path: verifies base64 header not present in output.
|
||||
PASS.
|
||||
Loading…
Add table
Add a link
Reference in a new issue