Skip to content

Fix sniffer decryption, threaded snifftest, and the shared-state teardown - #11319

Merged
dgarske merged 6 commits into
wolfSSL:masterfrom
Frauschi:sniffer_fixes
Sep 2, 2026
Merged

Fix sniffer decryption, threaded snifftest, and the shared-state teardown#11319
dgarske merged 6 commits into
wolfSSL:masterfrom
Frauschi:sniffer_fixes

Conversation

@Frauschi

@Frauschi Frauschi commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Nine sniffer fixes. Three of them mean whole classes of capture could not be decrypted at all, and one means a sniffer build could not complete a DTLS 1.3 handshake at all.

Encrypt-Then-MAC was never implemented. EXT_ENCRYPT_THEN_MAC was defined in src/sniffer.c but never handled in the ProcessServerHello extension switch, so startedETMRead was never set and the trailing MAC was passed to wc_AesCbcDecrypt() along with the ciphertext. The length is then not a block multiple and every record fails with BAD_FUNC_ARG. wolfSSL peers negotiate RFC 7366 by default, so no TLS 1.2 CBC capture between wolfSSL peers could be decrypted. A build without Encrypt-Then-MAC support now says so through a new error instead of failing every record generically; the complaint is deferred until the suite is known, since RFC 7366 covers only block ciphers.

Reported plaintext lengths included the MAC or AEAD tag. The length came from the record size without removing ssl->keys.padSz, so 14 bytes were reported as 30 under TLS 1.2 AES-GCM. The plaintext was correct, which is why this survived. For WOLFSSL_SNIFFER_STORE_DATA_CB this is a heap buffer overread - the callback pointer already sits at the plaintext start, so the extra bytes run off the decrypt output buffer. ASan reports a READ of size 68 past the allocation before the fix, clean after.

The X25519 static ephemeral key had no RNG. curve25519 blinding is on by default for the C implementation and draws from the key's own RNG, but the sniffer built its key with wc_curve25519_init_ex() and wc_Curve25519PrivateKeyDecode(), neither of which sets one. wc_RNG_GenerateBlock() then returned BAD_FUNC_ARG with every argument valid, and no X25519 traffic could be read. The library's own static ephemeral path and the sniffer's ECC path already called the matching setter; only X25519 was missed.

The rest:

  • A specific error was reported as a key mismatch. The client_key_exchange handler and the TLS 1.3 ServerHello path overwrote whatever SetupKeys() had reported with the generic "Server Client Key Mismatch", discarding the real reason every time. This is what made the X25519 failure look like a key problem.
  • snifftest could not report a failed capture. The read loop assigned hadBadPacket instead of accumulating it, so an early error was erased by any later clean packet. A new opt-in -expectdata additionally fails a run that decrypted nothing at all.
  • THREADED_SNIFFTEST decoded nothing. The worker checked its shutdown flag before draining its queue, and reading a capture file sets that flag before the worker is first scheduled. Separately, the server and secret tables are thread-local, so workers had to repeat main()'s keylog setup and never did.
  • ssl_FreeSniffer() tore down process-wide state from every thread. The trace file, mutexes and crypto device are shared, but every call released them, so a worker finishing early closed the trace file under threads still running. The init entry points now count their callers and only the last free releases the shared state.
  • A sniffer build could not complete a DTLS 1.3 handshake. With WOLFSSL_SNIFFER defined, the example client and server pin themselves to a static RSA and static ECC cipher list so a capture can be decrypted. The guard for that read version < 4, meant to leave TLS 1.3 alone, but the examples encode a DTLS version as a negative number and DTLS 1.3 is -4, so a -u -v 4 run was handed a TLS 1.2 only cipher list. The server then found no common TLS 1.3 suite in MatchSuite_ex() while processing the ClientHello and failed with MATCH_SUITE_ERROR, reaching the client as a missing_extension alert. This also took out the DTLS cases of scripts/ocsp-stapling_tls13multi.test. Only the examples were affected; an application that does not set that cipher list was always able to handshake.
  • The example store-data callback overflowed on a multi-record packet. It sized its buffer from the first record while the callback offset restarts at zero for each one.

Testing

