An unparseable packet permanently kills its nfqueue read loop - silent total packet-processing failure for that connection #32

New issue

Closed

opened 2026-07-07 14:35:20 +01:00 by danny · 9 comments

danny commented

2026-07-07 14:35:20 +01:00

(Migrated from gitlab.keyop.co.uk)

Found while investigating #4.

In getNfCallback (internal/nfq_forwarder/nfq_forwarder.go), the getPacketInfo error path (~lines 626-629, a malformed/unparseable packet) returns 1 from the callback without ever calling setVerdict on that packet.

go-nfqueue's read loop (socketCallback, vendored github.com/florianl/go-nfqueue@v1.3.2/nfqueue.go ~lines 419-421) treats any non-zero return from the callback as fatal: if ret := fn(m); ret != 0 { return } - this permanently stops that queue's entire read loop for the remaining life of the process. runConnection (nfq_forwarder.go ~lines 735-787) has no logic to detect this or restart the queue.

Impact: a single malformed or unparseable packet (e.g. a truncated/fragmented capture) doesn't just fail to get a verdict - it silently ends all packet processing for every destination on that connection for the rest of the process's uptime, with no error surfaced anywhere obvious and no automatic recovery short of a full process restart. This is a much larger blast radius than a single dropped packet.

Fix direction: getPacketInfo failure should still issue a verdict (presumably NfDrop, since the packet can't be safely forwarded) so the callback always returns 0, keeping the read loop alive.

Found while investigating #4. In `getNfCallback` (`internal/nfq_forwarder/nfq_forwarder.go`), the `getPacketInfo` error path (~lines 626-629, a malformed/unparseable packet) returns `1` from the callback **without ever calling `setVerdict`** on that packet. `go-nfqueue`'s read loop (`socketCallback`, vendored `github.com/florianl/go-nfqueue@v1.3.2/nfqueue.go` ~lines 419-421) treats any non-zero return from the callback as fatal: `if ret := fn(m); ret != 0 { return }` - this **permanently stops that queue's entire read loop** for the remaining life of the process. `runConnection` (`nfq_forwarder.go` ~lines 735-787) has no logic to detect this or restart the queue. **Impact**: a single malformed or unparseable packet (e.g. a truncated/fragmented capture) doesn't just fail to get a verdict - it silently ends *all* packet processing for *every destination* on that connection for the rest of the process's uptime, with no error surfaced anywhere obvious and no automatic recovery short of a full process restart. This is a much larger blast radius than a single dropped packet. **Fix direction**: `getPacketInfo` failure should still issue a verdict (presumably `NfDrop`, since the packet can't be safely forwarded) so the callback always returns 0, keeping the read loop alive.

danny commented

2026-07-07 14:36:04 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in issue #4

danny commented

2026-07-07 14:46:08 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !70

danny commented

2026-07-07 15:33:18 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit ff463817f6

mentioned in commit ff463817f61c086e9e2f9addaa2ae7efefc735fd

danny commented

2026-07-07 15:33:39 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !71

danny commented

2026-07-07 15:33:50 +01:00

(Migrated from gitlab.keyop.co.uk)

Implementation is up for review: !71 (branch issue32_dont_kill_queue_on_bad_packet). getPacketInfo failure now sets an explicit NfDrop verdict and returns 0, rather than returning 1 with no verdict at all (which permanently killed that queue's read loop per go-nfqueue's contract). Waiting on review before merging.

Implementation is up for review: !71 (branch `issue32_dont_kill_queue_on_bad_packet`). `getPacketInfo` failure now sets an explicit `NfDrop` verdict and returns 0, rather than returning 1 with no verdict at all (which permanently killed that queue's read loop per go-nfqueue's contract). Waiting on review before merging.

danny commented

2026-07-07 15:47:05 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit a1cae03277

mentioned in commit a1cae03277afbae948336574a9f0a3ae0a63424d

danny (Migrated from gitlab.keyop.co.uk) closed this issue

2026-07-07 15:47:06 +01:00

danny commented

2026-07-07 15:50:43 +01:00

(Migrated from gitlab.keyop.co.uk)

Merged via !71 (merge commit a1cae03) and released as v1.1.2 (patch bump).

Merged via !71 (merge commit `a1cae03`) and released as **v1.1.2** (patch bump).