#161: make three VICE suites measure what they claim (mutation-proven) - #176
Merged
Conversation
All three shared one shape: the rig never established a precondition the routine under test relies on, and whether that precondition is needed is BUILD-CONFIGURATION dependent — so the defect is invisible on the backend each suite was written against. Each fix is red/green-proven by mutation against BACKEND=uci (sha256 7c79a4e0…), not argued from reading the source. 1. tools/test_finished_verify.py — 16 negatives were vacuous under uci tls_verify_finished reads the received verify_data through (tls_hs_ptr)+4 and resets that pointer itself only .ifndef TLS_STREAM_DEFRAME. Under uci the deframer owns the pointer, so the reset is compiled out and this rig — which has no deframer in the loop — compared 32 bytes at a stale address. Every negative "rejected" for a reason unrelated to its vector. Fix: the carry stub now `jsr tls_hs_ptr_reset` before the routine — the repo's own entry point, so there is no second copy of the base address in Python. 2. src/tls_keyschedule.s + src/tls_cert.s docstrings Both told direct callers the pointer "is reset at entry" and named test_finished_verify.py as the beneficiary. False under uci since W1. Comment-only: the PRG sha256 is unchanged. 3. tools/test_tls_deframe.py — Certificate cases decided by an empty hostname x509_verify_hostname returns C=1 immediately when tls_hostname_len is 0, and it is the tail call of x509_extract_pubkey, so its carry IS the Certificate handler's result. http_get populates tls_hostname; this rig never calls it. The acceptance cases failed for that reason, and the REJECTION cases passed without their own guards ever running. Fix: mint the fixture cert with a SAN, and write tls_hostname from the same constant so cert identity and requested identity cannot drift. 4. tools/test_tls_deframer.py — same cause, opposite symptom: GREEN Confirmed directly (issue #161 item 4 asked for confirmation before fixing). Every Certificate in every scenario was being rejected, and the suite scored 11/12 anyway: its oracle is the extracted pubkey and the folded transcript, both already correct when the name check fires, and `carries` was collected and discarded as "diagnostic only". Fix: populate tls_hostname from the fixture cert's own SAN; give the wiki-sized leaf a SAN too (it is used as a LEAF, and a SAN-less leaf is rejected outright); and assert the flight outcome — an accept scenario must show no C=1, a reject scenario must show one. Without that last part the suite still cannot see an abort. tools/https_e2e/chain_certs.py: build_padded_intermediate grows an optional `sans=`. Default None, so ensure_chain_certs' padding intermediates are byte-unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #161.
Three VICE suites passed or failed for reasons unrelated to what they claim to
measure. All three share one shape: the rig never establishes a precondition
the routine under test relies on, and whether that precondition is needed is
build-configuration dependent — so the defect is invisible on the backend the
suite was written against.
Per the issue thread's own prescription, nothing here is argued from reading
code. Each claim was proved by mutating the code under test and showing the
suite did not notice, then re-running the same mutation after the fix and
showing it goes red.
Method
Everything below is
BACKEND=uci,C64_SKIP_BUILD=1, VICE viac64-test-harnesswith-reu -reusize 512(
tools/_vice_helpers.py::default_vice_config).Unmutated PRG sha256
7c79a4e00a19685de7540970247c759a5e1cc7e13a88b12bf8b8234fcc013c14;every mutant was rebuilt and its PRG hash confirmed different before running,
and confirmed back to
7c79a4e0…after restoring.Per-suite table
tools/test_finished_verify.pytls_verify_finishedreturn C=1 — the forged-Finished abort, which had no test before this suite existedtls_verify_finishedreads received verify_data through(tls_hs_ptr)+4and resets that pointer itself only.ifndef TLS_STREAM_DEFRAME. Under uci the deframer owns it, the reset is compiled out, and this rig has no deframer in the loop — so the compare ran against a stale address. Every negative "rejected" for a reason unrelated to its vectorcpx #32→cpx #1in the@vf_cmploop (compare only byte 0 — exactly the early-exit/off-by-one classtruncatedandrotatedexist to catch)flip_last_byteandtruncatedfail in both vector sets. Mutation killed. Restored → 18/18tools/test_tls_deframe.pycert_buf, chain discarded, pubkey extracted; plus the Certificate reject pathsx509_verify_hostnamereturns C=1 at its first instruction whentls_hostname_len == 0, and it is the tail call ofx509_extract_pubkey, so its carry is the Certificate handler's result.http_getpopulatestls_hostname; this rig never calls it. Acceptance cases failed for that reason; rejection cases passed without their own guards ever runningcertificate_request_contextmust-be-zero check insrc/tls_deframe.s(@ctx) — the guard case 14 exists to testPASS non-zero request context rejected (streamed)— green with the guard deleted from the source. Mutation survivesFAIL non-zero request context rejected (streamed) events=['msg']. Mutation killed. Restored → 45/45tools/test_tls_deframer.pycarrieswas collected and thrown away as "diagnostic only"@msg_endentry-count guard insrc/tls_deframe.s(beq @cert_fmt_err→bne), so a complete streamed Certificate errors. Chosen because it is 100% downstream of the leaf — never reached while the leaf is rejectedwikipedia_shaped_flight). Mutation survivesflight ABORTED … the client refused this Certificate. Mutation killed. Restored → 12/12Note the third row's post-fix
12/12:deframer/wikipedia_shaped_flighthad beenred before any of this work, and it was red for the empty-hostname reason.
Verbatim mutation-kill evidence
Suite 1 —
test_finished_verify.py, mutation M-CPRG
6c929c504f8f16082769bea9deed4512ea05ee34f4c5100ec6fd0f42a353d5c0.Pre-fix, mutated — indistinguishable from the unmutated baseline:
Post-fix, same mutation — killed:
Mutation restored (PRG back to
7c79a4e0…):Suite 2 —
test_tls_deframe.py, mutation M-BPRG
e99a92782ac78640187e446ac923eb42a3a7008e331b9d95d76ea2a4bb4698b5.Pre-fix, mutated —
diffagainst the unmutated baseline transcript showsonly the two freshly-minted-cert hash values; the verdict set is identical:
and the case whose guard was deleted:
Post-fix, same mutation — killed:
Mutation restored:
Suite 3 —
test_tls_deframer.py, mutation M-DPRG
b8698374488961efa12a915aac9fb612eda2ce580e40bea217e44498751a83a0.Pre-fix, mutated — identical to the unmutated baseline:
Post-fix, same mutation — killed:
Mutation restored:
Profile split: the ip65 control (added after review)
Suite 1's defect is a pure profile split, and both halves of that are now
measured rather than argued. ip65 built in this worktree; blob
ip65-build/ip65-c64.binverifiedcf1a5ff7809af4e4655e385b378b936054f41046ff2b7604828af3240c2d90dd,6,951 B — the worktree reads its own blob (#116).
7c79a4e0…6c929c50…6c929c50…7c79a4e0…8c46e27b…d780f719…The fifth row is the control: on ip65 the pre-fix rig was already
discriminating, because
tls_verify_finishedcallstls_hs_ptr_resetitselfthere. So the suite was sound on the profile
tools/run_all_tests.pybuildsand vacuous on the profile that ships and reaches real servers. One cause,
two symptoms — the positives failing and the negatives passing are the same
stale pointer seen from opposite sides — and the honest-accept control did not
hold on the backend that matters.
The last row also measures the "no-op on ip65" claim instead of asserting it:
the added
jsr tls_hs_ptr_resetleaves ip65 at 18/18.What changed
tools/test_finished_verify.py— the carry stub is nowjsr tls_hs_ptr_reset; jsr tls_verify_finished; …(13 B; the carry latchmoved $034C → $0350 to stay clear of it). Calling the repo's own entry point
rather than poking $3E/$3F from Python keeps one copy of the base address.
src/tls_keyschedule.s,src/tls_cert.s— the docstrings that promiseddirect callers the pointer "is reset at entry" (and named
test_finished_verify.pyas the beneficiary) now say who actually sets itunder which build. This is issue item 2, and it is what hid the failure.
Comment-only: PRG sha256 is unchanged at
7c79a4e0….tools/test_tls_deframe.py—generate_p256_cert()mints with a SAN;install_hostname()writestls_hostname/tls_hostname_len. Both take thename from one module constant
CERT_HOST(deframe.foo.invalid, reservedTLD per
tools/test_reserved_test_host.py), so cert identity and requestedidentity cannot drift apart into a
DF_ERR_CERT_FMTnobody can attribute.tools/test_tls_deframer.py—load_cert_fixture()reads the SAN dNSNamesback out of the emitted DER and they become
tls_hostname(derived, neverspelled);
build_wiki_leaf()mints its leaf with the same SAN; andrun_scenario()now judges the flight outcome, not just its side effects.tools/https_e2e/chain_certs.py—build_padded_intermediate(…, sans=None).Default is None, so
ensure_chain_certs' padding intermediates arebyte-unchanged (verified: still exactly one extension,
1.3.6.1.4.1.55555.1).Is production affected? No — and here is the trace, not the assertion
Raised in review, because "we changed the test until it passed" is what a
comment-only source diff looks like without this section.
1. In a real uci handshake, is
tls_hs_ptralways set before the verify?Yes, and the reachable set is small enough to enumerate.
tls_verify_finishedhas exactly two call sites in the tree:src/tls13.s:724(@enc_finished, insidetls_recv_encrypted).elsearm of.ifdef TLS_STREAM_DEFRAME(src/tls13.s:605.else … :752.endif), and that arm resets the pointer itselfsrc/tls_deframe.s:468(df_dispatch@disp_fin)So on a uci build the deframer's
df_dispatchis the only production caller.df_dispatchis not exported (src/tls_deframe.s:76-91exportstls_deframe_{init,new_record,pump},df_*state andtls_hs_allowed— notthe dispatcher), and it has exactly two entries,
jmp df_dispatchat lines 334and 425. Both write
tls_hs_ptrin the same basic block, a few instructionsearlier, with no branch in between:
src/tls_deframe.s:311-334:jsr df_rec_cursorgivesthe body start,
sbc #4backs up to the message header, stored totls_hs_ptr/+1at 316-319; then transcript fold, cursor advance,jmp df_dispatch. Straight-line.src/tls_deframe.s:418-425: on message completion,tls_hs_ptr = df_carry_bufat 420-424, thenjmp df_dispatch. Straight-line.The third route,
df_stream_begin, handlesTLS_HS_CERTIFICATEonly(
:338-341) and never reachesdf_dispatchat all (:759— it consumes themessage incrementally). A Finished cannot take it.
There is no route to the verify that does not write the pointer immediately
beforehand. It is never stale in production.
2. If it were stale, which way does it fail?
Fail-closed. Confirmed — your reading is right. Two independent reasons:
The expected value is computed from
tls_s_hs_secretandtls_transcript,neither of which
tls_hs_ptrinfluences. A stale pointer only changes which32 bytes are compared against it. For acceptance the attacker must place
the correct
verify_dataat the stale address — but computing it requiresthe server handshake traffic secret, and anyone holding that can simply send
a correct Finished and be authenticated legitimately. No advantage.
The one theoretical accept-anything shape is a pointer aimed at the expected
value itself — if
(tls_hs_ptr)+4landed ontls_verify_data, the routinewould compare the buffer with itself and always match. It is out of range:
tls_verify_data=$B309, while the pointer's entire reachable set istls_rec_buf=$A000..$A223(548 B,src/data.s:276) for the in-placeroute and the fixed
df_carry_buf=$3B89for the carry route.$B309is4,326 B above the top of the in-place window.
Flagging that second one honestly: it is closed by layout, and the issue
thread's own canonical case is a guard that held by layout rather than logic.
It is moot only because (1) holds — the pointer is never stale — but if the
guard in (1) were ever weakened, this is the property that would need a real
pin (a link-time
.assertthat the two ranges are disjoint). Not added here:out of scope for Three VICE suites pass or fail for reasons unrelated to what they measure (test_finished_verify's 16 negatives are vacuous on UCI) #161, and it would be pinning a counterfactual.
So: not a security fix. No path accepts a forged Finished.
3. Should the
.ifndefguard get a functional fix?No — the guard is correct, and removing it is a regression. Measured, not
argued. Mutation M-E: delete the
.ifndef TLS_STREAM_DEFRAMEwrapper sothe reset runs unconditionally, uci PRG
e0dffcaa885502d01444ac0d6721fa40917b4b35e8e0fb432b26e9fb69bd0b6a:err 3isDF_ERR_DISPATCH— a valid server Finished rejected, handshakeaborted. Both broken cases are real wire shapes the deframer exists for: a
Finished that arrived split across records (pointer =
df_carry_buf), and aFinished sharing a record with EncryptedExtensions (pointer =
tls_rec_buf + offset). Resetting totls_rec_bufin either case reads thewrong 32 bytes. Restored → 45/45, PRG back to
7c79a4e0….Note the third line of that output:
split Finished REJECTED on bad verify_datastill passes under M-E — it rejects for the wrong reason, the sameshape this PR is about. Even the regression is partly invisible to the suite.
The defect was therefore in the documentation and the harness, which is
what this PR changes. Leaving
src/tls_keyschedule.sfunctionally unchanged(PRG sha256 identical) is the correct call, and the
.ifndefshould stay.On the issue thread's "pin rather than restate" rule
Where a fact had to appear twice, it is now derived rather than copied:
the rig calls
tls_hs_ptr_resetinstead of hardcoding$3E; suite 3 reads thehostname out of the certificate it is about to send; suite 2 uses one constant
for both the SAN and
tls_hostname. The thread's second qualification appliesto this PR as much as to any other: mutation testing is a floor. M-C, M-B
and M-D are three mutants somebody thought of. They do not prove these suites
now catch the fourth.
Not done / caveats
Suites 2 and 3 are uci-only by construction (
TLS_STREAM_DEFRAME), so noip65 result exists for them and none is claimed.
tools/test_x509.pydrivesx509_parse_certover DMA and does not settls_hostname. It builds ip65 by default, where the name check is compiledout, so it is not affected today — but it is the shape the issue warns about,
and it is not covered here.
tools/test_tls_p384_negotiation.pyandtools/test_hs_sequence.pywere checked and already establish theirpreconditions (
tls_hs_ptr_resetandtls_hostnamerespectively).match
tls_hostname), so it cannot detect anx509_verify_hostnamethataccepts everything.
tools/test_x509_name.pyowns that; keeping this PR tothe reported defect.
pytestat repo root: 50 passed.tools/test_reserved_test_host.py: 9/9.