Nothing failed before because three things swallowed it: hadBadPacket was assigned rather than accumulated, the keylog leg read tee's exit status instead of snifftest's, and every other leg checked only the exit code, never the lengths. On top of that both sniffer-tls13-x25519*.pcap were checked in at 24 bytes - a bare pcap header with no packets - so those legs had always tested nothing.

  • New sniffer-tls12-etm.pcap and -etm-keylog.pcap, with a tls12-etm branch in sniffer-gen.sh. Two suites deliberately: HMAC-SHA1 makes the pre-fix decrypt fail outright, HMAC-SHA256 silently decrypts the wrong byte range.
  • Both X25519 captures regenerated and non-empty; sniffer-static-rsa.pcap and sniffer-ipv6.pcap regenerated Encrypt-Then-MAC-free, so IPv6 is no longer gated on an unrelated feature.
  • The Encrypt-Then-MAC and keylog legs now diff reported lengths against a stored .out oracle. Reverting the padSz fix makes the EtM leg report 36/52/48/64 where 14/22/14/22 are correct.
  • sniffer-gen.sh regenerates the oracles itself, takes capture names, and fails on a name that matched nothing. Six keylog assets had never been in any .am, so make dist shipped the test but not its captures; fixed.

Three CI entries added to .github/configs/os-check-linux.json, each covering a path no other entry compiles: sniffer-keylog-storedata (WOLFSSL_SNIFFER_KEYLOGFILE and WOLFSSL_SNIFFER_STORE_DATA_CB), sniffer-no-enc-then-mac (the new diagnostic and its assertion), and sniffer-threaded (THREADED_SNIFFTEST, session tickets off so the concurrent path is actually exercised).

The DTLS 1.3 fix is verified separately, since no CI entry reaches it - the only --enable-sniffer leg has no --enable-dtls13, so ocsp-stapling_tls13multi.test skips its DTLS cases there. Built --enable-all --enable-debug --enable-sniffer: before the fix a plain server -u -v 4 / client -u -v 4 pair fails deterministically and ocsp-stapling_tls13multi.test fails at TEST CASE DTLS-1; after it, both pass, TLS 1.3 is unchanged, and sniffer-testsuite.test still passes.

@Frauschi Frauschi self-assigned this Aug 31, 2026
@wolfSSL-Bot

wolfSSL-Bot commented Aug 31, 2026

Copy link
Copy Markdown

Can one of the admins verify this patch?

@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot previously requested changes Aug 31, 2026
View reviewed changes

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #11319

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread src/sniffer.c Outdated
Comment thread src/sniffer.c Outdated
Frauschi
Frauschi commented Aug 31, 2026
View reviewed changes
Comment thread src/sniffer.c Outdated
Frauschi
Frauschi commented Aug 31, 2026
View reviewed changes
Comment thread src/sniffer.c Outdated
wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot previously requested changes Aug 31, 2026
View reviewed changes

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #11319

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread src/sniffer.c Outdated
Frauschi
Frauschi commented Aug 31, 2026
View reviewed changes
Comment thread src/sniffer.c Outdated
@Frauschi
Fix sniffer Encrypt-Then-MAC support and reported plaintext length
6a35183 
The sniffer ignored the encrypt_then_mac extension (RFC 7366) in the
ServerHello. For a CBC suite that meant the trailing MAC was handed to the
block decrypt along with the ciphertext, so the length was not a multiple of
the block size and every record failed with BAD_FUNC_ARG. wolfSSL peers
negotiate Encrypt-Then-MAC by default, so no TLS 1.2 CBC capture taken from
one could be decrypted. Record the extension and remove the MAC before
decrypting.

The plaintext length returned to the caller, printed by snifftest as
SSL App Data(packet:length), came from the record size without subtracting
what DecryptMessage() had already worked out in ssl->keys.padSz. A 14 byte
payload was reported as 30 under TLS 1.2 AES-GCM and as 31 under TLS 1.3.
Subtract padSz so the sniffer removes the same bytes the ordinary read path
removes from ssl->curSize.

Both failures were invisible because the tests could not fail. snifftest
overwrote hadBadPacket on every packet instead of accumulating it, so errors
early in a capture were erased by later good packets and the process still
exited 0. The keylog leg read $? after a pipe into tee, which reports tee's
status rather than snifftest's. Accumulate the flag, use PIPESTATUS, and
treat a saved capture that yields no plaintext at all as a failure.

The two static RSA captures decode correctly again and the keylog reference
output is regenerated from the checked-in pcaps, which had drifted by a
packet since the files were last produced on 5.6.3.

