From 6b80a872704d87d70dbb6a57ab8a6500084c0a0c Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Tue, 31 Mar 2026 20:06:15 -0400 Subject: [PATCH] snort3: CWE-407 CHP match_tally O(M*T); pgbouncer: CWE-312 SCRAM secret logged snort3-0002: chp_add_candidate_to_tally() in http_url_patterns.cc calls std::find_if over CHPMatchTally vector for each Aho-Corasick HTTP key-pattern match callback, O(M*T) per packet. Fix: add unordered_map index to ChpMatchDescriptor for O(1) lookup. 48x op-count reduction at T=100/M=20. 3/3 PASS. pgbouncer-0001: scram_client_first() logs user->passwd (SCRAM verifier or plaintext password) at slog_debug level, CWE-312. Fix: remove the log line. 5/5 PASS. pgbouncer MOAD-0002/0003/0005 CLEAN (single-threaded libevent loop). snort3 MOAD-0002/0003/0004/0005 CLEAN. --- defects/pgbouncer-0001/NOTES.md | 43 ++++ .../pgbouncer-0001/patch/pgbouncer-0001.patch | 10 + ...ncerScramSecretLogTest$PgCredentials.class | Bin 0 -> 521 bytes .../test/PgBouncerScramSecretLogTest.class | Bin 0 -> 6054 bytes .../test/PgBouncerScramSecretLogTest.java | 132 ++++++++++++ defects/pgbouncer/CLEAN.md | 32 ++- defects/snort3-0002/NOTES.md | 42 ++++ defects/snort3-0002/patch/snort3-0002.patch | 66 ++++++ .../test/Snort3ChpMatchTallyTest$CHPApp.class | Bin 0 -> 458 bytes ...3ChpMatchTallyTest$CHPMatchCandidate.class | Bin 0 -> 646 bytes .../test/Snort3ChpMatchTallyTest.class | Bin 0 -> 5754 bytes .../test/Snort3ChpMatchTallyTest.java | 203 ++++++++++++++++++ 12 files changed, 525 insertions(+), 3 deletions(-) create mode 100644 defects/pgbouncer-0001/NOTES.md create mode 100644 defects/pgbouncer-0001/patch/pgbouncer-0001.patch create mode 100644 defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest$PgCredentials.class create mode 100644 defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.class create mode 100644 defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.java create mode 100644 defects/snort3-0002/NOTES.md create mode 100644 defects/snort3-0002/patch/snort3-0002.patch create mode 100644 defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPApp.class create mode 100644 defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPMatchCandidate.class create mode 100644 defects/snort3-0002/test/Snort3ChpMatchTallyTest.class create mode 100644 defects/snort3-0002/test/Snort3ChpMatchTallyTest.java diff --git a/defects/pgbouncer-0001/NOTES.md b/defects/pgbouncer-0001/NOTES.md new file mode 100644 index 000000000..c311e18ed --- /dev/null +++ b/defects/pgbouncer-0001/NOTES.md @@ -0,0 +1,43 @@ +# pgbouncer-0001: CWE-312 — SCRAM verifier logged verbatim at debug level + +## Target + +PgBouncer PostgreSQL connection pooler: `src/client.c`, function `scram_client_first()` + +## Defect + +Line 1124: +```c +slog_debug(client, "stored secret = \"%s\"", user->passwd); +``` + +During SCRAM-SHA-256 authentication, PgBouncer logs `user->passwd` at debug level. The +stored secret is either: +- A SCRAM-SHA-256 verifier string: `SCRAM-SHA-256$:$:` +- A plaintext password (when auth_type=plain is configured) + +Either form is sensitive. A SCRAM verifier can be used in an offline dictionary attack +to recover the original password. A plaintext password is immediately usable. + +Debug logging is commonly enabled during troubleshooting and the output is written to +persistent log files, creating indefinite credential exposure. + +## Fix + +Remove the `slog_debug` line. The adjacent line 1119 already logs the SCRAM event +(`SCRAM client-first-message`), preserving diagnostic context without exposing the secret. + +## Severity + +MEDIUM-HIGH (CWE-312). Requires debug log access, but operators routinely enable debug +logging during connection issues, leaving credentials in log files indefinitely. + +## All 5 MOAD Results for PgBouncer + +| MOAD | Status | Notes | +|------|--------|-------| +| 0001 (CWE-407) | CLEAN | find_database() is O(D) but D is config-bounded (<100 DBs typical); user lookup uses AA-tree O(log U) | +| 0002 (Intertangle) | CLEAN | Single-threaded libevent loop; no coupling via shared mutable runtime state | +| 0003 (Leaked Context) | CLEAN | Single-threaded; no thread_local usage; not applicable | +| 0004 (CWE-312) | DEFECT | pgbouncer-0001: client.c:1124 logs SCRAM verifier/password at slog_debug | +| 0005 (Thundering Herd) | CLEAN | Single-threaded; no concurrent cache access; not applicable | diff --git a/defects/pgbouncer-0001/patch/pgbouncer-0001.patch b/defects/pgbouncer-0001/patch/pgbouncer-0001.patch new file mode 100644 index 000000000..4b639c22e --- /dev/null +++ b/defects/pgbouncer-0001/patch/pgbouncer-0001.patch @@ -0,0 +1,10 @@ +--- a/src/client.c ++++ b/src/client.c +@@ -1121,7 +1121,7 @@ static bool scram_client_first(PgSocket *client, uint32_t datalen, const uint8_t + + if (!user->mock_auth) { +- slog_debug(client, "stored secret = \"%s\"", user->passwd); ++ /* Do not log user->passwd: it holds the SCRAM verifier or plaintext password (CWE-312). */ + switch (get_password_type(user->passwd)) { + case PASSWORD_TYPE_MD5: + slog_error(client, "SCRAM authentication failed: user has MD5 secret"); diff --git a/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest$PgCredentials.class b/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest$PgCredentials.class new file mode 100644 index 0000000000000000000000000000000000000000..b6d366c5fd375c098727d56b202a6d4f0529f560 GIT binary patch literal 521 zcmaJ;T}uK%6g}hDezc~US!sn2LGCK~1CpSGC=e4?eCQ>{b=cN)Hg?DQT|G$9LqDJ& z72WkELh#|u@1G5xHK(=dSP~p?aY|6tOxT_}xfMgj4~a#S9tA=YpZs z`y1h^D3Ja|BD!s%kSOL}cpSe?5^Bdn3A+r&JoKI)`9eK0um^^-#=68hV~=QLH=NMO zLNAb_znG6jH00wMF{T?XB2U}|$)Uc%#Ch^q2E9*)Zo^n(wG%#3qQi`ZMBOiin z=l`t%buyNs)u$|AWE5GH3HPY#B>zKLc0RzI<`SCIS|abHmngokZdWK* kr~_yq0~-b$Y@zW@LL literal 0 HcmV?d00001 diff --git a/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.class b/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.class new file mode 100644 index 0000000000000000000000000000000000000000..377a69b50e1ebd0802544544a1f45e8cadd41376 GIT binary patch literal 6054 zcmcIocYG7)8Gb(7ptCr@C~Qb_5Cvm{@qmzEV~8<0rWkOLfdHY-Svt#yC7rlCIXKNu zN7{5ZZ4)}uosM*bBw(N!-EEqVbdPlJk?!3l>GR#4EXfv>fBN&!(%pCO_kPd2p7%XH z{_Lqo0Cb3XVN{?pgenzNFjb)DkUpX}ru0m*ap%AxBkl@JU2kSgcY{FX^140MP*5F0 zSj9A`0*iW+o2*)hI zgb-$^n28#Jd1-yv=(I9%-Q8=tL%ecaJ>zoQWy}429;ldWo0(+WQ z%)#6e(|j#7ZP2pQI+3m`bH;%pjlM71${aEc^RXa=g(?qrJ&a#*pYtJ;iubr>*#Kzn*i41O#oU7ntq1 z7DY72i+=!H9182r6VFmP7M%C4>qZ2mgzSoo-%Y>!D?RWDKl;;xI|#1 z=c0yKcSl3ZC2ON=n%1ptjkk0;iS8Xk1Kqo>+qd^fQ#9s5*k+7&8@ACJGwcz=4(`H4 za;Z%4%LHoA$!B$Y6tpsIepFhUTP|JI#J}d&OV+GzQLtWMerDy$OuL71rQPn!WthH( zjMfGf8*#b7VnC8`+Ue66zEcM1-b>QT(4}k67fNnw`*FN#$HC)WO*8G)9^fO3!z8FcI;s3 z7IMBsw}7H&Mz`y3oSCvDu+@W5MrYlANiDr9u9W$BUX=M_*z#Owv+3vqDI*a=jBYRR zQ%7E6i)~xXncYm|&3Wf)W;&bFoFOZhN@y$@t+pgZYs1)uy)q@PIzw@?LiXc82v@5( zi07BG$Ef2PbZe!RBX!L2GupK1AT#5RyR`yc(oedI0mLQMm?_t=3siGPNBH{6gi5M2 zR1AWpUp;7M5?zuk1m>2+YiCw^O;5EcFj=4*!x%;?gtUqbEEZrPSb0f;v+OM?hZ(u7 zI3x2sRaCOUQUsMaEQy=;lx(5(lAg=(WEd`TA&jWF7DpIh4@Rle1VUNq!c=BLO!8Xy z$lAJ2#S3I@%`a11gXR$<5kVEYB=_H_;wIcots{CWXY3rDfVGTy!9fw`#bH@Vx2bp` zUc|z1%9Aq2P^@vUS@^DY%=7=;X47?Kc6B@GU#h z+(IMXM3Ttcx2|PX^V-W+HII|&Tlmnk&{HJIR4NRtRHT(}6Ie0P${joVJR7qqohf4+ zakc!3j)Hf{j#gyWWFals?^*FKGL>v-_Xy0&SZ;&fK!Qv)_~s~hFWX$+2H6V= z-QfKK3p@kzI%tEu3{0q&CVWt!alCJ~yLQfK&3DTIThGLY@?14wXwI;i%}Vcnn8S}} zhTPd8JMN@SzNp4IMk`gX;C_J(?d|PaHaXyrXbnwGP0g)Z=iV(1tD9RiKRehZ%|X+! zr8DUaO?NeKy3n{)mfHg=9+aJVp6m_d^eP8Bf7p{U$5ot=G7F@P7pTpa;q0*FBv_C- zsp6EBT0AZllbI@2Z5~fMsqUjH9+rX^NWtEGe0${t!G!YF5~>t^RK;UbbdD6=PA_%P zN#ha-biwZs|FukM=NS64tiqGJ)l#5##m$}3VXHhW=b`I%w%s8Ag*xTFc zIPW|@o(QG25ylhvLI__}@g;m&U}m9Hb*6}&<&iKPlTCEIam8HS&G7+wWq$}?$+MzI z{T;TgkIE@Ij3-e^!jGBB48?8oP_(E;NRjnL3>{F|hBmTRrFI%l9xdzSCR)Gl-|?MGNG-K zK^K^|L#Cr4UC!{dCR8VR~$Sb4r-zXi)T*7Z)8gU zR>hC-R1Kcu!tdq6A5{DhPbv75z(pP#uyyzoH?T!=boFHyO*Fx;h^HZ+Y~8RD9pV|E#=w zPQ~}+oe;be6)L{#-BpFf6frdNdA)-#Ee;Y52ZYdDuJGaVOINy)~((X3HMyto+3%nE^uPCb>1nH-8~U^pzM2{k09 zt73+j$z+{mQjb?i#H$bH4VsoNzI)>TZ{AsLFL~&DwAeKzk3GK?4?q)L!X)3C$r!fB ze+DaIddaI~NSw#QpX8%*(F}<>q@lBma*Xvuk0s2y3jVfG!EgRd!8e?*d3Rs0nJ-^2 zcs+}2{Jn#p-{9T({1*J4Qhx&CeqIQClb`Z2un@OX;Zj~p(PmD*mHeJ_459iJr!XB@ zIfhv$F|Pu93tQ!{Du9OB9u~d}2HX3nRBo@WkDOoiFe>&{u8fW0!W9qVqNA8yAF1;f zdA~xgONA947CF?)(`TTCzhSCH(NlGjx5L;fZ(bR35sL*x{)z;QSQkjqfb(>D{- z7P|5ZEXGze@-3(n+j!N3L;OzjB~*H%8sZsrh7cul%W)(y;hw;RYtCZAXaU&uB_=Ff zbT{To6DTl-8&2UCP<6qUTcy=6o&fp+j^;owQCvyTF@oMr(0d4XFZyv+0c=kImQzsx z_KpB-PYKwUM(*(Z{fhd?tHw|S_BAD73;up>fH87si8p;<2MFwH0y{`x{RDOmy{XfW z0|j7tFLDYyE3h|2?(%>gt&hC91lU_A0J}R#gnRP9-d+ZnL12RfmLxEfz=jDdVz;l1DTaK;$FTJceUqcxVh`V>msAN96Ly zf&+ct@ejP72i`#Xx{)P%6A9~P9)1hv;Z|IT+iJqHiv}lL7@QEQk9^`dK6M`tD<-fJ z3bO%E7QU;D@F5DbcTX1nTp8hh3UkJoEL;x8ZVGdbIXB@R3UdlNH{q@H)Q`tAVw1;M zz@mt99G^dmP}RMts(b)DPvgmbC-Bt=5GoLi&(jN0!yX{BYzivW%Q-pYC%mb^Pw_K; zR}n@OKj+mRgasKFzYx`=&tKu!csjWH9k1k*KL+=I4(|Wvt^Ole{nxnsGx;l~#?u`C Uz&DW!o}!3Z{Evv)VlEc^7kL6&FaQ7m literal 0 HcmV?d00001 diff --git a/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.java b/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.java new file mode 100644 index 000000000..c3755a436 --- /dev/null +++ b/defects/pgbouncer-0001/test/PgBouncerScramSecretLogTest.java @@ -0,0 +1,132 @@ +import java.util.*; + +/** + * pgbouncer-0001: CWE-312 — SCRAM verifier logged verbatim at debug level + * + * Source: src/client.c, function scram_client_first() + * Line: slog_debug(client, "stored secret = \"%s\"", user->passwd); + * + * Defect: During SCRAM-SHA-256 authentication, PgBouncer logs the user's stored + * secret (user->passwd) at slog_debug level. Depending on the auth_type configured, + * user->passwd may contain: + * - A plaintext password (auth_type=plain) + * - A SCRAM-SHA-256 verifier: "SCRAM-SHA-256$:$:" + * + * Even the SCRAM verifier is sensitive: it can be used in an offline dictionary attack + * or, for plaintext passwords, directly as the credential. Logging it to a debug log + * file violates CWE-312 (Cleartext Storage of Sensitive Information). + * + * Fix: Remove the slog_debug line. The log event type ("SCRAM client-first received") + * is still present on line 1119; no diagnostic value is lost. + * + * CVE-applicable: Yes — debug log file exposure of authentication credential. + * Severity: MEDIUM-HIGH (requires debug log access, but many ops enable debug logging + * during troubleshooting, leaving credential in log files indefinitely). + */ +public class PgBouncerScramSecretLogTest { + + // Simulate the log capture system + static List capturedLogs = new ArrayList<>(); + + static void slog_debug(String context, String fmt, Object... args) { + capturedLogs.add(String.format("[DEBUG][" + context + "] " + fmt, args)); + } + + // Simulated PgCredentials + static class PgCredentials { + String name; + String passwd; + boolean mock_auth; + PgCredentials(String name, String passwd) { + this.name = name; + this.passwd = passwd; + this.mock_auth = false; + } + } + + // --- DEFECTIVE: logs stored secret --- + static void scram_client_first_defective(String clientContext, PgCredentials user, String clientFirstMessage) { + slog_debug(clientContext, "SCRAM client-first-message = \"%s\"", clientFirstMessage); + if (!user.mock_auth) { + // DEFECT: logs the SCRAM verifier or plaintext password + slog_debug(clientContext, "stored secret = \"%s\"", user.passwd); + } + // ... rest of SCRAM processing + } + + // --- FIXED: does not log stored secret --- + static void scram_client_first_fixed(String clientContext, PgCredentials user, String clientFirstMessage) { + slog_debug(clientContext, "SCRAM client-first-message = \"%s\"", clientFirstMessage); + if (!user.mock_auth) { + // Fixed: no credential logging. Comment in code: + // /* Do not log user->passwd: it holds the SCRAM verifier or plaintext password (CWE-312). */ + } + // ... rest of SCRAM processing + } + + static void testDefectiveLogsSecret() { + capturedLogs.clear(); + PgCredentials user = new PgCredentials("alice", + "SCRAM-SHA-256$4096:c2FsdHNhbHRzYWx0$StoredKeyHere:ServerKeyHere"); + scram_client_first_defective("client:127.0.0.1:5432", user, "n,,n=alice,r=clientnonce"); + + boolean secretFound = capturedLogs.stream().anyMatch(line -> line.contains("stored secret")); + assert secretFound : "Defective impl should log 'stored secret'"; + System.out.println("PASS defective: secret IS logged: " + + capturedLogs.stream().filter(l -> l.contains("stored secret")).findFirst().orElse("?")); + } + + static void testFixedDoesNotLogSecret() { + capturedLogs.clear(); + PgCredentials user = new PgCredentials("alice", + "SCRAM-SHA-256$4096:c2FsdHNhbHRzYWx0$StoredKeyHere:ServerKeyHere"); + scram_client_first_fixed("client:127.0.0.1:5432", user, "n,,n=alice,r=clientnonce"); + + boolean secretFound = capturedLogs.stream().anyMatch(line -> line.contains("stored secret")); + assert !secretFound : "Fixed impl must NOT log 'stored secret', got: " + capturedLogs; + System.out.println("PASS fixed: secret is NOT logged (logs: " + capturedLogs.size() + " line(s))"); + } + + static void testFixedStillLogsDiagnostic() { + capturedLogs.clear(); + PgCredentials user = new PgCredentials("bob", "SCRAM-SHA-256$4096:abc$defgh:ijklm"); + scram_client_first_fixed("client:127.0.0.1:5433", user, "n,,n=bob,r=bobnonce"); + + boolean clientFirstLogged = capturedLogs.stream().anyMatch(line -> line.contains("client-first-message")); + assert clientFirstLogged : "Fixed impl must still log SCRAM client-first-message event"; + System.out.println("PASS fixed: diagnostic 'client-first-message' still logged"); + } + + static void testPlaintextPasswordNotLogged() { + capturedLogs.clear(); + PgCredentials user = new PgCredentials("carol", "supersecretpassword123"); + scram_client_first_fixed("client:192.168.1.1:5432", user, "n,,n=carol,r=carolnonce"); + + boolean plainPassFound = capturedLogs.stream().anyMatch(line -> + line.contains("supersecretpassword123")); + assert !plainPassFound : "Fixed impl must NOT log plaintext passwords"; + System.out.println("PASS fixed: plaintext password NOT logged"); + } + + static void testMockAuthNotLogged() { + capturedLogs.clear(); + PgCredentials user = new PgCredentials("mockuser", "not-a-real-secret"); + user.mock_auth = true; + // Both defective and fixed skip the branch when mock_auth=true + scram_client_first_defective("client:10.0.0.1:5432", user, "n,,n=mockuser,r=mocknonce"); + + boolean secretFound = capturedLogs.stream().anyMatch(line -> line.contains("stored secret")); + assert !secretFound : "mock_auth=true: secret branch should be skipped"; + System.out.println("PASS mock-auth: secret branch skipped for mock_auth=true"); + } + + public static void main(String[] args) { + System.out.println("=== pgbouncer-0001: CWE-312 SCRAM verifier logged at debug ==="); + testDefectiveLogsSecret(); + testFixedDoesNotLogSecret(); + testFixedStillLogsDiagnostic(); + testPlaintextPasswordNotLogged(); + testMockAuthNotLogged(); + System.out.println("ALL PASS"); + } +} diff --git a/defects/pgbouncer/CLEAN.md b/defects/pgbouncer/CLEAN.md index 65fb521e3..2b407d011 100644 --- a/defects/pgbouncer/CLEAN.md +++ b/defects/pgbouncer/CLEAN.md @@ -1,6 +1,8 @@ -# PgBouncer — CLEAN (CWE-407) +# PgBouncer — CLEAN (CWE-407) | DEFECT pgbouncer-0001 (CWE-312) -Scanned 2026-03-30. +Scanned 2026-03-30 (CWE-407 only). Re-scanned 2026-03-31 (all 5 MOADs). + +## MOAD-0001 (CWE-407) — CLEAN PgBouncer uses efficient data structures throughout: - **User lookup**: AA-tree (`aatree_search`) — O(log N) @@ -10,4 +12,28 @@ PgBouncer uses efficient data structures throughout: - **Database lookup**: `find_database()` is linear scan of `database_list`, but databases are configuration-bounded (typically <100), not attacker-controlled. -No CWE-407 defect found. +## MOAD-0002 (Intertangle) — CLEAN + +PgBouncer is single-threaded (libevent loop). Global lists (`database_list`, `pool_list`, +`user_tree`) are accessed from one thread only. No shared mutable god object coupling +independent subsystems. + +## MOAD-0003 (Leaked Context) — CLEAN + +Single-threaded event loop. No `thread_local` or `pthread_key` carrying request-scoped +identity. Not applicable. + +## MOAD-0004 (CWE-312) — DEFECT: see pgbouncer-0001 + +`src/client.c` function `scram_client_first()` line 1124: + +```c +slog_debug(client, "stored secret = \"%s\"", user->passwd); +``` + +Logs `user->passwd` at debug level. Depending on auth_type, this is a SCRAM-SHA-256 +verifier (offline-crackable) or a plaintext password. Fix: remove log line. + +## MOAD-0005 (Thundering Herd) — CLEAN + +Single-threaded. No concurrent cache access patterns. Not applicable. diff --git a/defects/snort3-0002/NOTES.md b/defects/snort3-0002/NOTES.md new file mode 100644 index 000000000..47c7d5eb5 --- /dev/null +++ b/defects/snort3-0002/NOTES.md @@ -0,0 +1,42 @@ +# snort3-0002: CWE-407 — CHP match_tally O(M*T) linear scan per HTTP packet + +## Target + +Snort3 network IDS: `src/network_inspectors/appid/detector_plugins/http_url_patterns.cc` + +## Defect + +`chp_add_candidate_to_tally()` uses `std::find_if` over `CHPMatchTally` (a +`std::vector`) to locate a CHPApp entry and decrement its +`key_pattern_countdown`. This function is called from `chp_key_pattern_match()`, which is +the Aho-Corasick match callback invoked for every key-pattern match in HTTP payload +inspection. + +With M pattern match callbacks per HTTP packet and T distinct CHPApp candidates in the +tally, the total work is O(M * T) per packet. + +## Fix + +Add `std::unordered_map match_tally_index` to `ChpMatchDescriptor`. +The index maps each CHPApp pointer to its position in the `match_tally` vector. Lookup is +O(1) amortized. Total work becomes O(M + T) per packet. + +## Complexity + +O(M*T) -> O(M+T) per HTTP packet. Measured: 48x op-count reduction at T=100, M=20. + +## Severity + +MEDIUM. HTTP inspection is a hot path in any network IDS deployment. Flows with many +HTTP rule patterns (e.g., enterprise deployments with hundreds of CHP rules) see +measurable per-packet overhead. + +## All 5 MOAD Results for Snort3 + +| MOAD | Status | Notes | +|------|--------|-------| +| 0001 (CWE-407) | DEFECT x2 | snort3-0001 (service_candidates, patched); snort3-0002 (CHP match_tally, this) | +| 0002 (Intertangle) | CLEAN | SnortConfig is immutable at runtime; per-thread flow state; no god object | +| 0003 (Leaked Context) | CLEAN | THREAD_LOCAL used correctly: per-thread packet stats, cleared after each packet | +| 0004 (CWE-312) | CLEAN | No sensitive headers logged verbatim; extractor does not log auth headers | +| 0005 (Thundering Herd) | CLEAN | Per-thread flow/session caches (THREAD_LOCAL); no shared mutable cache without sync | diff --git a/defects/snort3-0002/patch/snort3-0002.patch b/defects/snort3-0002/patch/snort3-0002.patch new file mode 100644 index 000000000..9c72d3ca7 --- /dev/null +++ b/defects/snort3-0002/patch/snort3-0002.patch @@ -0,0 +1,66 @@ +--- a/src/network_inspectors/appid/detector_plugins/http_url_patterns.h ++++ b/src/network_inspectors/appid/detector_plugins/http_url_patterns.h +@@ -217,9 +217,11 @@ struct CHPMatchCandidate + + typedef std::vector CHPMatchTally; + ++#include ++ + class ChpMatchDescriptor + { + public: + void sort_chp_matches() + { + for(unsigned i = 0; i < NUM_HTTP_FIELDS; i++) +@@ -235,6 +237,7 @@ public: + unsigned cur_ptype = 0; + uint8_t* buffer[NUM_HTTP_FIELDS] = {}; + unsigned length[NUM_HTTP_FIELDS] = {}; + CHPMatchTally match_tally; ++ std::unordered_map match_tally_index; + }; + +--- a/src/network_inspectors/appid/detector_plugins/http_url_patterns.cc ++++ b/src/network_inspectors/appid/detector_plugins/http_url_patterns.cc +@@ -589,14 +589,14 @@ static int chp_pattern_match(void* id, void*, int match_end_pos, void* data, vo + static inline void chp_add_candidate_to_tally(CHPMatchTally& match_tally, CHPApp* chpapp) + { +- auto it = std::find_if(match_tally.begin(), match_tally.end(), +- [&chpapp](const CHPMatchCandidate& item){ return chpapp == item.chpapp; }); +- if (it != match_tally.end()) +- { +- (*it).key_pattern_countdown--; +- return; +- } +- +- match_tally.emplace_back( CHPMatchCandidate{ chpapp, chpapp->key_pattern_length_sum, +- chpapp->key_pattern_count - 1 } ); + } + ++static inline void chp_add_candidate_to_tally(CHPMatchTally& match_tally, ++ std::unordered_map& match_tally_index, CHPApp* chpapp) ++{ ++ auto it = match_tally_index.find(chpapp); ++ if (it != match_tally_index.end()) ++ { ++ match_tally[it->second].key_pattern_countdown--; ++ return; ++ } ++ ++ std::size_t idx = match_tally.size(); ++ match_tally.emplace_back( CHPMatchCandidate{ chpapp, chpapp->key_pattern_length_sum, ++ chpapp->key_pattern_count - 1 } ); ++ match_tally_index[chpapp] = idx; ++} ++ + // In addition to creating the linked list of matching actions this function will + // create the CHPMatchTally needed to find the longest matching pattern. + static int chp_key_pattern_match(void* id, void*, int match_end_pos, void* data, void*) +@@ -611,7 +613,7 @@ static int chp_key_pattern_match(void* id, void*, int match_end_pos, void* data + if (target->key_pattern) + { +- chp_add_candidate_to_tally(cmd->match_tally, target->chpapp); ++ chp_add_candidate_to_tally(cmd->match_tally, cmd->match_tally_index, target->chpapp); + } + + return chp_pattern_match(id, nullptr, match_end_pos, cmd, nullptr); diff --git a/defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPApp.class b/defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPApp.class new file mode 100644 index 0000000000000000000000000000000000000000..50abe2b343594090be31977816156017a308317a GIT binary patch literal 458 zcmZ`#O;5r=5Pe%dT55$V2qGV9;sJV)gI8igni!fOl0di@*g&ytNxL=SXL%qd9{d6R zDB~0lo;d8j*_k&p@9p=`=NEt;8afoDRHO}LkYzB}?wi~3U9s%E%+`D^8M0kZc=C}U z)oM?3(2!H18#sZ%&>V{(ln?f5`|Qg3YU29-Zo(sZXZJ_F?Uq6D77WZu+VTbpNl)RG z??$eaJQQ{i3z^i;44fym;(u+x#Zs=u@rD$%9LH%-8PayJ;51?22|kQBGagRdnNJyg z9K_+AKY7WcenixL;*CKYlE8itNflQNl_O*Z!x4gqw(mv}k4VIyCESo)h5jxDGx-CG z%*59L$payylR=TNL{XR6X~L5A0cO?qP_6nNr&ePR^ZmfSpu0>RK!cb~d<}xKSf%- literal 0 HcmV?d00001 diff --git a/defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPMatchCandidate.class b/defects/snort3-0002/test/Snort3ChpMatchTallyTest$CHPMatchCandidate.class new file mode 100644 index 0000000000000000000000000000000000000000..8ac1a8bf130b0e22e21d6706590b8beef87468e2 GIT binary patch literal 646 zcmah`%SyvQ6g{`CrmfLx>)WdJ1$~GvTq=S@M4>*Q#r?Dy+Gw1FWUBaCf<|JZ6RZ01fzt>8*wjoeW7~0mz^8g)r8RlPkH)~ zkgnFQbI2lRA#Y<0Hlfy1L8SMbet0f)x8D}N|JatX-gHi{62=kA^<1GP`*i!E2tz`p z`A64L81CmVfr5oe8&fC}R{oX~O1JXyN@y)3)s(8I`>nwpfzHgZ1!Fd2W7aqozJyLN zP}&IQZ7i6(Zzjo|8$2k&M)kj8YuAK~6S%x#q3J1kF}UlMD`P+hLpbm literal 0 HcmV?d00001 diff --git a/defects/snort3-0002/test/Snort3ChpMatchTallyTest.class b/defects/snort3-0002/test/Snort3ChpMatchTallyTest.class new file mode 100644 index 0000000000000000000000000000000000000000..6fa70d1365972a3f8774a4dcf40d1ce7b84ce95b GIT binary patch literal 5754 zcmbtY`&$&}6@G_Zc6OIh5V)5N5SrnwtwZf#?XA~rEe8*^{d`{lPjPydHLu}1oSGs9j2G4?4x%zpEI=R4my z?|IMp&hYWYskZ^N;cs4K!6Cz`AR8_Tc{|nJYVC*`9jfiyzEcYs5?pH{(TLG1!BJVY zB?oSJWOxIi z39=X0!Qn+GHi__dlQ@-KU0qdMyy!)rjLizZgX<*}e~oepOG&grhIlYEL&HHsA2b9@ z2gBM9CR}7U@m^(np3JP~Z)TCOpmmtt`-nJ1x>3eW5=v4$YnzekjK}qOK8w)J3I=ftK^s+fX&rhr zq#9cz#xO?-Lya1VS!@d$@kn&2*^6!1F2WG{nta)k4+%Htv%u{TM-1@@Gg@gBvQ>Ey z!A=>w6pUarqX-7~B@B&I4qXtk(ApZ&Yd6ybgS$0#v^gIR=;8$CAi14E?xd}8BxD#0 z#;|*anwe^tAu*wJge4>+{cRN%$@VJPhuc_qw`8k2=29>VL$@9!>b?l1G5BE`66Z z&3$p)uiybZ$mBBtV?|k$UMOt5wxxyHy2_eub(y#~yVe$vh5XBh6Az0xJ~Hd@WEdlO zRKa8T39DmNjq3f8QL8#`bmIvLgC?dUYJ5lwBtq(l_6@LBR;;MsF`B6QD!ys+89=+* zR*c^*Om(eYDt?5`*?8EEXC$m{X=(8%%*!uOS65fxI|M_XQ}DdF`*RK6lO1|I&MFku5(%+(RKbg4Ek~^N#ms9?%uguznV6q1=DUT% zLE*0(FB6%z?ry(8!i!h&nv9<-_=ULYb5m8IV}#X$*%HF47KNap8e?2=cIaUO zo7)|UYQ1Bl+qHPVs3^SiC=uGl@?^hzf{~#p_r$q&%U4ri&83QFW;4*-dKvXj!qNZp zh>Qw*N%KUnk;dSeOY-u}Z1|td6i2l-vs@7OQ8mK)Q&gFrJ;?&Z(5?xM*xjHvsE@@% z+WLqH%N1X9;jIu3lRdp;NslJt$l&I+>$;IJ;%cl%GlunW!i}>M%96#b$-gJg65%&* zMZf4H{E{2*O7NN0YwqP=OQ=saVgAub!eqW*HT;oiSljEDn$oJ8WS29leP6;gX4ZyM z#BcI%*NtI+EUt&gLYhAk)}ls)^tLUk>~E23SSqC|H$I?MoqJ=<7%l8?UA@JhA4w=pEw8N0T+emk6E8l+XEJ^(>dx=jAEa_565Xxu(rPXAt$T638e(g`Pr?)F z1;(9I!{;ygR>oh6zuC)AL3&*XLOjJXP{bydO&k>n(fzy=ocvw8{4`_Pr_kb z$-zPi@1VFha00&SW0)TpIDs-}_)V0(J~91|0~p%9xd#EH)4ZnrW?`)ER#8JnfNC6u{*I$lhig4jP4R68&H`z7P<-OP?6KCHaY`7d4?-Z9-rqN%4&R`x#%*1T7oYK z5gsL~ zQI29Ta4W{y*F8cspP$RZkMEm;L$qAA7{Cvj`4;O%6o18ygq z?;w-!WKH-!3*J2}d;8Ifdzm-m%##DuwV&L$k8C(dsr&hT@BohDLA-{C7{>dl^b!0W zhj5YIw~WW71{{_;aYP#6`yhTQjo>MhG1nn?nhbG@^m4PJpU2`a;T7Wxm`CpL{XDWS zqQIh4a<-haOO{?hmh>NN<(SlE>uYfc|F$@U$MWi%^SpDjXwe)js#!FNJ?-Lodt-L_ z!J_Pg`ZY3v%KyImEo2QiN|{T+0Vmh_lU+WEyMqI+qHOb5E|Dj3Ab1i7x0)eaR3aDP zp~s&>zKBhUJcS=y@Tah@H!JRQ{2&$de}OO@MJj{2my@Yjm8JqD6bM0m3*sFMy z=!yze0>?DdG##^x2s@n)g1|oC0zuluG+IFEPxHdfc?Fjb!Xyrv5bSBBV}}~))uBd@ zg-MPW&m$~FIYcH`AoF;hJO?y3Caxm6Tsv5R!@7WGKrW&JLgp@-SL8{X&vTXJeg(VS z0z8?-E_aG#F;gdnomH`3Cw9l_@f*bM1hG5GYx)$c)+8_IDdKkqoA4`Q_%=I>v#f9L z5Y@9p@m+j^_e?A+W@9%eRB(P{oU2%?8-CvA4EXL5+%qg43+G_z+C&rafnNgNXs zFWKp2bt$DZ4tSq|Imc_^1Dg3EgYpr4_?UC2XM)8`OK1cZ~x;&aND Y62xL&pueX@o&5a+fB%R-. For each Aho-Corasick + * pattern match callback (chp_key_pattern_match), chp_add_candidate_to_tally performs + * std::find_if over the entire match_tally vector to locate the CHPApp entry and + * decrement its key_pattern_countdown. With M pattern matches and T unique CHPApp + * candidates in the tally, the total work is O(M * T) per HTTP packet. + * + * Fix: Add std::unordered_map match_tally_index alongside the vector. + * Lookup becomes O(1) amortized, reducing total work to O(M + T) per packet. + * + * Complexity: O(M*T) -> O(M+T) per HTTP packet + * Severity: MEDIUM (per HTTP flow, M~50-200 pattern matches, T~10-50 candidates) + * Speedup: up to T-fold (e.g., 50x at T=50 candidates) + */ +public class Snort3ChpMatchTallyTest { + + // Simulate CHPApp as an opaque identity (pointer in C++) + static class CHPApp { + final int id; + final int keyPatternCount; + final int keyPatternLengthSum; + CHPApp(int id, int kpc, int kpls) { + this.id = id; + this.keyPatternCount = kpc; + this.keyPatternLengthSum = kpls; + } + } + + static class CHPMatchCandidate { + CHPApp chpapp; + int keyPatternLengthSum; + int keyPatternCountdown; + CHPMatchCandidate(CHPApp app) { + this.chpapp = app; + this.keyPatternLengthSum = app.keyPatternLengthSum; + this.keyPatternCountdown = app.keyPatternCount - 1; + } + } + + // --- DEFECTIVE implementation: O(M*T) --- + static void chp_add_candidate_to_tally_defective(List match_tally, CHPApp chpapp) { + // Linear scan over tally - O(T) per call + for (CHPMatchCandidate item : match_tally) { + if (item.chpapp == chpapp) { + item.keyPatternCountdown--; + return; + } + } + match_tally.add(new CHPMatchCandidate(chpapp)); + } + + // --- FIXED implementation: O(1) amortized --- + static void chp_add_candidate_to_tally_fixed( + List match_tally, + Map match_tally_index, + CHPApp chpapp) { + Integer idx = match_tally_index.get(chpapp); + if (idx != null) { + match_tally.get(idx).keyPatternCountdown--; + return; + } + int newIdx = match_tally.size(); + match_tally.add(new CHPMatchCandidate(chpapp)); + match_tally_index.put(chpapp, newIdx); + } + + // Simulate M pattern match callbacks for a given set of CHPApp candidates + static long benchDefective(List apps, int matchesPerApp) { + List tally = new ArrayList<>(); + long ops = 0; + for (CHPApp app : apps) { + for (int m = 0; m < matchesPerApp; m++) { + // Count ops: linear scan + int scanned = 0; + boolean found = false; + for (CHPMatchCandidate item : tally) { + scanned++; + if (item.chpapp == app) { + item.keyPatternCountdown--; + found = true; + break; + } + } + ops += scanned; + if (!found) { + tally.add(new CHPMatchCandidate(app)); + ops++; + } + } + } + return ops; + } + + static long benchFixed(List apps, int matchesPerApp) { + List tally = new ArrayList<>(); + Map tallyIndex = new HashMap<>(); + long ops = 0; + for (CHPApp app : apps) { + for (int m = 0; m < matchesPerApp; m++) { + ops++; // O(1) hash lookup + Integer idx = tallyIndex.get(app); + if (idx != null) { + tally.get(idx).keyPatternCountdown--; + } else { + int newIdx = tally.size(); + tally.add(new CHPMatchCandidate(app)); + tallyIndex.put(app, newIdx); + ops++; // map insert + } + } + } + return ops; + } + + static void testCorrectness() { + int T = 5; // distinct CHPApp candidates + int M = 3; // matches per app + List apps = new ArrayList<>(); + for (int i = 0; i < T; i++) apps.add(new CHPApp(i, 3, i + 10)); + + // Build tally with defective impl + List tallyD = new ArrayList<>(); + for (int m = 0; m < M; m++) { + for (CHPApp app : apps) { + chp_add_candidate_to_tally_defective(tallyD, app); + } + } + + // Build tally with fixed impl + List tallyF = new ArrayList<>(); + Map indexF = new HashMap<>(); + for (int m = 0; m < M; m++) { + for (CHPApp app : apps) { + chp_add_candidate_to_tally_fixed(tallyF, indexF, app); + } + } + + // Both should have same number of candidates + assert tallyD.size() == T : "Defective: wrong tally size " + tallyD.size(); + assert tallyF.size() == T : "Fixed: wrong tally size " + tallyF.size(); + + // Both should have same countdown values (first match creates with count-1=2, then M-1=2 decrements -> 0) + for (int i = 0; i < T; i++) { + assert tallyD.get(i).keyPatternCountdown == tallyF.get(i).keyPatternCountdown + : "Countdown mismatch at index " + i + + ": defective=" + tallyD.get(i).keyPatternCountdown + + " fixed=" + tallyF.get(i).keyPatternCountdown; + } + System.out.println("PASS correctness: both produce identical tally (T=" + T + ", M=" + M + ")"); + } + + static void testOpCount() { + // T candidates, each with M=10 pattern matches + int T = 50; + int M = 10; + List apps = new ArrayList<>(); + for (int i = 0; i < T; i++) apps.add(new CHPApp(i, M, i + 10)); + + long opsD = benchDefective(apps, M); + long opsF = benchFixed(apps, M); + + double ratio = (double) opsD / opsF; + System.out.printf("PASS op-count: T=%d M=%d | defective=%d ops | fixed=%d ops | ratio=%.1fx%n", + T, M, opsD, opsF, ratio); + assert ratio > 5.0 : "Expected >5x ratio, got " + ratio; + } + + static void testLargeScale() { + // Simulate a heavy HTTP scan: T=100 candidates, M=20 matches each + int T = 100; + int M = 20; + List apps = new ArrayList<>(); + for (int i = 0; i < T; i++) apps.add(new CHPApp(i, M, i + 10)); + + long t0D = System.nanoTime(); + long opsD = benchDefective(apps, M); + long t1D = System.nanoTime(); + + long t0F = System.nanoTime(); + long opsF = benchFixed(apps, M); + long t1F = System.nanoTime(); + + double ratio = (double) opsD / opsF; + System.out.printf("PASS large-scale: T=%d M=%d | defective=%d ops (%.2fms) | fixed=%d ops (%.2fms) | ratio=%.1fx%n", + T, M, opsD, (t1D - t0D) / 1e6, opsF, (t1F - t0F) / 1e6, ratio); + assert ratio > 10.0 : "Expected >10x ratio, got " + ratio; + } + + public static void main(String[] args) { + System.out.println("=== snort3-0002: CHP match_tally O(M*T) -> O(M+T) ==="); + testCorrectness(); + testOpCount(); + testLargeScale(); + System.out.println("ALL PASS"); + } +}