Repository navigation
Conversation
DecryptDo() decrypts AEAD records with the encrypt function so the tag is not checked, and passed the 13 byte decrypt.additional buffer as the tag output. GCM accepts a 13 byte tag, but wc_AesCcmCheckTagSize() rejects it, so every TLS 1.2 AES-CCM record failed with a decrypt error. This has been broken since 2ee6a01 swapped aead_mac_size for AEAD_AUTH_DATA_SZ to stop a 16 byte GCM tag overflowing that buffer. Write the tag into the output buffer after the plaintext, using the suite's tag size. The caller already counts that space as tag (padSz), and it stays valid until an async operation completes, which a stack buffer would not. Wipe it together with the nonce once the record is done. Also copy the explicit IV into the nonce only after checking the record is long enough, so a record shorter than 8 bytes is no longer read past its end. Add a static RSA AES-CCM-8 capture to sniffer-gen.sh and the sniffer test suite. The sniffer does not check the tag, so the test compares the plaintext against known good output. Have snifftest report aes_ccm support, and enable AES-CCM in the sniffer-curves-enckeys os-check entry. The Linux os-check job never installed libpcap, so snifftest was not built and none of its sniffer entries ran a capture. Install libpcap-dev there and add it to the ubuntu-24.04-minimal bundle list.
When a record decrypt returns WC_PENDING_E, DecryptMessage() polls and pops the event itself instead of calling the decrypt function again. That left ssl->decrypt.state at CIPHER_STATE_END, so for the next record DecryptTls() found no pending event, skipped DecryptDo() and returned VERIFY_MAC_ERROR. With the default WC_ASYNC_THRESH_AES_GCM of 128 bytes, any record that size or larger broke the one after it. The end state's work, wiping the nonce and the unused AEAD tag and mapping a failure to VERIFY_MAC_ERROR, never ran for the async record either. Once the poll completes, call the decrypt function again, as the TLS layer does, so it pops the event and, for TLS 1.2, its end state does that work. The version dispatch moves into DecryptRecord() for that. Add an async sniffer entry to the async workflow, with WC_ASYNC_THRESH_NONE so the short records in the captures go async, and with WOLFSSL_SNIFFER_KEYLOGFILE so the TLS 1.2 AES-GCM keylog capture runs. It is the only TLS 1.2 AEAD capture that goes async, as AES-CCM has no async path. The entry runs only the sniffer suite, since the API tests' 10-round memio handshakes do not complete when every AES operation is async. Install libpcap-dev in that job as well. Without it snifftest is not built and the suite skips every capture, so the entry also checks that snifftest exists.
There was a problem hiding this comment.
🔵 Needs a closer look
Cryptographic buffer and asynchronous state changes need human confirmation of synchronous and asynchronous capture-test results.
No open issues 🎉
What changed in this PR
Fixes TLS 1.2 AES-CCM and asynchronous record decryption in the wolfSSL sniffer, and enables Linux CI jobs to exercise capture decoding.
Changes:
- Corrects AEAD tag-buffer handling, checks record lengths before copying explicit IVs, and resumes decryption after asynchronous completion.
- Adds AES-CCM capture generation and expected-output regression checks.
- Installs libpcap in Linux CI and adds dedicated asynchronous sniffer coverage.
| File | Description |
|---|---|
sslSniffer/sslSnifferTest/snifftest.c |
Reports AES-CCM support for test gating. |
src/sniffer.c |
Fixes AEAD buffer handling and asynchronous decryption completion. |
scripts/sniffer-tls12-ccm.out |
Adds expected decrypted application data. |
scripts/sniffer-testsuite.test |
Adds capability-gated AES-CCM output checks. |
scripts/sniffer-gen.sh |
Adds TLS 1.2 AES-CCM capture generation. |
scripts/include.am |
Includes CCM regression fixtures in distributions. |
.github/workflows/os-check.yml |
Installs libpcap for sniffer tests. |
.github/workflows/async.yml |
Installs libpcap and adds an asynchronous sniffer test entry. |
.github/configs/os-check-linux.json |
Enables AES-CCM in a sniffer configuration. |
.github/ci-deps/packages-ubuntu-24.04-minimal.txt |
Adds libpcap to the dependency bundle. |
Review effort: Balanced
|
@wolfSSL-Fenrir-bot review balanced |
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11906
Scan targets checked: wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): .github/ci-deps/packages-ubuntu-24.04-minimal.txt, .github/configs/os-check-linux.json, .github/workflows/async.yml, .github/workflows/os-check.yml, scripts/include.am, scripts/sniffer-gen.sh, scripts/sniffer-testsuite.test, scripts/sniffer-tls12-ccm.out, scripts/sniffer-tls12-ccm.pcap, sslSniffer/sslSnifferTest/snifftest.c
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes the sniffer's TLS record decryption and the async decrypt state handling, so a human should still look at it.
What was reviewed:
DecryptDo: the computed tag is written toplain + plainSz. That stays inside thesz - 8bytes of output, because the length check now runs before the explicit IV is copied into the nonce.DecryptTls: the tag area is only ForceZero'd in CIPHER_STATE_END, so an operation that returns WC_PENDING_E doesn't have its nonce or tag wiped while it is still in flight.DecryptMessage: the new poll and re-call loop throughDecryptRecordreplaces the singlewolfSSL_AsyncPop.DecryptRecordhandles unused parameters under WOLFSSL_NO_TLS12.
Extended reasoning...
The change touches src/sniffer.c's AEAD (AES-GCM/CCM) decrypt path and its async-crypt retry loop, plus test captures, the snifftest banner and CI workflow and config updates. It is crypto-adjacent code that handles decrypted record buffers, nonce and tag zeroing, and the async state machine. That makes it security-sensitive even though the sniffer intentionally skips tag verification. No bugs were found, but the crypto surface and the async control-flow change are why this defers to a human instead of approving.
Description
Fixes two TLS 1.2 decrypt bugs in the sniffer, and makes the Linux GitHub CI sniffer entries actually decode captures. Found through the Jenkins nightly-sniffer-test-v2 job, whose AES-CCM-8 captures fail on master.
DecryptDo()passed the 13-bytedecrypt.additionalbuffer as the tag output of the encrypt call it decrypts with.wc_AesCcmCheckTagSize()rejects 13, so every TLS 1.2 AES-CCM record failed (broken since 2ee6a01). The tag now goes into the output buffer after the plaintext with the suite's tag size, and is wiped with the nonce.DecryptMessage()popped the event itself and leftdecrypt.stateat END, so the next record failed withVERIFY_MAC_ERROR. It now calls the decrypt function again after the poll (newDecryptRecord()helper), as the TLS layer does.libpcap-dev, which is also added to the ubuntu-24.04-minimal bundle list.Testing
scripts/sniffer-tls12-ccm.pcap(static RSA, AES128/256-CCM-8, from the newtls12-ccmmode insniffer-gen.sh). Its sniffer-testsuite leg compares the decrypted app data withsniffer-tls12-ccm.out, as the sniffer does not check the tag, and is gated on the newaes_ccmentry in snifftest's banner.--enable-aesccmadded to thesniffer-curves-enckeysos-check entry.asynccrypt-sw-snifferentry in async.yml withWC_ASYNC_THRESH_NONE(capture records go async) andWOLFSSL_SNIFFER_KEYLOGFILE(runs the TLS 1.2 AES-GCM keylog capture). It runs only the sniffer suite and fails if snifftest was not built.