The X25519 legs are skipped: both captures are in the tree as a bare 24 byte
pcap header with no packets. sniffer-gen.sh could never produce them because
its configure line for those two builds without ed25519, so the example
client and server fail to load their certificates and tcpdump writes an empty
file. Add the missing option, and pin the group with --disable-mlkem so the
capture negotiates X25519 rather than the X25519MLKEM768 hybrid the static
key does not match. Regenerating them is left out of this change: the sniffer
cannot yet derive the shared secret for an X25519 static ephemeral key, which
is a separate defect.
@Frauschi
Address review findings on the sniffer Encrypt-Then-MAC fix
960551c 
Hand the corrected plaintext length to the WOLFSSL_SNIFFER_STORE_DATA_CB
callback. The previous commit fixed the length only for the branch that
copies into the caller's buffer; the callback branch still passed the raw
record size, which spans the explicit IV, the tag or MAC and any padding.
The callback pointer already points at the start of the plaintext, so the
extra bytes run off the end of the decrypt output buffer, which leaves only
a record header of slack. AddressSanitizer reports a heap-buffer-overflow
read of 68 bytes on a TLS 1.2 CBC capture before this change and is clean
after it.

Reject a non-empty encrypt_then_mac extension in the ServerHello. RFC 7366
section 3.1 specifies empty extension_data and TLSX_EncryptThenMac_Parse
enforces it, as do the neighbouring length-constrained cases in the same
switch.

Clear the Encrypt-Then-MAC decision at the start of every ServerHello.
Nothing reset it, so a renegotiation that dropped the extension kept
stripping a MAC that was no longer there, and one that added it started
stripping while the previous cipher was still active.

Reset the decrypted flag at doMessage and gate the padSz subtraction on it.
The flag is set inside the decrypt block and was never cleared, so it
described whichever earlier record in the packet had last been decrypted.
The handshake case already guards the same value with it.

Set the decoded-data flag on the WOLFSSL_ASYNC_CRYPT drain path too. A
record that goes pending is completed inside SnifferAsyncPollQueue rather
than DecodePacket, so an async build could decrypt a capture correctly and
still report that nothing was decrypted. Send that diagnostic to stderr
rather than into the decoded output, and note the exit status in the usage
text so a wrapper is not surprised by it.

Add a TLS 1.2 CBC Encrypt-Then-MAC capture and the sniffer-gen.sh recipe
that produces it, so the fix has a regression test that can be regenerated.
The existing static RSA and IPv6 captures exercise the same path but no
script in the tree can rebuild them. The new capture covers both failure
modes: with an HMAC-SHA1 suite the old code fails the decrypt outright,
and with HMAC-SHA256, whose MAC is a multiple of the block size, it
silently decrypts the wrong byte range and emits 48 bytes where 14 are
correct.

Warn when the Encrypt-Then-MAC gate drops the TLS 1.2 CBC captures, so a
build configuration that loses coverage says so, and regenerate the keylog
reference output with a binary that carries the new feature token.
@Frauschi
Give the sniffer's X25519 static ephemeral key an RNG
d065b14 
curve25519 blinding is enabled by default for the C implementation, in
settings.h, and draws from the private key's own RNG on every shared secret.
The sniffer built its key with wc_curve25519_init_ex() followed by
wc_Curve25519PrivateKeyDecode(), and neither sets one, so the shared secret
reached wc_RNG_GenerateBlock() with a NULL RNG and came back BAD_FUNC_ARG
while every argument it was given was valid. SetupKeys() then reported a
server and client key mismatch and no X25519 session could be read at all.

The library's own static ephemeral path already calls
wc_curve25519_set_rng() under the same guard, and the sniffer already calls
wc_ecc_set_rng() for the ECC key a few lines above. Only X25519 was missed.

Regenerate the two X25519 captures. Both were committed as a bare 24 byte
pcap header with no packets in them, so those legs had never tested anything;
they now hold real traffic and fail against the unfixed sniffer.

Drop the has_packets guard added earlier in this branch. It existed to skip
the empty captures, and with every capture holding packets it is dead code.
Removing it is also the better behaviour: snifftest already fails a capture
that yields no plaintext, so an empty file now fails the suite loudly instead
of being skipped, which is what a missing fixture deserves.
@Frauschi
Fix threaded snifftest dropping every queued packet
1a30387 
snifferWorker() looped while the shutdown flag was clear and only then
drained its queue. Reading a capture file, main() enqueues every packet
and then sets that flag and joins, normally all before the worker thread
is scheduled for the first time, so the worker woke up, saw the flag and
returned without decoding a single packet. Instrumenting the loop shows
it exiting with zero iterations and a non-empty queue, at -O0 as well as
-O1, so this is an ordering race rather than a hoisted load.

