Mcdc test coverage part 8 - #11355
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new DTLS white-box has configuration-guard/compile-availability issues and the OCSP white-box currently leaks heap entries created by GetOcspEntry().
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds new MC/DC white-box drivers under tests/unit-mcdc/ to exercise previously unreachable file-static logic in src/ocsp.c, src/dtls.c, and src/crl.c, and wires them into the unit-mcdc smoke expectations and distribution list.
Changes:
- Introduces three new white-box translation units: OCSP, DTLS, and CRL.
- Updates
tests/unit-mcdc/smoke-expected.txtto include the new binaries in the smoke list. - Updates
tests/include.amto ship the new sources inEXTRA_DIST.
File summaries
| File | Description |
|---|---|
| tests/unit-mcdc/test_ocsp_whitebox.c | New OCSP white-box driver for MC/DC coverage of internal cache matching logic. |
| tests/unit-mcdc/test_dtls_whitebox.c | New DTLS white-box driver targeting stateless DTLS helpers and argument guards. |
| tests/unit-mcdc/test_crl_whitebox.c | New CRL white-box driver targeting CRL lookup argument-guard conditions. |
| tests/unit-mcdc/smoke-expected.txt | Adds the new white-box binaries to the expected passing set for the smoke script. |
| tests/include.am | Adds the new white-box sources to EXTRA_DIST. |
Review details
Suppressed comments (1)
tests/unit-mcdc/test_dtls_whitebox.c:343
- The skip stub message only mentions WOLFSSL_DTLS, but the driver also depends on DTLS 1.3 and CID helpers (and is excluded under WOLFCRYPT_ONLY). If this TU is skipped due to those feature macros, the current message is misleading for troubleshooting.
int main(void)
{
printf("dtls white-box: skipped (WOLFSSL_DTLS not built)\n");
return 0;
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed portability/build issues in test_wolfio_whitebox.c (unconditional POSIX socket headers and socketpair() usage) and a constant conditional expression in test_internal_clienthello_whitebox.c that may break -Werror builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 3
- Review effort level: Lite
15732a8 to
a7d1080
Compare
Every uncovered condition in dtls.c is in a file-static helper on the stateless path, where the public API fixes most arguments: no caller can ask CreateDtls12Cookie for a NULL secret or FindExtByType for a length that overruns its own vector. The dtls group already runs 87 of 103 and still leaves 46 of 56 uncovered, so the API limit is reached before the file is. Drives CreateDtls12Cookie, FindExtByType, ClientHelloSanityCheck, TlsCheckSupportedVersion and DtlsCidGetSize with paired vectors in one binary. dtls.c 10/56 -> 16/56.
crl.c measured 3 of 48: there is no crl test group, CRL lives in certman, and certman runs 11 of 36 on this option list. Drives CheckCertCRLCm's (serial == NULL || serialSz == 0) && serialHash == NULL guard with the four vectors MC/DC needs. crl.c 3/48 -> 6/48. Both drivers were including the target .c before wolfssl/options.h. config.h carries no feature macros, so the smoke build compiled them with WOLFSSL_DTLS and HAVE_CRL undefined: they took their skip stubs, exited 0, and were recorded as passing while testing nothing. options.h now comes first, which is the rule AGENTS.md already states. Under the smoke build they now drive 22 and 4 vectors respectively.
The ocsp group runs 3 of its 8 tests on the campaign option list, so most of ocsp.c is never entered from tests/api. GetOcspEntry's cache-match compares an entry against the request by issuer hash and issuer key hash; a caller coming through the public API builds both from the same certificate, so neither operand has a false case from outside and the loop body needs a seeded cache to execute at all. ocsp.c 7/47 -> 9/47.
InitSuites is a long table of 'tls1_2 && haveX && haveAES128' rows over twelve have* flags. No handshake can drive it: every caller derives those flags from what the build compiled in, so on one binary they are constant and no operand has an independence pair. A one-at-a-time sweep from an all-ones baseline, over six protocol versions and both sides, gives the pair for every operand of every row in n+1 calls instead of 2^n. 156 calls, internal.c 532/1730 -> 573/1722. Also extends the crl driver to CheckCertCRLCm's cm != NULL && cm->cbMissingCRL and cm != NULL && cm->crlCb guards, whose operand 0 is true by construction from wolfSSL_CertManagerCheckCRL.
Eight ocsp tests were registered with a bare TEST_DECL and no group, and ApiTest_RunGroup selects on group != NULL, so none of them has ever run under --group. Among them are both OCSP-to-CRL fallback tests and the DecodeUrl CR/LF injection test. Grouping them takes the ocsp group from 3 of 8 running to 9 of 17, and src/wolfio.c from 18/87 to 43/87. Adds test_wolfIO_DecodeUrl_host_bounds for the two host-parsing loops, whose four operands are each ended by a different vector: a bracketed IPv6 literal with no closing bracket, one cut short by a NUL, a host running to the end of the buffer, and a host longer than the item cap, with well-formed URLs as the accepting partners. Those are attacker-controlled shapes that no in-tree caller produces.
The previous vectors reached CheckCertCRLCm's cm != NULL && cm->cbMissingCRL and cm != NULL && cm->crlCb chains and covered nothing, because every vector took the decision false: once by short-circuit on a NULL cm, once because no callback was installed. An operand that changes value without changing the decision outcome has no independence pair. Installs both callbacks so the decisions go true, and drives the error callback's own return value both ways for the third operand of the :686 chain, plus a url longer than the 256-byte stack buffer for the copy guard inside the :668 body. crl.c 6/48 -> 11/48.
BufferLoadCRL and BufferStoreCRL are argument-validated with wide OR chains and then branch on DER vs PEM. Every in-tree caller passes a real CRL and a real type, so the rejecting side of each operand is unreachable from outside, while the accepting side needs a genuinely parsed entry: a hand-built one has no toBeSigned or signature, so it can only take the :1036 guard true and leaves the whole store path uncovered. Loads certs/crl/crl.der through the real loader and lets the CRL context own the entry, then stores it as DER, as PEM, into an undersized buffer and with a size query. crl.c 11/48 -> 18/48.
wolfio.c reads as needing a transport and does not. It needs a byte source and a descriptor, and both can be supplied locally. wolfIO_HttpProcessResponseGenericIO takes a WolfSSLGenericIORecvCb, which is int (*)(char*, int, void*), so the whole HTTP response state machine is drivable from a memory buffer. The mock caps bytes per call, which forces the reassembly loop to iterate and produces split headers and split chunks that a real socket would not reproduce on demand, and it can return -1 to inject a read error with no transport involved. socketpair(AF_UNIX) covers the descriptor half: wolfIO_SockIsDGram wants an fd, not a peer, and a stream pair, a datagram pair and a closed descriptor give its getsockopt branch all three outcomes. No ports, no DNS, no listener. wolfio.c 43/87 -> 47/87.
CheckOcspResponder takes an OcspResponse and four raw hashes, walks bs->single and compares bytes. Nothing is stored, so the response can be a local -- unlike GetOcspEntry, which links what it is given into ocsp->ocspList, a list the library allocates and frees, where a stack fixture crashes. Each vector breaks a chain at a different operand: no key hash offered, name mismatch, key mismatch, and the delegated-responder arm behind the OCSP-signing usage bit, with a fully matching vector as the accepting partner. A real response is self-consistent, so the mismatch cases have no independence pair from outside. ocsp.c 11/47 -> 18/47.
MatchDomainName decides whether a presented certificate name matches the host being connected to. It is pure -- two strings, two lengths, a flags word, no ssl, no allocation -- but nearly uncovered, because callers reach it only after a completed verification: the name comes from a parsed SAN or CN and the host from local configuration, so an empty pattern, a bare star, a wildcard that is not leftmost, a wildcard with no dot after it, or a zero length on one side only never arrive. Those are the inputs an attacker picks. 38 vectors over both wildcard-policy flag settings, each row flipping one named operand with an accepting partner differing in a single field. internal.c 573/1722 -> 587/1722.
SanityCheckMsgReceived refuses a handshake message that arrives on the wrong side, arrives twice, or arrives out of order. Every condition in it is a rejection, so a conforming peer takes all of them false and no number of handshakes produces an independence pair; the vectors have to be malformed on purpose. The fixture is a zeroed WOLFSSL with its ctx pointed at a client CTX -- the function reads only options, msgsReceived and specs, and dereferences nothing else unguarded except SSL_CM(ssl)->ocspMustStaple in the ServerHelloDone arm. No certificate, no transport, no peer. Sweeping from all-clear and all-set alone measured 15 conditions: both saturated ends are refused by the prerequisite or the duplicate check before the out-of-order chain is ever evaluated. Each arm therefore also gets the state in which its message is accepted, and one flipped bit from there puts exactly one chain operand true with the rest false. That took it to 47. 3552 calls. internal.c 587/1722 -> 630/1722.
GetRecordHeader is the first code to look at bytes off the wire, before any key or MAC. Its decisions are the version-mismatch tolerance rules -- which peer, in which handshake state, may send which version -- plus the length and record-type checks. From tests/api the header always comes from wolfSSL's own record writer, so pvMajor and pvMinor equal ssl->version on every call and the whole mismatch block is dead: downgrade, connectState, acceptState and the alert-before-negotiation carve-out are evaluated only for a header the local writer never produces. The fixture is five bytes and a struct. inputBuffer.buffer is a pointer, so it points at a local array; the function writes only through its out-parameters and stores nothing. Rows are one flip from a well-formed TLS 1.2 record, with the baseline re-run between rows so the DTLS replay window does not carry. 61 calls. internal.c 630/1722 -> 642/1722.
Four batches, measured together. crl.c 18->29. CompareCRLnumber over hex strings a parser would never emit (non-hex, empty, a number that went backwards); FindRevokedSerial with a same-length different-serial vector; BufferStoreCRL's five-operand guard, driven by hand-linking a CRL_Entry that is missing each field in turn and unlinking it before FreeCRL walks the list; LoadCRL over a real directory and over NULL arguments. wolfio.c 47->53. wolfIO_DecodeUrl against the malformed URLs an attacker supplies and a certificate never carries: unterminated IPv6 bracket, CR/LF smuggled into the host, a host and a port at the length cap, a colon with no digits, a port past the 16-bit ceiling. MAX_URL_ITEM_SIZE is private to wolfio.c, so the API test has to mirror it and hope; the white-box does not. ocsp.c 18->21. A CbOCSPIO mock is a complete responder for this function -- it can return a negative error, a zero-length body, or a positive length with a NULL buffer on demand, which no real responder can be made to do. keys.c 26->33. The file has no group and no caller outside internal.c and tls13.c, and everything in it runs from values a handshake has already fixed: one cipher suite, one version, enc and dec never NULL. GetCipherSpec swept over every suite-table selector, SetKeys called twice on the same objects so the lazy-allocation guards get both halves, SetKeysSide over the four dtls/1.3 combinations.
The guards run before any parser, so the message body can be zeros; they all call SendAlert on the way out, which is the only reason this needs more than the zeroed fixture -- a send callback that consumes and discards, or the alert path dereferences a NULL CBIOSend. internal.c 642/1722 -> 644/1722. Small: most of this function's 22 uncovered conditions sit after the dispatch, inside the per-message parsers, not in the guards.
The error paths in internal.c are most of what is left uncovered there, and a conforming pair of endpoints never enters them. Reaching them needs a peer that sends something wrong -- not a transport. test_memio already runs both endpoints through a byte buffer with credentials from certs/, and both sides are ours, so the buffer between them can be edited between rounds. test_tls_wire_mangle flips a bit at a chosen offset of a chosen round, which selects which handshake message gets hit and where: record type, record length, handshake type, handshake length, or inside the body. test_tls_wire_sequence uses the harness's own drop, duplicate, reorder and length-rewrite helpers, which reach the ordering and retransmit logic a byte flip cannot -- a flipped byte still arrives once, in sequence, at the right length. Both assert only that a corrupted handshake fails rather than crashes; the coverage is in the paths it takes on the way out. Both run in the ssl_hs group, which five of the six protocol modules already measure. internal.c 650/1722 -> 690/1722. The DTLS arms of both tests currently contribute nothing -- see the commit that follows.
… reach Same mechanism as the TLS wire mangler, with the offsets named after DTLS framing rather than TLS: a DTLS record header is thirteen bytes, and the handshake header carries a message sequence, fragment offset and fragment length that TLS has no equivalent of. Replay, drop and reorder are included because DTLS is built to tolerate them, so they enter the retransmit pool and the reassembler rather than being refused. Two mistakes are fixed here and worth keeping written down. The endpoints are stepped by hand rather than through test_memio_do_handshake, which runs the client and the server in one round and leaves nothing in flight to corrupt -- the first version passed in 2.3 seconds and measured nothing. And the offsets now reach past the fixed header into the extension block. It still earns zero. Three measurements put dtls.c at exactly 16/56 and dtls13.c at exactly 70/132 every time; an identical number means the code is not entered, not that the vectors are weak. dtls.c's residue is entirely inside SendStatelessReplyDtls13's extension parsing, and a corrupted DTLS record is dropped by the record layer before that parser sees it -- which is the tolerance DTLS exists to provide. Reaching it needs a well-formed record carrying a malformed extension block: a built ClientHello, not a corrupted one. The sweep is kept narrow so it costs seconds until that fixture exists.
…ries test_memio is a byte stream; DTLS is datagrams, and every guardrail in the protocol is about which datagram arrives, in what order, how many times, and carrying which epoch and sequence number. This replaces the transport rather than editing its buffer: each datagram the stack sends is captured whole, its record header parsed, and a policy consulted before it is queued -- deliver, drop, hold for n rounds, duplicate, rewrite a header field, coalesce with its neighbour, truncate. The receiver then reads whole datagrams out of the queue as a UDP socket would. 24 policies, each named for the guardrail it provokes: replay, sequence number past and future the replay window, unknown and zeroed epoch, lost flight, reordering, fragment offset past the message, fragment longer than the message, overlapping fragments, message sequence forward and backward, record length longer and shorter than the datagram, content-type confusion, two records in one datagram, truncation, and body corruption at six depths inside the extension block plus the echoed cookie. No key material is needed: the DTLS record header is not encrypted, and every guardrail above keys off it. A secret callback is wired behind HAVE_SECRET_CALLBACK for cases that later need a protected body; that macro is not in the campaign option list, so it compiles out there. df_run reports whether the handshake actually completed rather than whether the loop ran -- the first version returned success unconditionally, which is how a transport that never connects still passes. This adds no MC/DC on dtls.c, and the export says why: the file is 85% line covered and 20% MC/DC covered in this variant. SendStatelessReplyDtls13 is entered -- 247 executed segments in its range -- so the problem is not reach. The residue needs ClientHellos that are well-formed but semantically specific: one with no supported_versions, one offering PSK modes, one whose key share names a group the server does not have, one echoing a corrupted cookie. Corruption cannot produce those; they have to be built. The builder exists in tests/unit-mcdc/test_internal_clienthello_whitebox.c and wants pointing down this transport.
The named hellos were known-answer cases: each says one specific wrong thing. Testing the parser's limits is a different job, so the factory generates them -- one policy, one mutation id, driven from a loop -- and each mutation breaks exactly one invariant while keeping the datagram a datagram, so the parser reaches the check that invariant belongs to instead of bailing at the door. 23 mutations: extension block one byte long, one byte short, and declared zero; one, eight and forty unknown extensions; a 900-byte extension; a known extension with an empty body; supported_versions, key_share and cookie each duplicated; session id declared 0, 33 and 255; cookie declared 0 and 255; cipher suite list odd-length, zero and huge; compression list zero and huge; legacy_version at both extremes. Plus surgery that removes supported_versions, key_share, psk_key_exchange_modes or pre_shared_key outright, rewrites the named group to one the server has not got, forces PSK_KE-only and PSK_DHE_KE-only, and corrupts the echoed cookie -- each fixing up the extension, handshake, fragment and record lengths so the result is refused on its meaning rather than its framing. Also here because neither is expressible as a payload: a small-MTU pass, so the stack fragments its own ClientHello and the isFirstCHFrag operands are reachable at all; and a Connection ID pass that calls wolfSSL_dtls_cid_use on both endpoints, because CID must be negotiated and nineteen of the conditions left in dtls.c are in CID functions that are never entered otherwise. dtls.c 16/56 -> 18/56. The surgery is what moved it; six earlier measurements with corruption alone returned exactly 16/56 every time.
Nineteen of the conditions left in dtls.c were in the CID functions, and a
great deal of packet machinery was pointed at them first and moved none. They
are not protocol behaviour: they are NULL-and-zero argument guards.
if (ssl == NULL || buf == NULL) DtlsCidGet
if (id == NULL || id->length == 0)
if (ssl == NULL || cid == NULL) DtlsCidGet0
if (info == NULL || info->rx == NULL || !info->rx->length) DtlsCIDCheck
if (ssl == NULL || cid == NULL || size == 0) DtlsCidReplaceTx
if (msg == NULL || cidSz == 0 || msgSz < OPAQUE8_LEN + cidSz)
No handshake passes NULL and no forged datagram can make it; the only way to
pair these operands is to call the functions directly. Three ssl states are
needed because "no CID info", "info but no id" and "an id of length zero" are
distinct operands: no ssl, an ssl with CID compiled but never enabled, and an
ssl with CID enabled but not yet negotiated.
The assertions are weak on purpose -- these vectors establish that the guard
is taken and the process survives, not what each function returns for an
argument it is documented to reject.
dtls.c 18/56 -> 26/56.
The campaign reports defects, it does not carry fixes, so src/dtls.c is back to origin/master and the four NULL-argument calls that segfault cannot run in the suite -- a crash discards the coverage of every test in the variant. They are left in the source behind WOLFSSL_DTLS_CID_NULL_ARGS_GUARDED with the fault of each written next to it, so the gap is visible and re-enabling them is one define once the library guards them. dtls.c stays at 26/56: the coverage came from the guards that are reachable, not from the four that crash.
src/ssl_ech.c measured 0 of 52 conditions -- zero, not "poorly covered" -- even though ECH is compiled in and five ECH tests run in the tls13 group of the same binary. Those tests drive ECH through a handshake using configs the harness makes for them; none calls the public configuration API, and that is where every condition in the file lives: generating a config for a named KEM/KDF/AEAD, importing one from raw bytes or base64, reading one back into a caller's buffer, and the size and argument checks on all of it. The file was invisible to the campaign until this part because ssl_ech.c is #included into ssl.c rather than compiled standalone, so it produced no object file and never appeared in a filtered llvm-cov export. Vectors a handshake cannot produce: a NULL ctx, a buffer one byte too small, a length of zero, base64 that is not base64, base64 that decodes to something that is not a config, a KEM triple no build implements, and a retry-config query on a connection that never negotiated ECH. ssl_ech.c 0/52 -> 19/52.
…them The four calls fenced behind WOLFSSL_DTLS_CID_NULL_ARGS_GUARDED crashed an unpatched library, so they could not run: a segfault discards the coverage of every test in the variant. origin/master now guards all four -- wolfSSL_dtls_cid_use, _is_enabled and _set check ssl, and _set checks the cid buffer after its size == 0 early return, so (NULL, 0) still means empty CID -- and the fence and its explanation are no longer needed. Verified against the rebased tree with the dtls module: builds, runs, and gates clean with the vectors live. dtls.c 26/56, dtls13.c 70/132, unchanged.
The rebase push took CI from 108 failures to 10. Of those, three are ours.
LeakSanitizer flagged two allocations the tests own and discard:
wolfSSL_SESSION_dup() returns a new session object, not a borrowed one, so
calling it for its side effect leaks it -- 2664 bytes from
wolfSSL_NewSession. The duplicate is freed now.
wolfSSL_CertManagerNew_ex(NULL) returns an owned CertManager -- 280 bytes.
It was called bare to exercise the NULL-heap argument; the result is freed
now.
wolfSSL_SNI_GetRequest and wolfSSL_SNI_GetFromBuffer are compiled under
HAVE_SNI && !NO_WOLFSSL_SERVER (src/ssl_api_ext.c): both read what a client
sent, so a client-only build has neither. Guarding on HAVE_SNI alone left them
undefined at link time there.
Verified with -Werror in a client-only build (NO_WOLFSSL_SERVER, SNI and ALPN
on): both touched files compile clean.
The other seven failures are not ours: scripts/ocsp.test needs external DNS
and the runner had none ("Couldn't find www.google.com, skipping", then
"Both OCSP connection to globalsign and google failed"); that script is
upstream and untouched by this branch. The make-check-linux matrix entries
report "aborted (fail-fast)", i.e. cascade from a sibling, not independent
failures.
The largest remaining category in dtls13.c is not NULL guards but ssl->options.side comparisons. A connection has one side for its whole life, so each of those decisions is taken the same way on every call that endpoint makes, and a test owning both endpoints does not help: MC/DC wants both outcomes of the SAME decision in one binary's profile. Setting the side by hand is the only way to pair them. New driver test_dtls13_role_whitebox.c, 348 vectors over a zeroed WOLFSSL with its ctx pointed at a client CTX -- these functions read options, keys and dtls13Rtx and take scalars; none needs a peer or a handshake: Dtls13AcceptFragmented side x type x encryption x ChFrag x dtlsStateful Dtls13CheckEpoch side x type x epoch, over the whole switch Dtls13SaveOrFlushClientHello side x connectState across the range bounds Dtls13SetEpochKeys stored epoch side vs requested side, all nine pairs dtls13.c 70/132 -> 79/132. CRL and OCSP get the null guards their callers cannot reach: StoreCRL(crl == NULL || path == NULL) -- both operands; every in-tree caller validates both before reaching it. FreeOcspEntry(entry == NULL || !entry->ownStatus) -- an entry with a borrowed status list is what the multi-response path builds, and freeing one must be a no-op rather than a double free. CheckOcspRequest's ioCtx selection (ssl && ssl->ocspIOCtx != NULL) -- an ssl with no per-connection IO context, which falls back to the manager's, is produced by no existing test. crl.c 29/48 -> 31/48, ocsp.c 21/47 -> 24/47. Smoke: 78 drivers, 0 failed.
A census of the remaining NULL-shaped conditions splits them by how the NULL actually arises: 282 from an argument the caller passes, 145 from a struct member legitimately NULL in some state, and only 15 from a failed allocation. This batch takes the first kind in public functions -- no fixture needed at all, which makes it the cheapest coverage left. One call per uncovered operand with every other argument valid, then the all-valid partner: a NULL in the first slot pairs only the first operand because the rest short-circuit away. Covered here: the two cipher-list getters (buf/len), check_domain_name and check_ip_address, CTX_GetDevId, get_cipher_suite_from_name, get_curve_name including its per-curve OID arms, load_verify_locations_ex's compound (file == NULL && path == NULL), use_certificate_ASN1, and export_keying_material. Every symbol was checked against BOTH its ssl.h declaration guard and its implementation guard in src/ before being called. A declaration without a compiled implementation is a link error rather than a compile error, and that distinction has cost this branch several CI rounds. ssl.c 12/89 -> 24/89, ssl_load.c 35/155 -> 41/155, ssl_certman.c 47 -> 48.
…regression The rebase cost ssl_sess.c two conditions: 32/120 before, 30/120 after, same denominator, deterministic across two runs with byte-identical GAPS.md. The only upstream change to that file is a one-line fopen swap inside wolfSSL_save_session_cache, whose conditions are all still covered, so the cause is elsewhere -- most likely the +52 lines upstream added to src/ssl.c, which drives these paths. Rather than accept a lower baseline, the two are recovered by covering more: the session object lifecycle guards, which are almost all "session == NULL || something about the session" and which a test that establishes a session reaches with a well-formed object every time. wolfSSL_SESSION_new / _dup / _up_ref / _free are unguarded in both ssl.h and src/ssl_sess.c -- checked before writing, since a declaration without a compiled implementation is a link error, not a compile error. Vectors cover the NULL half of each entry point, a session that exists but was never established, the up_ref / double-free refcount path, set_session with a not-set-up session, and SetServerID's three operands plus its new-session arm. ssl_sess.c 30/120 -> 32/120. Gate passes with no baseline drop.
Two more batches of public-API argument NULLs, the cheapest category left. ssl_api_dtls.c 11/53 -> 19/53. Its guards are ordinary NULL-and-zero pairs, but the accepting half of most needs a DTLS connection, which is why the file sat at 3/53 until one was supplied. Added: dtls_get0_peer's two operands, DTLSv1_get_timeout's two, set_timeout_max including the zero boundary, dtls13_use_quick_timeout with the fast-timeout flag set both ways, the dtls13_pending_work chain driven through each state it reports on (output buffered, key update owed, ack owed) because a connection only reaches those mid-flight between a blocked write and its retry, and SetCookieSecret's "buffer with zero length", which is neither the clear call (NULL, 0) nor a real secret. x509.c 12/37 -> 21/37, reusing the parsed-certificate fixture already in this file. Added: check_host's object and string operands plus the chklen case where the length includes the NUL terminator -- what a caller using strlen() never passes; check_ip_asc's three operands including an unparseable address; and load_certificate_file's NULL name, an empty file, a directory, and an unknown format. Guards were read from src/ssl_api_dtls.c and src/x509.c before writing rather than assumed from ssl.h. dtls13_pending_work is compiled only under WOLFSSL_DTLS13, SetCookieSecret only under WOLFSSL_DTLS && !NO_WOLFSSL_SERVER, several are gated on !WOLFSSL_LEANPSK, and the X509 name checks need !NO_ASN.
…ack cause The sweep only drove a CTX, one connection object and three file loads, so it reached few of the error arms it exists for. It now also exercises the extension setters, the session object lifecycle, the CertManager with its CRL and OCSP sub-objects, and the chain loader -- each of which allocates on paths whose failure branches a working configuration never takes. ssl_certman.c 48/113 -> 50/113, ssl_load.c 41/155 -> 43/155. The WOLFSSL_SMALL_STACK exclusion stays, but its comment no longer says the cause is unknown: DecodeCertInternal indexes RPKdataASN before checking the ret that CALLOC_ASNGETDATA sets, so an allocation failure dereferences NULL while parsing any certificate. A per-index sweep crashes at five indices (7, 30, 51, 68, 90), all at the same instruction, reached through load_verify_locations, use_certificate_file, use_certificate_chain_file and CertManagerVerify. Fixed upstream in PR 11378; the exclusion comes off once that merges and the sweep passes on the small-stack variant.
Error propagation is the largest uncovered category in internal.c, and most of it needs the failing value produced by something upstream. These two functions are the exception: they classify an error handed to them, so the failing value is an argument and every arm is reachable by passing the code that arm names. DoCertFatalAlert maps a verification failure onto the alert the peer is sent. A handshake produces one failure at a time and most of them not at all -- an expired certificate, then a path-length-invalid one, then a revoked one, each needing its own chain -- so the arms are mutually exclusive per run and never pair. The mapping is security-relevant: it decides what a rejected peer learns about why. Swept over every code it discriminates on, two it does not, and both tls1_3 settings, since NO_PEER_CERT branches again on that. ProcessPeerCertCheckKey enforces the per-algorithm minimum key size. The minimums are configuration, fixed for the life of a connection, and the negative sentinel is never set by a working one, so both operands of each guard are constant in any real run. Swept over each key OID the switch names plus one it does not, four minimums including the sentinel, and verifyNone both ways. 75 vectors, no fixture, no fault injection, no certificate chain -- a zeroed WOLFSSL and a DecodedCert filled in by hand. internal.c 762/1732 -> 780/1732.
Three more error-classification clusters, none needing a peer. CsrDoStatusVerifyCb lets an application override the library's OCSP verdict, and the interesting arms are the disagreements: the callback forcing an error on a good status, and clearing one on a bad status. No in-tree test installs a callback that disagrees, so neither arm had been taken. A mock callback returning a chosen value against a chosen incoming result sweeps the matrix, including the invalid positive return. DoCertificateStatus compares the declared status length against the record size; a conforming peer always makes them agree, so the mismatch arms need bytes no real peer sends. Driven with crafted input, no fixture. SendData's opening guards ask whether the connection is resuming from a blocked write, which a test that writes successfully never sets up. The oversized-length guard above them returns before any IO happens. internal.c 780/1732 -> 784/1732.
…isions ProcessPeerCertLeafRevocation decides what a revocation answer MEANS, and its guards discriminate between specific codes: an explicit assertion that the certificate is revoked, a responder that does not know it, one that could not be reached, a certificate naming no responder, a lookup still in flight, and the CRL equivalents. The difference between them is the difference between failing a handshake and continuing it. Producing them for real needs four separate responder deployments, one per vector, so none of these arms had been taken. This translation unit already #includes internal.c, so the revocation entry points it calls but does not define are redirected to fakes with a #define ahead of the include -- the idiom mcdc_fault_hash.h already uses for wolfcrypt primitives. CheckCertOCSP_ex, CheckCertCRL, CheckCertCRL_ex and OcspNoUrlPolicy live in ocsp.c and crl.c, so this rewrites only the driver's copy and leaves the library untouched. Each fake returns the code the vector chose, which is the point: the answer is the input under test. The answers are only half of it. The guards below them also read ocspEnabled, crlEnabled, crlCheckAll, tls1_3, totalCerts and whether the decoded certificate has a CA; a first version pinned all six while sweeping only the codes and gained 2 conditions. Sweeping them one at a time from both saturated ends, crossed with the codes, gained 13. 7168 vectors. internal.c 786/1732 -> 799/1732.
GetOcspStatus walks the cached status list for a matching serial and then decides whether the cached answer is still usable. A cache the parser populated always holds self-consistent entries, so "same length, different serial", "cached with no stored response body" and "cached with a date that no longer validates" are states the library only reaches after time passes or a responder misbehaves. Built by hand: it reads entry->status and writes *status but stores nothing, so stack objects are correct and the entry is never linked into ocsp->ocspList. CheckOcspRequest's remaining guards read what the responder gave back, and a real responder cannot be asked for a positive length with a NULL buffer, nor for the two negative sentinels the caller maps onto WANT_READ and HTTP_TIMEOUT. The mock returns whatever the vector chose, across: a body with and without a free hook, a NULL buffer with a positive length, a zero-length reply, a generic error, no transport installed at all, a URL that is present but empty, and no URL. Each row uses a distinct issuer hash so it misses the cache and actually reaches the transport. ocsp.c 24/47 -> 28/47. Not attempted, and worth recording: CheckOcspResponse's newStatus/newSingle/ ocspResponse NULL checks are WOLFSSL_SMALL_STACK allocations and plain stack arrays otherwise, so in the default variant the decision cannot be true at all and in the small-stack variant it needs the allocation injector, which is fenced until PR 11378 lands.
The signature-verify wrappers in internal.c all end in the same two-operand guard, and an ordinary handshake pairs neither operand. A good signature gives (F,F). A bad signature also gives (F,F) at that line, because a bad signature is not an error: the wc_*_verify_* call returns 0 and reports the verdict in eccVerifyRes. The first operand is true only when the maths itself breaks. WOLF_CRYPTO_CB is the supported way to be the thing that breaks. mcdc_fault_ cryptocb.h registers a device that answers CRYPTOCB_UNAVAILABLE to everything except the one operation a vector selects, so each wrapper can be driven three ways: the device refuses, the device succeeds with the verdict "no", the device succeeds with the verdict "yes". Dispatch happens after the argument checks but before any key material is touched, so the vectors need a key object carrying a devId and nothing else -- no certificate, no peer, no valid public point. wc_ed448_verify_msg zeroes *res before dispatching and the other two do not, so the device sets the verdict rather than the caller. VerifyRsaSign's recovered-plaintext check uses the #define idiom instead, and the reason is worth keeping. RsaPublicDecrypt routes every operation except verify through the callback, and the path that does dispatch feeds its output back through PKCS#1 unpadding, so a device returning anything but a correctly padded block makes ret negative and the guard is never reached. The middle operand also defends against a positive length arriving with a NULL buffer, which no implementation produces. Redirecting the call reaches all three. internal.c 799/1748 -> 810/1748. One vector failed and is kept for what it shows. EccMakeKey's (ret == 0 && key->dp) looked like the same shape -- let a device claim success without generating a key and dp should still be NULL. It is not: _ecc_make_key_ex calls wc_ecc_set_curve before it consults the device, and set_curve either fails, making ret non-zero, or assigns dp. (T,F) does not exist, so the operand is now an exclusion with the argument written out rather than an open condition.
The ClientHello builder wrote the renegotiation_info extension type as a conditional whose two branches were the same constant, which is a constant expression some -Wextra builds reject and which hid what 0xFF01 is. It is TLSX_RENEGOTIATION_INFO; use that. The CRL and OCSP skip stubs are guarded on their feature macro plus certs plus not-WOLFCRYPT_ONLY. Their printed messages already said so; the #else comments still named only the feature macro, so a reader chasing a skip saw the wrong reason. No coverage change: tls_core 810/1748 and revocation 28/47 + 31/48 both re-measured identical, gates green.
Five configurations failed, four of them at build time, each the same mistake in a different place: a guard that describes what the test needs rather than what the build actually provides. Also registers the four tests/unit-mcdc files that were missing from EXTRA_DIST. Verified by building all five configurations locally: all pass. Campaign unaffected -- dtls, ssl_api and tls_core re-measured identical, gates green, white-box smoke 82 passed 0 failed.
The upstream hardening commits added defensive checks on deserialized private
key state, and none of them are reachable from a keygen/sign/verify cycle: the
library's own output always satisfies them, so every one of the decisions is
permanently false in an ordinary run. Each is driven here by handing the
function the state a tampered or truncated key would produce.
wc_lms_priv_state_load stack offset past the end of the stack, and an
offset that is in range but not a whole number
of nodes, plus both accepting partners
wc_lms_treehash_update a restored offset that says the data stack is
already full, so the first push has nowhere to
go
wc_xmss_bds_update a NULL height array, and an offset past the
subtree height
wc_xmss_bds_next_idx a retain index below the first retained node,
and one that would run off the end of retain
The retain guard needs the merge loop to reach a height at or above
sub_h - bds_k, so bds_k = 3 against sub_h = 4 puts that at height 1 and the
caller-supplied height/offset pair steers the loop there in two iterations.
The three indices then select each operand: 4 gives (i >> h) = 2, 6 gives a
retain offset inside the buffer, 18 gives one exactly at its end.
wc_lms_impl.c 134/140 -> 137/140, wc_xmss_impl.c 71/79 -> 75/79.
The LMS drivers belong inside WB_GAP_SIGN. Placed outside it first, they
failed to compile under WOLFSSL_WC_LMS_SMALL and WOLFSSL_LMS_VERIFY_ONLY, and
because a white-box that does not build is recorded as a skip, both variants
were dropped whole and took four conditions elsewhere in the file with them --
the file went down by one overall while the intended three were covered. Both
drivers are now syntax-checked against every combination of the small and
verify-only macros.
Two conditions are left and neither is this shape: wc_lms_impl.c:2454's ret
operand needs a hash failure earlier in the same loop iteration, which belongs
to the hash-fault driver, and wc_xmss_impl.c:3077 needs a tree-hash instance
that is in use while the stack offset is zero.
while (signers && ret == NULL) is false in its second operand only when a match was just assigned and the bucket still holds an entry after it, so the loop condition is evaluated once more with ret set. If the match is the only entry in its row, or the last one, signers goes NULL and the first operand ends the loop instead. That makes the operand a property of CA-table occupancy rather than of any test: it needs two CAs hashing to the same row and a lookup for the one that is not at the tail. Which certificates a run loads, and in what order they were added, decide whether that ever happens -- which is why this one condition was covered on one host and not on another from the same tree, the same tests and byte-identical certificates. The sweep measured 49/113 two nights running where the development host measured 50/113, and the difference was this line and nothing else. The bucket is now built rather than hoped for. GetCAByName reads only subjectNameHash and next and takes cm->caLock, so two zeroed Signers linked head to tail are a complete fixture; they are on the stack, so the row is detached before the CertManager is freed. Four vectors: the head of a two-entry row, its tail, an absent hash, and a NULL manager. ssl_certman.c is #included into ssl.c and refuses to compile alone, so the white-box includes src/ssl.c.
Three more configurations failed to link, all the same shape as the last
round: a guard that names what the test needs rather than what the build
provides.
NO_TLS builds (certgen-no-tls, no-tls-cryptocb-aesgcm-setkey-free)
wolfSSLv23_client_method and wolfSSLv23_server_method are implemented
under !NO_TLS && !NO_WOLFSSL_{CLIENT,SERVER}, and wolfSSL_UseSNI,
wolfSSL_CTX_UseSNI, wolfSSL_SNI_Get*, wolfSSL_UseSupportedCurve and
wolfSSL_CTX_UseSupportedCurve all sit under !NO_TLS in ssl_api_ext.c on
top of their own feature macro. Eleven blocks were missing !NO_TLS.
dtls13-client-minimal (WOLFSSL_NO_TLS12)
wolfDTLSv1_2_{client,server}_method are implemented under
!WOLFSSL_NO_TLS12: DTLS 1.2 is built on the TLS 1.2 code. Three blocks
wanted that, including the CID argument-guard test.
Found the first pass by line number and missed a second block carrying the
identical guard text, so this was checked mechanically instead: for every call
site of each of these symbols, walk the enclosing #if chain and assert it
carries the macros the implementation requires. That found the leftovers and
now reports zero for this branch. Verified by building all seven affected
configurations locally -- the three new ones and the four fixed last round, so
neither set regressed the other.
Campaign unaffected: ssl_api, dtls, revocation and tls_core re-measured, gates
green, white-box smoke 82 passed 0 failed.
Rebasing onto master put upstream's new verify-mode tests in the same place as this branch's, and git aligned the two on the shared create-ctx / free-ctx boilerplate, which splits both function bodies. Resolving those hunks by copying whole functions across is right, but it moved the mock without the two file-scope counters above it, so g_crlIoCalls and g_crlIoResult became undeclared. They are back, immediately above the mock that uses them. Nothing else was lost: every file-scope static that existed before the rebase and is still referenced is still defined.
test_ssl_certman_whitebox includes src/ssl.c, and against the previous smoke build that failed to link: the harness compiles with fixed flags, and whether XFDOPEN is defined decides whether wc_fopen_owner_only is a function or a macro. It builds and passes against the post-rebase --enable-all --enable-static tree, so it is in the expected list now and a break in it will be caught locally rather than only by a full sweep.
a299c5b to
03206d6
Compare
The rebase resolution copied test_crl_io_mock across without the #if that had surrounded it, leaving it defined at file scope while its only caller keeps the guard inside its own body. Any build where that body is compiled out -- a default build has neither HAVE_CRL nor HAVE_CRL_IO -- then has a static function nothing references, which -Werror=unused-function rejects. The guard is back, and it now carries the !defined(NO_TLS) the caller's body gained after the original was written, so the two conditions are identical rather than merely similar. Reproduced before fixing: the pre-fix file fails to compile in a default build with exactly "'test_crl_io_mock' defined but not used", and builds clean after. Checked the other six functions the rebase moved in this file the same way -- comparing each one's enclosing guard chain against its pre-rebase version -- and this was the only one that lost anything. Verified across default, --enable-crl, --enable-all and a no-TLS build, all with -Werror=unused-function and -Werror=unused-variable.
The sweep was excluded from WOLFSSL_SMALL_STACK because DecodeCertInternal indexed RPKdataASN before checking the ret that CALLOC_ASNGETDATA sets, so failing an allocation dereferenced NULL while parsing any certificate. That was reported and fixed upstream in PR 11378, which the last rebase brought in, so the exclusion is gone. Verified rather than assumed: built --enable-all --enable-smallstack with WOLFSSL_SMALL_STACK actually defined in options.h, ran the ssl_cert group, and the sweep passes with no errors and no crash. A crash would have discarded the whole variant, which is what the exclusion was protecting against. The small-stack variant now contributes what it always should have: internal.c 810/1748 -> 825/1754 ssl_load.c 43/182 -> 48/182 ssl_certman.c 50/113 -> 53/113 keys.c 33/40 -> 34/40
tests/api is one binary compiled in every CI configuration, so a test calling an API the build did not compile is not a test failure -- it is a link error that takes the whole binary down. It is also invisible to header inspection, because wolfSSL declares plenty of API unconditionally and implements it under a narrower condition. That combination broke CI four separate times on this branch, each time found by CI rather than locally, and each time the fix was the same: name what the build provides, not what the test needs. check-api-guards.py walks the enclosing #if chain of every call site and requires the macros the IMPLEMENTATION carries. It is a whitelist rather than a parse of ssl.h on purpose: the mapping from symbol to implementation guard cannot be derived from the declaration, which is the whole problem. Two things make it usable rather than noisy: It only looks at call sites this branch changed. Run over everything it reports 28 long-standing sites that are fine in practice because the configurations that would break them are not built; auditing those is a different job, and --all still does it. It knows which macros imply TLS. A block under WOLFSSL_TLS13 or HAVE_SNI cannot also need !defined(NO_TLS) spelled out, and comments and string literals are blanked before matching, since these files discuss the very API names being searched for. It refuses to run against a ref it cannot resolve rather than reporting success, because a shallow checkout would otherwise make every diff empty and the check would pass without looking at anything. The workflow checks out with fetch-depth: 0 for that reason, and runs the check before the smoke build -- it needs no build and costs a second. Verified both directions: clean on this branch, and it reports the exact site when !defined(NO_TLS) is removed from a guard that needs it.
Both session caches walk their row backwards from the most recently used
entry, and both start the walk with
idx = row->nextIdx - 1;
if (idx < 0 || idx >= SESSIONS_PER_ROW)
nextIdx is the ring's insertion point, so idx lands in [-1, PER_ROW-1] and the
first operand is true exactly when nextIdx is 0 -- an untouched row, or one
that has just wrapped. Whether any test produced that state was decided by
what an earlier test in the same binary had left in the cache, not by anything
the test itself did. That is not hypothetical: ssl_sess.c measured 32/120 on
2026-09-05 and 33/120 on 2026-09-06 from identical wolfssl and campaign
commits on the same host, and 33 is what this now reaches every run.
The white-box empties the rows itself and looks up against them, then repeats
each lookup with nextIdx in the middle of the ring, because both halves have
to run in one binary or the operand has no independence pair -- a first
attempt drove only the true side and scored zero.
Two details cost a measurement each and are worth writing down: ClientCache is
CLIENT_SESSION_ROWS long while SessionCache is SESSION_ROWS, so zeroing the
first with the second's bound leaves the row the id hashes to untouched; and
wolfSSL_GetSessionClient returns before the ring walk when the context has the
cache switched off, which makes every vector a silent no-op.
ssl_sess.c 32/120 -> 33/120. The second operand of each guard is excluded
rather than chased: nextIdx is bounded by the ring at every write, so
idx >= PER_ROW cannot occur and has no pair in any configuration.
…failed Expect Two PRB findings, unrelated to each other. tests/unit-mcdc/test_ssl_certman_whitebox.c and test_ssl_sess_whitebox.c were never added to EXTRA_DIST, so make dist left them out. Confirmed by building a tarball before and after: the two were the only files this branch adds that were missing, and both are in it now. Every other file the branch adds was already listed. test_wolfSSL_ocsp_stapling_accessors wrote cssl->ocspProducedDateFormat directly. Expect* records a failure and carries on rather than returning, so on the path where wolfSSL_new() failed that is a dereference of NULL, which is what the static analyser reported at the ExpectNotNull above it. The three direct field writes are guarded; the calls that merely pass cssl are safe either way, because the API checks it. Checked the rest of the file for the same shape rather than fixing only the reported line: of the raw dereferences of Expect-obtained pointers, the DTLS ones were already inside if (dssl != NULL) blocks and the remainder are inside Expect* macros, which short-circuit once a previous one has failed. These three were the only unguarded ones.
…eep opt-in Two Jenkins findings.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11355
Scan targets checked: wolfssl-bugs
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| g_input[0] = r->type; | ||
| g_input[1] = r->pvMajor; | ||
| g_input[2] = r->pvMinor; | ||
| g_input[3] = (byte)(r->len >> 8); |
There was a problem hiding this comment.
DTLS records use the TLS header layout · Logic errors
wb_record() writes len at TLS offsets 3–4 even when r->dtls is set. DTLS parsing treats those bytes as the epoch and reads length from zeroed offsets 11–12, so every DTLS row follows unintended epoch/zero-length paths.
Suggested fix: Encode DTLS epoch at 3–4, sequence at 5–10, and len at 11–12 when r->dtls is set; retain offsets 3–4 for TLS.
Basis: RFC 6347 §4.1 places the DTLS epoch and 48-bit sequence number before the record length.
| (void)wolfSSL_SetCRL_IOCb(ssl, test_crl_io_mock); | ||
| /* loading a certificate whose CRL is missing is what drives the | ||
| * callback; the load itself is allowed to fail */ | ||
| (void)wolfSSL_CTX_load_verify_locations(ctx, caCertFile, NULL); |
There was a problem hiding this comment.
CRL I/O callback test never invokes the callback · Weak or missing assertions
Unlike known #8888 (setter before CRL creation), this CRL exists, but load_verify_locations() never enters CheckCertCRL(), so the callback count stays zero; -1 also misses the WOLFSSL_CBIO_ERR_WANT_READ result branch.
Suggested fix: Verify a certificate with CRL enabled, include WOLFSSL_CBIO_ERR_WANT_READ in results, and assert one callback invocation per case.
Basis: wolfSSL's CheckCertCRLCm() invokes crlIOCb only during certificate CRL checking and handles WOLFSSL_CBIO_ERR_WANT_READ specially.
| (void)wolfSSL_X509_check_host(NULL, "example.com", 11, 0, NULL); | ||
| #endif | ||
| wSz = (word32)sizeof(buf); | ||
| (void)wolfSSL_X509_get_pubkey_buffer(x509, buf, (int*)&wSz); |
There was a problem hiding this comment.
X.509 size output uses an incompatible pointer type · Incorrect sizeof/type usage
wSz is word32, but the API writes through the cast int *; with WC_16BIT_CPU, this accesses only half the object through an incompatible type. Unlike known #1973 in ASN parsing, this is the new X.509 test call site.
Suggested fix: Declare wSz as int and pass &wSz without a pointer cast.
Basis: ISO/IEC 9899:2018 §6.5 ¶7 restricts object access to compatible, qualified, corresponding signed/unsigned, aggregate, or character types.
| } | ||
| /* Hold it open both ways so the library's fopen() returns immediately | ||
| * and there is something to read. */ | ||
| fd = open(fifo, O_RDWR | O_NONBLOCK); |
There was a problem hiding this comment.
FIFO fixture relies on undefined O_RDWR behavior · API contract violations
open(fifo, O_RDWR | O_NONBLOCK) relies on behavior that POSIX leaves undefined, although the test is enabled for every __unix__. On non-Linux Unix targets, an open failure silently skips every FIFO check.
Suggested fix: Create the FIFO in a mkdtemp() directory and hold separate nonblocking read-only and write-only descriptors, failing if either open fails.
Basis: POSIX.1-2017 open() defines nonblocking FIFO behavior for O_RDONLY and O_WRONLY but leaves opening a FIFO with O_RDWR undefined.
| * client's second flight and the finished exchange. */ | ||
| for (i = 0; i < sizeof(pols) / sizeof(pols[0]); i++) | ||
| for (t = 0; t < 4; t++) | ||
| (void)df_run(mc, ms, pols[i], t); |
There was a problem hiding this comment.
DTLS forgery tests do not require a mutation · Weak or missing assertions
df_sweep() discards every forged run, and df_run_ex() frees the context without checking its mutation counters. The test passes when a policy never matches a packet, so all advertised forgery cases can silently exercise clean traffic.
Suggested fix: Return whether the selected policy changed a packet and assert that at least one targeted run per policy applies it.
Basis: The wolfSSL API-test convention in tests/unit.h requires an evaluated Expect* predicate for a failed condition to affect EXPECT_RESULT().
| for (o = 0; o < (int)(sizeof(offsets) / sizeof(offsets[0])); o++) { | ||
| for (m = 0; m < (int)(sizeof(masks) / sizeof(masks[0])); m++) { | ||
| for (dir = 0; dir < 2; dir++) { | ||
| (void)test_wire_mangle_one(wolfTLSv1_2_client_method, |
There was a problem hiding this comment.
TLS wire mangler discards failed mutations · Weak or missing assertions
test_tls_wire_mangle() discards the helper's explicit nonzero status when no byte exists at the requested round and offset. The sweep therefore passes when individual corrupted cases apply no corruption; only the final clean control is asserted.
Suggested fix: Assert each requested round/offset mutation succeeds, or remove combinations that cannot reach an in-flight byte.
Basis: The wolfSSL API-test convention in tests/unit.h requires an evaluated Expect* predicate for a failed condition to affect EXPECT_RESULT().
No description provided.