updates from codex security scan and testing#40
Merged
Conversation
WebSocket::decode() read the 0x7f-encoded extended payload length as a uint64 and then checked completeness with 'n < pos + 4 + payload_len'. For payload_len near UINT64_MAX the right-hand side wraps below n, the check passes, and the unmask loop / memmove walks over the fixed 1024-byte pending[] buffer. ASan confirmed the OOB. Bound payload_len against the pending buffer space before any addition. Add a regression test that fires three flavours of impossibly-large 0x7f frames at the engineer-side WS port and asserts the parent supportproxy stays alive.
handle_setup_signing() previously copied the packet's secret_key and
initial_timestamp straight into keys.tdb. The combination of all-zero
key + zero timestamp is exactly what load_signing_key() detected as
'disable signing entirely', so a single signed engineer packet could
persistently turn off signature verification on a port pair: the next
load nulled status->signing, and the generated mavlink_signature_check
returns true when signing == NULL, treating subsequent IFLAG_SIGNED
frames as valid without checking any secret.
Two layered defences:
* handle_setup_signing() now rejects the all-zero key + zero
timestamp combination at the source, before any DB write.
* load_signing_key() safe-fails on the same combination: leave
key_loaded=false so receive_message() rejects every frame on the
channel, rather than wiring NULL signing into status->signing.
The dead all_zero branch later in load_signing_key() is removed; the
early return makes the remaining branch unconditional.
Regression: a signed SETUP_SIGNING with zeros is rejected, the channel
keeps validating subsequent signed traffic with the original key, and
no 'Set new signing key' log appears.
The engineer-UDP idle-timeout close loop at supportproxy.cpp:446-454 called c2.close() without decrementing conn2_count or shrinking max_conn2_count. Repeated unauthenticated UDP churn from varying source tuples therefore monotonically grew max_conn2_count, and once it crossed MAX_COMM2_LINKS the per-port-pair child hit the BUG guard and exit(1)'d — DoSing the affected port pair until the parent forked a fresh child. Decrement the counters in the close loop, matching the pattern other close sites in this file already use. As defence in depth, replace the BUG exit(1) with a clamp + warning so an unexpected leak from some other future path can't take the child down. Regression: 50 unsigned UDP tuples → wait past the 10s idle threshold → 60 more tuples → child stays up. Pre-fix this drove max_conn2_count to 110 and tripped exit(1).
A conn2 slot used to be allocated at TCP accept or first UDP datagram,
before any signed MAVLink packet had a chance to validate. An
unauthenticated client could camp on a slot indefinitely (TCP) or
keep one alive by spamming traffic (UDP), so MAX_COMM2_LINKS attackers
could deny a legitimate signed engineer.
Add a pre-auth deadline: any slot whose MAVLink channel hasn't
validated a signed packet within CONN2_PREAUTH_SECONDS (5 s) of
slot creation is closed and its counters returned to the pool. We
compare against connected_at, not last_pkt, so traffic-spam can't
refresh the deadline. Authenticated UDP retains the existing 10 s
idle close; authenticated TCP/WS continues to live until disconnect.
MAVLink::is_authenticated() exposes got_signed_packet for this check.
Regression: 60 unsigned TCP hogs + 7 s wait + signed engineer connects
and validates ('Got good signature' appears in proxy log).
In bidi-sign mode, validate the signature of the first user-side UDP datagram before connect()ing the listener to that tuple, so unsigned or wrong-key senders cannot latch conn1 and deny the legitimate signed user. TCP/WS still latch on accept, so add a pre-auth deadline that restarts the child if no signed user packet validates in time.
check_children() and reload_ports() only ran when epoll_wait() returned 0 - a full second with no events. Children inherit the listening sockets, so the parent's epoll registrations survive its close() after fork and fire for traffic on active sessions; with any busy session the timeout never fired, exited children were never reaped and their listeners never reopened. A connection killed via the web UI stayed dead (UDP packets arriving, nothing bound) until every session went quiet, and keys.tdb reloads stopped too. Run housekeeping on every loop iteration and reload immediately after reaping a child so its reopened listeners get straight back into the epoll set. Add a regression test that keeps one pair streaming while killing the other pair's user connection, and a Robustness phase to the test runner so it runs in CI.
After fork the child holds references to the pair's socket descriptions, so the parent's close() does not remove the epoll registrations: every packet the child handled woke the parent for nothing, and stale fd numbers could alias newly opened sockets and trigger spurious forks. EPOLL_CTL_DEL the four fds while we still own them, and close the inherited epoll fd in the children.
fork() returning -1 was stored in p->pid after the pair's sockets were already closed: no child ever matches pid -1 in check_children and the pid==0 checks skip reopening, so the pair went permanently dead. Worse, the kill(p->pid, SIGTERM) calls in upsert_port/reload_ports guard only against pid==0 - with pid==-1 they would SIGTERM every process we can signal. Keep the sockets open and let the still-pending epoll event retry the fork.
MAVLink::ws had no initializer and init() never reset it: a fresh object relied on the fork-time stack happening to be zero, and when a WebSocket engineer disconnected, Connection2::close() deleted the wrapper but left mav.ws pointing at freed memory - the next plain TCP/UDP engineer reusing that slot sent through it and crashed the child, taking down the whole session. Default it to nullptr, reset it in init(), and clear it in Connection2::close().
Every engineer-slot close path shrank the scan watermark with 'if (conn2_count == max_conn2_count) max_conn2_count--', which fires whenever the slot table has no holes regardless of which slot closed. With engineers in slots 0 and 1, an EOF on slot 0 dropped the watermark to 1 and slot 1 fell out of the select/read/forward/idle loops: that engineer silently stopped receiving until a new connection happened to raise the watermark again. Move all close paths to one helper that only shrinks the watermark while the top slots are actually free. Add a regression test with two signed TCP engineers.
Nothing handled SIGPIPE and no send uses MSG_NOSIGNAL, so a write to a peer-closed TCP/WebSocket/SSL connection could terminate the per-pair child - ending the session for the user and every engineer. The socket_is_dead() precheck in send_message() is inherently racy and mav_printf() writes with no check at all. Ignore the signal so writes fail with EPIPE and go through the normal close paths.
The 5s connections.tdb snapshot deletes and rewrites every record for its port2 with flags=0, so a CONN_FLAG_DROP_REQUESTED set by the webadmin while a snapshot was in flight was wiped before the child's SIGUSR1 scan ran - the kill silently did nothing and nothing retried. Preserve DROP_REQUESTED across the snapshot rewrite (conn_drop_mask collects flagged indices inside the same transaction) and rescan for drop requests on the snapshot cadence as a safety net, so a request whose signal raced the rewrite is honoured within a few seconds.
open_socket_in_udp/open_socket_in_tcp returned -1 on bind()/listen() failure without closing the socket. The parent retries failed listeners every 5s, so a port held by another process slowly leaked fds toward EMFILE.
…failure reload_ports() exited the whole proxy when db_open_transaction() returned null, so one transient keys.tdb open failure at runtime took down every session. Skip the reload cycle instead. Give db_open() the same EBUSY retry/backoff conn_db_open() already has (keydb.py and the webadmin hold the open lock briefly during transactions), and make db_open_transaction() report a failed tdb_transaction_start() instead of carrying on without a transaction.
… partial headers decode() returned the same -1 for 'frame incomplete, wait for more' and 'frame can never fit in pending[]', and recv() treated both as no frame yet: a frame larger than the buffer (e.g. several MAVLink2 messages batched into one WS frame) stalled the connection forever with no close and no progress. Return a distinct fatal code and fail the connection so the owner closes it. The bound now accounts for the NUL byte fill_pending() reserves, which made exactly-buffer-sized frames uncompletable too. recv() also ran decode() on partial HTTP upgrade headers when the request arrived fragmented; the header bytes could parse as a bogus frame and be consumed, breaking the handshake. Skip decoding until the handshake is done.
WebSocket's error paths closed the socket fd it was given, but Connection2 / listen_port own that fd and close it again later. In between, the child can open new descriptors (lazy tlog/binlog session files, tdb handles in forked writers) that reuse the number, and the owner's later close then hits an unrelated file. Mark the wrapper dead and let recv()/send() report failure so the owner does the single close. Also add the missing destructor: SSL/SSL_CTX objects leaked on every WebSocket teardown in a long-lived child.
log_cleanup_loop() never returns, and the child ignores parent exit: every proxy shutdown outside systemd's cgroup kill left an orphan supportproxy process looping forever. Request SIGTERM on parent death and bail if the parent was already gone before prctl took effect.
The test pre-seeded an over-quota sparse file before starting the proxy, but log_cleanup_once() now runs an immediate quota pass at startup and deletes it, so the write-time gate saw an under-quota tree and the test failed. Seed after the cleanup child has started (its startup pass over the still-empty tree is instantaneous) so the write-time gate is what gets exercised. Pre-existing failure, not introduced by the robustness fixes.
Replace the hardcoded 1 GiB MAX_PER_PORT2_BYTES with port2_quota_bytes(), overridable via SUPPORTPROXY_PORT2_QUOTA_BYTES. One source of truth shared by the cleanup quota pass and binlog's write-time gate. 1 GiB is small for a LOG_DISARMED aircraft (~70 MB/hour observed), and the override also lets tests exercise quota behaviour with small real files instead of giant sparse ones.
The quota passes summed st_size, but .bin files are sparse: a gappy log's apparent size wildly overstates what it costs on disk, starving later sessions of quota they actually have. Count st_blocks*512 instead, in both the cleanup quota pass and binlog's write-time baseline.
The quota pass deleted oldest files only until total <= quota, so an active session's growth re-breached the cap within minutes and binlog writes stalled again until the next hourly pass (seen on a production server: relief at 02:01, stalled again by 02:09). Free down to 80% of the quota instead.
When the write-time quota gate tripped, every block was silently dropped until the next *hourly* cleanup pass — on a production server a preflight-rebooted aircraft got an empty .bin for exactly this reason (old sessions held the whole quota). Age out the oldest sessions for this port2 right at the gate (rate-limited to once per 30s), re-baseline, and only drop the block if the dir still can't get under quota.
Reboot rotation relied on seeing a SYSTEM_TIME backward jump before the vehicle resumed streaming. If the vehicle restarted first (our 5s keep-alive START reaches it before SYSTEM_TIME re-streams), its new log's block 0 overwrote the head of the old session file, and the later SYSTEM_TIME rotation then waited for a fresh seqno=0 that never comes - ArduPilot ignores redundant STARTs while streaming, so only its 10s client timeout eventually broke the stall. Detect the restart from the data itself: block 0 arriving while the file is open with highest_seen past 200 cannot be a retransmission (AP_Logger_MAVLink's resend window is ~32 blocks) - rotate on the spot. SYSTEM_TIME detection stays as the fallback for a vehicle that reboots and doesn't resume streaming.
supportproxy.o didn't list binlog.h, so a BinlogWriter layout change rebuilt binlog.o but left supportproxy.o with the old object size - the two then corrupted each other's stack (garbage session numbers, mass test failures) until a clean rebuild. Add binlog.h there and cleanup.h to binlog.o (newly included).
A per-pair child outlives the parent's SIGTERM by up to its 10s conn1 idle timeout while holding the fixed test ports; a test starting inside that window can't bind its listeners and every packet goes to the stale child, cascading failures through the file. Probe the TCP listen port until it frees before returning from _terminate().
…file When blocks are rejected by the strict-start gate (proxy restarted mid-flight, or a rotation is armed), the vehicle can only recover by restarting its log from seqno 0 - and ArduPilot ignores redundant STARTs while streaming, so recovery waited on its 10s no-ACK client timeout. Send the MAV_REMOTE_LOG_DATA_BLOCK_STOP magic (throttled to 2s) as soon as gated blocks arrive: the vehicle stops immediately and the existing START logic restarts it from 0, shrinking the log hole from ~10-15s to a second or two.
The write-time gate trips on prospective size, but the quota pass only freed when the current on-disk total already exceeded the cap. With old sessions sitting at or just under the quota, the inline cleanup freed nothing, the block was dropped, and - since nothing ever gets written - every retry hit the same wall forever, reproducing the empty-.bin field symptom at the boundary. Pass the needed headroom into the pass and free when total+needed exceeds the quota. Found by review (codex).
The quota pass deleted oldest-first with no active-file protection: an over-quota active session (made likelier by the write-time trigger and the 80% hysteresis) had its own open .bin/.tlog unlinked - on Linux the writer keeps appending to the invisible unlinked inode, the whole log is lost on close, and the vanished path stops being counted so real disk use can exceed the quota unseen. Skip (but still count) files with mtime under 60s. Found by review (codex).
The gate charged the active .bin's logical end offset while everything else was counted by allocated blocks: a legitimate sparse forward jump (allowed up to 100 MB) could falsely trip the quota, and the cleanup pass - correctly seeing allocated usage under the cap - would free nothing, leaving the stall permanent. fstat the open file and charge st_blocks*512 plus the incoming block. Found by review (codex).
The STOP nudge fired on the first gated block with target_system=0 (identity not yet latched): a broadcast STOP can stop a healthy remote-log stream owned by another collector on a multiplexed link, and right after a rotation a single delayed pre-reboot block could stop the freshly restarted stream before its seqno 0 arrived. Latch the sender's sysid/compid from the gated block so STOP is addressed, and only fire once 3 blocks have been gated since the last open/rotation - a lone straggler stays harmless while a genuinely mid-log stream crosses the threshold in well under a second. Found by review (codex).
SUPPORTPROXY_PORT2_QUOTA_BYTES='1GB' was prefix-parsed by strtoll to a 1-byte quota, which blocks every binlog write and lets the cleanup pass delete nearly the whole log tree; an out-of-range value became LLONG_MAX and disabled the quota entirely. Require a full-string positive parse and log the fallback to the default. Found by review (codex).
A 200-byte write into an unallocated region allocates a whole filesystem block, so projecting only BLOCK_BYTES let the total overshoot the allocated-byte quota by the difference. Use st_blksize. Also document the accepted residuals of the active-file mtime heuristic and the single-stream STOP semantics. Found by review (codex).
The bidi pre-latch path re-initialised mav1 for every unsigned datagram: each init loads the signing key from keys.tdb and forks a timestamp-save child, so a stream of unsigned traffic (an attacker, or simply a not-yet-signed user) caused per-packet disk I/O, a fork per packet, and tdb lock collisions that could stall the child's main loop. Initialise once at child start - parser and signing state carry across datagrams safely, and validation still gates the latch.
run_test_scenario connected the engineer at test start but only began its signed traffic after wait_for_connection_user, which burns its full 10s in the bidi negative tests (no conn1 latch by design). The proxy's 5s conn2 pre-auth deadline then closed the idle engineer socket and the client's handshake/reconnect raced the close - the flaky bidi failures under -j 16. Connect and sign the engineer after the wait so it authenticates well inside the pre-auth window, and stop sender threads from the finally so a mid-setup exception can't leak a running sender.
The connection phase runs at -j 16, which on a 2-core CI runner is 8x oversubscription. WS/WSS (and TCP) construction connects and handshakes synchronously; under that load the proxy child can be slow enough that pymavlink's own connect retries are exhausted, leaving self.sock unset - and older pymavlink then dereferences self.sock.fileno() and crashes the test at construction (AttributeError), before the body even runs. The four flaky bidi WS/WSS cases in CI were all this. Retry stream construction a few times, and treat a returned fd==None (newer pymavlink's graceful form of the same failure) as retryable.
WebSocket::send wrote MAVLink frames whenever the caller had data,
even before the HTTP upgrade 101 had been sent. When an engineer's
signed traffic was forwarded to a user whose WS/WSS handshake was
still in flight (a window that widens under load), the frame landed
ahead of the 'HTTP/1.1 101' status line and corrupted the handshake -
the peer's WS parser rejected the connection ('illegal status line'
starting with the MAVLink magic byte). Drop-but-report-success until
done_headers so forwarding resumes cleanly once the handshake
completes, without tearing down the session.
Also wire test_websocket_decode.py into the runner's Robustness phase
so these (and the existing decode-overflow) tests run in CI, and add a
deterministic regression test for the ordering.
Found via 2-core stress reproduction of a CI flake.
The WebSocket::send handshake guard didn't cover the earlier window: between TCP accept (which latches have_conn1) and the first user data event (which runs WebSocket detection and sets mav1's ws wrapper), mav1 is still in raw-TCP mode. An engineer frame forwarded in that window is written as plain MAVLink onto a socket that then turns out to be WebSocket, landing ahead of the HTTP 101 and corrupting the handshake — the field-observed 'illegal status line' starting with the 0xfd MAVLink magic. Gate the engineer->user forward on count1>0: WebSocket detection only runs while count1==0, so count1>0 means the transport is decided (raw or WS). For UDP conn1 have_conn1 already implies count1>0, so this only affects the TCP accept-but-no-data window. Deterministic regression test in its own isolated proxy (sticky conn1 makes a shared-fixture version racy). Found via 2-core stress reproduction of a CI flake.
WebSocket::detect classified a stream from its first bytes but had two faults: ws_prefix is a char* so sizeof() was 8 not 14, and it returned a plain bool with no 'need more data' state. A WS client whose first TCP read delivered 8-13 bytes of 'GET / HTTP/1.1' (common under load) was therefore misclassified as raw MAVLink: count1 advanced, conn1 stayed in raw mode, and the signed engineer's forwarded frame went out as raw MAVLink ahead of the eventual HTTP 101 — the field-observed handshake corruption, and the residual the count1 gate and send guard couldn't cover. Return a tri-state (WS_NO / WS_YES / WS_MORE): only commit to raw when the bytes definitively can't be a handshake (a raw MAVLink frame never starts with 'G' or 0x16), and wait for more when the prefix matches so far. Callers hold conn1/conn2 undecided on WS_MORE. Regression test covers the fragmented-prefix case alongside the two earlier windows. Found via 2-core stress reproduction of a CI flake.
A bidi negative test (unsigned / wrong-key user) is dropped by the proxy's 5s pre-auth deadline, which tears the child down and resets the engineer socket while the test is still draining it (test_duration sits on the deadline). recv_match then raised ConnectionResetError uncaught. Treat a transport error in the drain loop as end-of-stream (nothing was forwarded, which is exactly what the negative test asserts) and make connection teardown tolerant of an already-reset socket. A positive test never trips the deadline, so a spurious reset there still surfaces as an under-count failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security hardening, robustness fixes and binlog reliability work, driven by a codex security scan, a full robustness review, and two field incidents (a web-UI "kill" that never came back, and an empty .bin after a preflight reboot).
Security fixes (codex scan)
pending[])Robustness fixes
The headline bug: the parent only reaped children and reopened listeners when
epoll_wait()returned 0 — a full second with no events. Children inherit the listening sockets, so their traffic kept waking the parent through registrations that survivedclose()after fork; with any active session the timeout never fired. A connection killed via the web UI stayed dead (UDP arriving, nothing bound) until every session went idle, and keys.tdb reloads stopped too.EPOLL_CTL_DELa pair's sockets before the fork handoff closes themfork()failure (previously stored -1 inp->pid, permanently killing the pair and arming a laterkill(-1, SIGTERM))MAVLink::wsuninitialized/dangling (use-after-free child crash when a WebSocket engineer's slot was reused)DROP_REQUESTEDand the child rescans on the snapshot cadenceexit(1)s the proxybind()/listen()failure (slow leak toward EMFILE)BinlogWriterlayout change could leavesupportproxy.owith a stale object layout)Binlog quota + reboot handling
Field incident: with LOG_DISARMED logging, old sessions consumed the whole 1 GiB per-port2 quota; after a preflight reboot the new session file was created but every block was dropped until the hourly cleanup pass, leaving an empty .bin.
SUPPORTPROXY_PORT2_QUOTA_BYTES(plain bytes; strict parsing — malformed values fall back to the 1 GiB default)st_blocks), not sparse apparent size, on both the cleanup and write-gate sides; the active file is charged consistently, with growth projected as one filesystem blockhighest_seenfar along), instead of racing SYSTEM_TIME reboot detection and corrupting the old log's headTests
Every behavioural fix that could be exercised end-to-end has a red/green regression test (killed-pair recovery under load, conn2 slot orphaning, lost drop requests, quota boundary/immediate-cleanup/active-file/sparse-accounting cases, in-stream rotation, STOP nudge and its threshold, malformed quota env). The test runner gains a Robustness phase so these run in CI. Also fixed test-harness races (lingering per-pair children holding ports between tests) and a flaky bidi ordering where the engineer connected 10s before signing and collided with the intended 5s pre-auth deadline.