Skip to content

fwdmachine: drop a packet when its callback raises - #5132

Open
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:fwdmachine-callback-fails-closed
Open

fwdmachine: drop a packet when its callback raises#5132
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:fwdmachine-callback-fails-closed

Conversation

@KernelClint

Copy link
Copy Markdown
Contributor

ForwardMachine passes each packet to a caller-supplied transformation callback, which signals what
should happen by raising FORWARD, FORWARD_REPLACE, DROP, ANSWER or REDIRECT_TO. Callers
use it to decide what crosses the machine.

scapy/fwdmachine.py:471-531
also catches every other exception, logs it, and forwards the original packet. So a callback that
fails for a reason its author did not anticipate is treated as though it had said "forward".

That is reachable from the wire. Scapy's HTTP method detector
(scapy/layers/http.py:624-635) does not recognise PATCH, so a PATCH request is returned as raw
bytes; a callback that inspects pkt.Method then raises AttributeError, and the request it was
written to block is sent upstream unchanged.

The change treats an unexpected failure as a drop:

             except Exception as ex:
                 self.vprint(self.ERROR, ctx, cs, "callback error: %s" % ex, None)
-                data = req
+                continue

The diagnostic is unchanged. Every documented outcome behaves exactly as before; only the
undocumented failure case changes, from allowed to denied.

The added regression drives a callback that raises on an unrecognised method and asserts nothing
reaches the far side. Without the source change it fails.

Performance was measured on one computer, before and after the fix: a forwarded request took
33.3 µs before and 33.7 µs after. Repeat runs of this test moved by about 9%, which is wide enough
that only a large change would show — this is not one.

AI-Assisted: yes (GPT-5.6-Cyber)
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.73%. Comparing base (b3bbcc8) to head (572f862).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5132      +/-   ##
==========================================
+ Coverage   80.63%   80.73%   +0.09%     
==========================================
  Files         390      390              
  Lines       96936    96935       -1     
==========================================
+ Hits        78168    78264      +96     
+ Misses      18768    18671      -97     
Files with missing lines Coverage Δ
scapy/fwdmachine.py 35.00% <100.00%> (+35.00%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant