Martino Ferrari
18 KiB
18 KiB
Bug Fix Plan — Security & Correctness Remediation
Date: 2026-06-26
Based on: BUG_REPORT.md
Scope: Source/ and Client/
This plan organizes the ~60 findings from the audit into prioritized, dependency-ordered phases. Each phase is independently shippable. Phases are ordered by risk reduction: Critical remote-exploitable issues first, then High crash/OOB issues, then Medium robustness/DoS, then Low hardening.
Guiding principles
- Fix root causes, not symptoms. The integer-overflow-in-bounds-check pattern appears in 6+ places — fix the pattern, not each instance ad hoc. Introduce a shared
boundsCheck(off, count, elemBytes, bufLen)helper (C++) and avalidateCount(count, elemSize, bufLen)helper (Go) and use them everywhere. - Defense in depth. Origin checks + auth + input validation — not just one layer.
- No regressions. After each phase, run the existing test suites (
make -f Makefile.gcc test,python3 -m unittest tests_py,go test ./...in each Go module) and the E2E suite (./Test/E2E/chain/run_chain_e2e.sh --skip-build). - Minimal blast radius. Each fix is surgical to the file/function listed in the bug report. No refactors beyond what the fix requires.
Phase 1 — Critical remote-exploitable fixes (ship first)
Goal: Eliminate drive-by takeover and remote heap corruption. All fixes are small and localized.
| # | Bug | File(s) | Fix | Est. effort | Depends on |
|---|---|---|---|---|---|
| 1.1 | CR-1: 1-byte heap OOB write in WS frame NUL-term | WSServer.cpp:251 |
Change kRecvBuf = WS_MAX_RECV_PAYLOAD + 14u → + 14u + 1u |
5 min | — |
| 1.2 | CR-2: XSS via unescaped src.addr |
Client/udpstreamer/static/app.js:3503; Client/debugger/static/app.js:3549 |
Wrap src.addr with existing escHtml() in _statsKV calls (or inside _statsKV itself) |
10 min | — |
| 1.3 | CR-3: WebSocket CSRF (Origin check disabled) | Common/Client/go/wshub/hub.go:128; Source/Applications/StreamHub/WSServer.cpp:186-239 |
Go: Replace CheckOrigin: func(r *http.Request) bool { return true } with a same-origin check (compare Origin header host to Host header). Add a configurable allowlist env var for non-local deployments. C++: Parse Origin header in UpgradeHTTP; reject if present and host doesn't match the listen address. |
30 min | — |
| 1.4 | CR-4: Unauthenticated command injection to MARTe2 | Client/debugger/martecontrol.go:217-263 |
Add an allowlist of permitted MARTe2 commands (DISCOVER, TREE, INFO, LS, VALUE, TRACE, UNTRACE); reject FORCE, UNFORCE, PAUSE, RESUME, STEP, BREAK, MSG unless an explicit --enable-dangerous-commands flag is set. Log all forwarded commands. |
1 h | 1.3 |
| 1.5 | CR-5: No auth on DebugService TCP | DebugService.cpp:276 |
(a) Bind TCP server to localhost by default (add BindAddress config key, default 127.0.0.1). (b) Add an optional AuthToken config key; if set, require the first line from a client to be AUTH <token> before accepting commands. |
2 h | — |
Validation: bash -n on shell scripts; go build ./... in each Go module; make -f Makefile.gcc core apps; manual test: open browser console on a cross-origin page and confirm WS to localhost:8090 is rejected; confirm a crafted 65536-byte WS frame no longer corrupts.
Commit: fix(security): critical remote-exploitable fixes (CR-1..CR-5)
Phase 2 — High-severity crash / OOB / UAF fixes
Goal: Eliminate remote crash and memory-corruption vectors. These are the integer-overflow and concurrency bugs.
2A — Integer-overflow bounds checks (uniform pattern)
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 2A.1 | HI-1: DATA bounds check overflow | UDPSourceSession.cpp:358; UDPStreamerClient.cpp:520 |
Replace off + elemsToRead * wireElemBytes > size with 64-bit arithmetic. Add a validateBounds(off, count, elemBytes, size) static helper in UDPSProtocol.h and use it in both files. |
| 2A.2 | HI-2: Go unbounded allocations | protocol.go:121, 229, 325 |
Add validateCount(count, elemSize, bufLen) in protocol.go; call before every make([]T, n) that uses a network-derived count. Cap NumElements() at 1M. |
| 2A.3 | HI-3: accumFill overflow + size calc |
UDPStreamer.cpp:700, 738, 757, 857-860 |
(a) Add if (accumFill >= maxBatchCount) { flush; } before the write at line 857. (b) Use uint64 for maxBatchCount * totalSrcBytes size calculations. |
| 2A.4 | MD-4: numRows * numCols overflow |
UDPSourceSession.cpp:240, 346; protocol.go:121 |
Use static_cast<uint64>(numRows) * static_cast<uint64>(numCols); cap at 1M. |
| 2A.5 | MD-15: pairCount * 16u overflow |
Client/streamhub/Protocol.cpp:77, 117 |
Check pairCount > (len - off) / 16 before multiplication; use ull suffix. |
| 2A.6 | HI-6: FD_SET overflow |
UDPSServer.cpp:273, 308; UDPSClient.cpp:383 |
Add if (fd < FD_SETSIZE) guard before each FD_SET; otherwise skip that client this cycle (or switch to poll(), which the codebase already uses elsewhere). |
Est. effort: 3 h (pattern is repetitive once the helper exists)
2B — Use-after-free and concurrency
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 2B.1 | HI-5: Broadcast vs FreeSlot UAF | WSServer.cpp:345-366, 432-445 |
FreeSlot must acquire clients[idx].writeMutex before setting active=false and deleting sock. This ensures BroadcastText/BroadcastBinary cannot dereference a freed socket. |
| 2B.2 | HI-9: TraceRingBuffer not thread-safe | DebugCore.h:79-142 |
Replace volatile uint32 readIndex/writeIndex with Atomic<uint32> (MARTe2 Atomic::Load/Atomic::Store). Ensure Push writes data before storing writeIndex (release ordering); Pop loads writeIndex before reading data (acquire ordering). |
| 2B.3 | HI-4: ProcessSignal unclamped memcpy + forcedMask OOB |
DebugServiceBase.cpp:310, 313-318 |
(a) Clamp size to sizeof(signalInfo->forcedValue) (1024). (b) Cap the array-forcing loop at min(nEl, 256). (c) Validate nEl <= 256 in RegisterSignal. |
| 2B.4 | HI-7: Weak PRNG for WS handshake | WSClient.cpp:29-31 |
Replace srand(time(nullptr)) + rand() with std::random_device or getrandom()//dev/urandom read. |
| 2B.5 | HI-8: Global registry patching | DebugServiceBase.cpp:217-242 |
(a) Save original builders before patching (item->GetObjectBuilder()); store in a static array for restore on destruction. (b) Add a PatchRegistry config flag (default true for back-compat; document the implication). (c) Guard against double-patching (skip if already patched). |
Est. effort: 4 h
Validation: make -f Makefile.gcc test + ./Build/x86-linux/GTest/MainGTest.ex + ./Build/x86-linux/Test/Integration/Integration/IntegrationTests.ex + python3 -m unittest tests_py (in Test/E2E/chain/) + go test ./... (in each Go module). Craft a UDP packet with numSamples=0x20000001 and confirm no crash. Run the E2E suite: ./Test/E2E/chain/run_chain_e2e.sh --skip-build.
Commit: fix(security): high-severity crash/OOB/UAF fixes (HI-1..HI-9)
Phase 3 — Medium-severity robustness / DoS / parser fixes
Goal: Harden input validation, fix reassembly logic, and improve WS RFC compliance.
3A — UDPS protocol hardening
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 3A.1 | MD-1: recvMask too small |
UDPSClient.cpp:544, 592-594 |
Enlarge recvMask to 64 bytes (512 bits) to match the totalFragments <= 512 cap. |
| 3A.2 | MD-2: No type matching in reassembly | UDPSClient.cpp:548-555 |
Add type field to ReassemblySlot; key on counter && type. |
| 3A.3 | MD-3: Signal name not null-terminated | UDPSourceSession.cpp:219-223 |
After memcpy, force name[63]='\0' and unit[31]='\0'. |
| 3A.4 | MD-6: No auth on UDP CONNECT | UDPSServer.cpp:655-723 |
Document trust boundary in Docs/Protocol.md. Optional: add a ConnectToken config key. |
| 3A.5 | MD-13: Reassembler unbounded map growth (Go) | reassembler.go:41-89 |
Add maxSets = 1024 cap; reject new sets when full. |
| 3A.6 | LO-1: totalFrags overflow |
UDPSServer.cpp:541-542 |
Validate payloadSize <= maxPayloadSize * 65535 before the calculation. |
| 3A.7 | LO-17: bufMutex.Create unchecked |
UDPStreamer.cpp:119; UDPStreamerClient.cpp:149 |
Check return value; REPORT_ERROR on failure. |
| 3A.8 | LO-19: Reassembler ticker panic | reassembler.go:93 |
Guard if r.expiry <= 0 { r.expiry = 2 * time.Second }. |
Est. effort: 2 h
3B — WebSocket and JSON robustness (C++ clients)
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 3B.1 | MD-16: readU16/readU32 silent failure |
Client/streamhub/Protocol.cpp:21-38 |
Change readU16/readU32/readF64 to return bool (or set an ok flag); ParseBinaryFrame fails fast on any truncated read. |
| 3B.2 | MD-17: JSON injection in command builders | Client/streamhub/Protocol.cpp:183-213 |
Add a jsonEscape(str) helper; use it for all %s string interpolations. Switch to std::string to avoid truncation. |
| 3B.3 | MD-18: strstr-based JSON parsing |
Client/streamhub/Protocol.cpp:296-310, 495, 510 |
Migrate ParseSources, ParseZoom, ParseStats to a real JSON parser. ImGui: add a minimal JSON parser or vendor a single-header library (e.g. nlohmann/json). Qt: use QJsonDocument. |
| 3B.4 | MD-19: WS RFC 6455 violations | WSClient.cpp:204-223 |
(a) Reject control frames with payloadLen > 125. (b) Implement CONTINUATION opcode reassembly (or at least log and drop with a clear message). (c) Echo CLOSE frame. |
| 3B.5 | MD-20: Handshake no timeout | WSClient.cpp:290-301 |
Set SO_RCVTIMEO to 5s on the socket before the handshake loop. |
| 3B.6 | MD-5: SHA1 latent overflow | SHA1.h:50; WSFrame_client.h:113 |
Add if (len > 119u) return; guard; use uint64_t bitLen; use std::vector instead of new[]/delete[]. |
| 3B.7 | MD-24: parseCapture panic |
Test/E2E/chain/client/main.go:140-171 |
Add bounds checks before each read, mirroring parsePush. |
Est. effort: 4 h (3B.3 is the largest item — JSON parser migration)
3C — Go hub and debugger hardening
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 3C.1 | MD-10: No WS client cap | hub.go:367-377 |
Track len(h.clients); reject above configurable max (default 32). |
| 3C.2 | MD-11: Silent data loss | hub.go:346-358 |
Add a droppedCount atomic counter per channel; expose via Snapshot(). |
| 3C.3 | MD-12: SSRF via addSource |
hub.go:83-96; sources.go:62-67 |
Validate addr against a configurable allowlist (default: localhost + private RFC1918 ranges; reject link-local/metadata endpoints like 169.254.169.254). |
| 3C.4 | MD-14: Index panic | martecontrol.go:543 |
Use strings.TrimPrefix(line, "OK SERVICE_INFO ") with a length check. |
| 3C.5 | LO-14: stopCh double-close |
martecontrol.go:182-189 |
Use sync.Once for closing stopCh. |
| 3C.6 | LO-10: unsafe.Pointer aliasing |
hub.go:588-594 |
Replace float64ToBytes with binary.LittleEndian put operations. |
| 3C.7 | LO-11: +Inf in JSON |
stats.go:115-116 |
Guard if avg > 0 { si.RateHz = 1.0 / avg } else { si.RateHz = 0 }. |
Est. effort: 2 h
3D — TcpLogger and DebugService fixes
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 3D.1 | MD-7: StringHelper::Copy overflow |
TcpLogger.cpp:87 |
Replace with strncpy(entry.description, description, MAX_ERROR_MESSAGE_SIZE-1); entry.description[MAX_ERROR_MESSAGE_SIZE-1]='\0'; |
| 3D.2 | MD-8: volatile indices + lost wakeup |
TcpLogger.cpp:83-153, 157-158 |
Use Atomic::Load/Store for writeIdx/readIdx; use eventSem.ResetWait() instead of Wait+Reset. |
| 3D.3 | MD-9: printf on RT thread |
TcpLogger.cpp:75-76 |
Add a MirrorToStdout config key (default false); guard the printf/fflush behind it. |
| 3D.4 | MD-21: Stack buffer + shadowed member | DebugService.cpp:438, 489 |
Remove the local udpsSampleBuf (use the member); heap-allocate cfgBuf. |
| 3D.5 | MD-23: configValidated read without lock |
UDPStreamerClient.cpp:463 |
Mark volatile or acquire bufMutex before reading. |
| 3D.6 | MD-22: Spinlock on RT path | UDPStreamer.cpp:856, 947-976 |
Minimize the RT-side critical section: swap a pointer instead of memcpy under the lock. Move the memcpy outside the lock (double-buffer pattern). |
| 3D.7 | LO-7: JSON escaping in DISCOVER | DebugServiceBase.cpp:900-906 |
Use the existing EscapeJson helper for signal names. |
| 3D.8 | LO-8: EvaluateBreak only element 0 |
DebugBrokerWrapper.h:61-86 |
Document the limitation in the function comment. |
| 3D.9 | LO-9: fprintf(stderr) on init |
DebugBrokerWrapper.h:195-197 |
Replace with REPORT_ERROR. |
Est. effort: 3 h
Validation: Full test suites + E2E. For 3B.3 (JSON parser migration), add unit tests for crafted JSON inputs (nested quotes, escaped chars, truncated payloads). For 3A.1/3A.2, add a unit test that sends duplicate high-index fragments and mixed-type same-counter fragments.
Commit: fix(robustness): medium-severity input validation, parser, and DoS fixes (MD-1..MD-24)
Phase 4 — Low-severity hardening and documentation
Goal: Clean up latent bugs, fix doc mismatches, add missing hardening. These are non-urgent but improve code health.
| # | Bug | File(s) | Fix |
|---|---|---|---|
| 4.1 | LO-2: Stop() TOCTOU |
WSServer.cpp:104-134 |
Replace Sleep(200ms) with thread join. |
| 4.2 | LO-3: Spinlock priority inversion | UDPSourceSession.h; WSServer.h |
Document that FastPollingMutexSem is only for very short critical sections on same-core RT configs. Consider MutexSem for non-RT-contended paths. |
| 4.3 | LO-4: SignalBuffer mod-0 |
SignalBuffer.h:36-41 |
Guard push/readLast/readRange against capacity == 0. |
| 4.4 | LO-5: Misleading "Thread-safe" comment | SignalBuffer.h:18 |
Remove the claim or add internal locking. |
| 4.5 | LO-6: GAM type validation + doc | SineArrayGAM.cpp:81; TimeArrayGAM.cpp:54; TimeArrayGAM.h:8,27 |
Add GetSignalType checks; update TimeArrayGAM.h doc from uint32 to uint64. |
| 4.6 | LO-12: Directory listing | Client/webui/main.go:26 |
Disable directory listings (return 404 for directories). |
| 4.7 | LO-13: No security headers | Client/debugger/main.go:55; Client/udpstreamer/main.go; Client/webui/main.go |
Add a middleware that sets X-Content-Type-Options: nosniff, X-Frame-Options: DENY, Content-Security-Policy: default-src 'self'. |
| 4.8 | LO-15: host_/port_ race |
WSClient.cpp:48-58 |
Protect with sendMutex_ or make atomic<uint16_t> + std::string guarded by a small mutex. |
| 4.9 | LO-16: ReadExactTCP edge case |
UDPSClient.cpp:474-487 |
Add a max-iterations guard. |
| 4.10 | LO-18: RangeMin < RangeMax validation |
UDPStreamer.cpp:403-404 |
Validate when quantType != None; REPORT_ERROR if rangeMax <= rangeMin. |
Est. effort: 2 h
Commit: fix(hardening): low-severity fixes, doc corrections, security headers (LO-1..LO-19)
Phase 5 — Cross-cutting refactors (optional, post-hardening)
These are not bug fixes but structural improvements that prevent the recurrence of the bug classes found in this audit.
| # | Refactor | Rationale | Est. effort |
|---|---|---|---|
| 5.1 | Shared validateBounds / validateCount helpers |
Centralizes the integer-overflow-prevention pattern; prevents future copy-paste bugs | 1 h |
| 5.2 | Real JSON parser in C++ clients (nlohmann/json or Qt's QJsonDocument) | Eliminates the entire class of strstr/snprintf JSON bugs (MD-16, MD-17, MD-18) |
4 h |
| 5.3 | poll()/epoll everywhere (replace all select+FD_SET) |
Eliminates the FD_SETSIZE limitation entirely (HI-6) |
2 h |
| 5.4 | Auth framework for DebugService + web UIs | Token-based auth shared between the Go web UIs and the C++ DebugService; eliminates the "no auth anywhere" theme | 1 d |
| 5.5 | Fuzzing harness for UDPS protocol parsers | libFuzzer or go-fuzz harnesses that feed random bytes to ParseConfig/ParseData/DecodeElems/ParseBinaryFrame; catches future overflow variants |
1 d |
| 5.6 | Thread-sanitizer and address-sanitizer CI runs | make CXXFLAGS="-fsanitize=address,undefined"; go test -race; catches UAF and races automatically |
4 h |
Verification checklist (run after each phase)
source env.sh
# C++ build + tests
make -f Makefile.gcc clean
make -f Makefile.gcc core apps test
./Build/x86-linux/GTest/MainGTest.ex
./Build/x86-linux/Test/Integration/Integration/IntegrationTests.ex
# Go tests (each module)
cd Common/Client/go && go vet ./... && go test ./... && cd -
cd Client/debugger && go vet ./... && go build ./... && cd -
cd Client/udpstreamer && go vet ./... && go build ./... && cd -
cd Test/E2E/chain/client && go vet ./... && go test ./... && cd -
# Python framework tests
cd Test/E2E/chain && python3 -m unittest tests_py && cd -
# Full E2E suite
./Test/E2E/chain/run_chain_e2e.sh --skip-build
# ASan/UBSan smoke test (after Phase 2+)
make -f Makefile.gcc clean
make -f Makefile.gcc CXXFLAGS="-fsanitize=address,undefined -g" core apps
./Build/x86-linux/GTest/MainGTest.ex
Timeline summary
| Phase | Scope | Est. effort | Risk reduction |
|---|---|---|---|
| 1 | Critical remote-exploitable (CR-1..CR-5) | ~4 h | Eliminates drive-by takeover + heap corruption |
| 2 | High crash/OOB/UAF (HI-1..HI-9) | ~7 h | Eliminates remote crash + memory corruption |
| 3 | Medium robustness/DoS/parser (MD-1..MD-24) | ~11 h | Hardens input validation + RFC compliance |
| 4 | Low hardening/doc (LO-1..LO-19) | ~2 h | Code health + defense in depth |
| 5 | Cross-cutting refactors (optional) | ~3 d | Prevents recurrence of bug classes |
Total (Phases 1-4): ~24 h of focused work. Phase 5 is optional and can be scheduled separately.