Keep going after shutdown for as long as packets are queued. The check
has to be its own helper that takes the worker's semaphore, because a
placeholder head means an empty queue: looping on worker->head alone
would spin forever on a placeholder nothing will ever fill. shutdown and
unused are polled across threads, so mark them volatile.

That alone left -keylogfile broken in this build. The sniffer's server
and secret tables are thread local, so every worker has to repeat the
setup main() did for itself, but the workers only ever called load_key()
and so reported every packet as coming from an unregistered server. Carry
the keylog path in SnifferWorker and load the secrets and create the
keylog sniffer server per worker.

Verified with --enable-sniffer CFLAGS=-DTHREADED_SNIFFTEST: the whole
sniffer test suite passes, and the multi-session captures decode the same
application data at 1, 2, 4 and 8 threads as they do without threading.
@Frauschi
Only release the sniffer's shared state on the last free
3c503f5 
The sniffer's session, server and secret tables are qualified THREAD_LS_T, so
each thread that calls ssl_InitSniffer*() owns its own. Its trace file, its
mutexes and its crypto device are not: they are shared by the whole process.
ssl_FreeSniffer() released all of it on every call, so in a build such as
snifftest with THREADED_SNIFFTEST, where every worker thread initializes and
frees the sniffer for itself, a thread that finished early closed the trace
file and freed the mutexes under the threads still running. A worker that was
still registering its key then crashed inside ssl_SetNamedEphemeralKey(),
writing to a file another thread had closed:

  flockfile -> vfprintf -> fprintf -> ssl_SetNamedEphemeralKey -> load_key

Count the callers of ssl_InitSniffer_ex(), which ssl_InitSniffer() and
ssl_InitSniffer_ex2() both funnel through, and only release the shared state
once the count reaches zero. The count is atomic where the platform offers
atomics and a plain int otherwise, which covers the single threaded build. The
per-thread tables are still freed on every call, and wolfSSL_Cleanup() is
still called every time, since it keeps a count of its own.

Verified with two ssl_InitSniffer() callers and one ssl_FreeSniffer(): before
this the trace file was closed by the early free and everything traced after
it was lost, now both entries are present. The full sniffer test suite passes
with and without THREADED_SNIFFTEST.
@Frauschi
Let a sniffer build complete a DTLS 1.3 handshake
4c4856f 
With WOLFSSL_SNIFFER defined, the example client and server pin themselves to
a static RSA and static ECC cipher list so a capture can be decrypted. The
guard for that read "version < 4", meant to leave TLS 1.3 alone, but the
examples encode a DTLS version as a negative number and DTLS 1.3 is -4, so a
"-u -v 4" run was handed a TLS 1.2 only cipher list.

The server then found no common TLS 1.3 suite in MatchSuite_ex() while
processing the ClientHello and failed with MATCH_SUITE_ERROR, which reached
the client as a missing_extension alert. Every DTLS 1.3 handshake between the
two examples failed in a sniffer build, which also took out the DTLS cases of
scripts/ocsp-stapling_tls13multi.test. Only the examples were affected; an
application that does not set that cipher list was always able to handshake.
@Frauschi
Frauschi requested review from wolfSSL-Fenrir-bot and removed request for wolfSSL-Fenrir-bot September 1, 2026 07:33
wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot reviewed Sep 1, 2026
View reviewed changes

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #11319

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot dismissed stale reviews from themself September 1, 2026 08:04

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

@Frauschi Frauschi assigned wolfSSL-Bot and unassigned Frauschi Sep 1, 2026
@dgarske
dgarske merged commit 021ec85 into wolfSSL:master Sep 2, 2026
381 checks passed
@Frauschi
Frauschi deleted the sniffer_fixes branch September 2, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left review comments
@dgarske dgarske dgarske approved these changes

Assignees

@dgarske dgarske

@wolfSSL-Bot wolfSSL-Bot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Frauschi @wolfSSL-Bot @dgarske @wolfSSL-Fenrir-bot