Skip to content

fwdmachine: verify upstream TLS certificates - #5141

Open
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:fwdmachine-verify-upstream-tls
Open

fwdmachine: verify upstream TLS certificates#5141
KernelClint wants to merge 1 commit into
secdev:masterfrom
KernelClint:fwdmachine-verify-upstream-tls

Conversation

@KernelClint

Copy link
Copy Markdown
Contributor

ForwardMachine in TLS mode terminates the client's connection and opens its own onward connection
to the real server. The downstream side is an interception by design — it generates lookalike
certificates — but the upstream side is an ordinary TLS client connection.

scapy/fwdmachine.py:365-379
loads the system certificates and then switches both forms of authentication off:

clisslcontext = ssl.SSLContext(ssl.PROTOCOL_TLS)
clisslcontext.load_default_certs()
clisslcontext.check_hostname = False
clisslcontext.verify_mode = ssl.CERT_NONE

Anything on the path between the machine and the origin can therefore present any certificate and
have its data relayed onward. A self-signed upstream and a validly signed one for a different
hostname both went through.

The change uses Python's default client policy, which requires a trusted chain and checks the
hostname:

-            clisslcontext = ssl.SSLContext(ssl.PROTOCOL_TLS)
-            clisslcontext.load_default_certs()
-            clisslcontext.check_hostname = False
-            clisslcontext.verify_mode = ssl.CERT_NONE
+            if self.verify_upstream:
+                clisslcontext = ssl.create_default_context()
+            else:
+                clisslcontext = ssl.SSLContext(ssl.PROTOCOL_TLS)
+                clisslcontext.load_default_certs()
+                clisslcontext.check_hostname = False
+                clisslcontext.verify_mode = ssl.CERT_NONE

Two details matter for not breaking working setups.

The hostname checked comes from the downstream ClientHello's SNI, and a client may send none. The
name passed to wrap_socket() therefore falls back to the destination, matching the idiom the
certificate cache already uses at scapy/fwdmachine.py:349 (ident = server_name or dest):

-                ss = clisslcontext.wrap_socket(ss, server_hostname=server_name)
+                ss = clisslcontext.wrap_socket(
+                    ss, server_hostname=server_name or dest[0]
+                )

And because interception against an upstream that cannot be verified is a legitimate use of this
class, verify_upstream=True is a constructor option rather than a hard change of behaviour.

The added regressions assert the verifying context is used by default, the opt-out still reaches
the old one, and a connection with no SNI is checked against the destination host. Reverting the
source with the tests in place fails them.

Performance was measured on one computer, before and after the fix: building the context took
758.7 ns before and 542.2 ns after — 28.5% faster. Repeat runs moved by about 3%, so this is
larger than the test's own variation. create_default_context() does in one step what the previous
four lines did separately.

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

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.72%. Comparing base (b3bbcc8) to head (d9cb53b).

Files with missing lines Patch % Lines
scapy/fwdmachine.py 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5141      +/-   ##
==========================================
+ Coverage   80.63%   80.72%   +0.09%     
==========================================
  Files         390      390              
  Lines       96936    96939       +3     
==========================================
+ Hits        78168    78258      +90     
+ Misses      18768    18681      -87     
Files with missing lines Coverage Δ
scapy/fwdmachine.py 31.43% <50.00%> (+31.43%) ⬆️

... and 6 files with indirect coverage changes

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

@KernelClint

Copy link
Copy Markdown
Contributor Author

The one red job here is not this change.

macos-14 3.14 both -k scanner fails at test/contrib/automotive/scanner/uds_scanner.uts:1104, on:

pkt = UDS_RMBARandomEnumerator._random_memory_addr_pkt()
pkt2 = UDS_RMBARandomEnumerator._random_memory_addr_pkt()
assert pkt != pkt2

Those are two independently random packets, so the assertion holds only most of the time. memorySizeLen and memoryAddressLen are each drawn from four values, and the payload is small, so a collision is not rare enough to never happen. It has nothing to do with the certificate verification in this pull request, which does not touch the automotive scanner.

Happy to send a separate pull request making that assertion deterministic if you would like — the straightforward fix is to draw until the two differ, or to compare the generator's spread over a number of draws instead of two single ones. Say the word and I will open it; I did not want to fold an unrelated test change into this one.

The rest of the matrix is green.

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