Skip to content

fix(tls): cap weak-cipher evidence vec at 64 with elision marker#195

Merged
Zious11 merged 2 commits into
developfrom
fix/weak-cipher-cap
Jun 8, 2026
Merged

fix(tls): cap weak-cipher evidence vec at 64 with elision marker#195
Zious11 merged 2 commits into
developfrom
fix/weak-cipher-cap

Conversation

@Zious11

@Zious11 Zious11 commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Summary

handle_client_hello in src/analyzer/tls.rs built the weak-cipher evidence Vec without any upper bound. A crafted ClientHello advertising a large number of weak cipher suites caused a transient String allocation proportional to the count (up to several hundred KB). This PR caps the evidence Vec at 64 entries and appends a (+N more) elision marker when the cap is exceeded.

Reclassification (explicit): Research confirmed this is LOW-SEVERITY HARDENING, NOT a DoS or CWE-405 issue. CWE-405 requires asymmetric amplification; this allocation is strictly linear and is already bounded by MAX_RECORD_PAYLOAD = 18,432 bytes (~9,216 cipher IDs maximum). No security label has been applied.

Problem

// Before: unbounded
let weak: Vec<String> = ch.ciphers.iter()
    .filter(|&&id| is_weak_cipher(id))
    .map(|&id| cipher_name(id))
    .collect();

With a maximal weak-cipher ClientHello (~9,216 cipher IDs), the transient allocation was in the hundreds of KB before the finding was stored.

Fix

const WEAK_CIPHER_EVIDENCE_CAP: usize = 64;
let total_weak = ch.ciphers.iter().filter(|&&id| is_weak_cipher(id)).count();
let mut weak: Vec<String> = ch.ciphers.iter()
    .filter(|&&id| is_weak_cipher(id))
    .take(WEAK_CIPHER_EVIDENCE_CAP)
    .map(|&id| cipher_name(id))
    .collect();
if total_weak > WEAK_CIPHER_EVIDENCE_CAP {
    weak.push(format!("(+{} more)", total_weak - WEAK_CIPHER_EVIDENCE_CAP));
}
  • Two-pass approach: first count() the weak ciphers, then .take(64) during collection — avoids materializing the full Vec.
  • Elision marker: analyst-visible (+N more) entry preserves forensic transparency.
  • Cap value (64): matches existing MAX_MAP_ENTRIES idiom used elsewhere in the codebase.

Architecture Changes

graph TD
    A[ClientHello parser] --> B[handle_client_hello]
    B --> C{weak ciphers present?}
    C -->|yes| D[count total_weak]
    D --> E[take up to 64 entries]
    E --> F{total_weak > 64?}
    F -->|yes| G[append elision marker]
    F -->|no| H[evidence Vec as-is]
    G --> I[push Finding]
    H --> I
    C -->|no| J[skip]
Loading

Spec Traceability

flowchart LR
    BC102["BC-TLS-102\nWeak-cipher evidence unbounded"] --> AC102["AC-102\nEvidence capped at 64\n+ elision marker"]
    AC102 --> T1["test: overflow_weak_cipher_evidence_is_capped\n(red→green, TDD)"]
    T1 --> IMPL["src/analyzer/tls.rs\nWEAK_CIPHER_EVIDENCE_CAP = 64"]
Loading

Test Evidence

  • TDD red→green: 477a6f3 introduces the failing test; d22b9fe makes it green.
  • Test added: test_overflow_weak_cipher_evidence_is_capped in tests/tls_analyzer_tests.rs — constructs a synthetic ClientHello with 200 weak ciphers, asserts evidence.len() == 65 (64 real + 1 elision marker), and verifies the marker format (+136 more).
  • Full suite: 1,128 tests green locally (cargo test --all-targets).
  • Clippy: clean under -D warnings.
  • Fmt: cargo fmt --check passes.

Security Review

No new attack surface introduced. The fix reduces (not increases) the maximum transient allocation. No injection, auth, or input-validation concerns. OWASP Top 10 not applicable to this change.

Risk Assessment

Dimension Assessment
Blast radius Single function (handle_client_hello) in the TLS analyzer
Performance impact Neutral to positive (allocation cap reduces peak memory)
Behavioral change evidence Vec truncated at 64 entries + elision marker appended
Regression risk Low — existing tests continue to pass; new test validates the cap

Dependency Graph

graph LR
    THIS["fix/weak-cipher-cap\n(this PR)"] --> DEVELOP["develop"]
Loading

No upstream PR dependencies.

Pre-Merge Checklist

Closes #102

Zious11 added 2 commits June 8, 2026 14:22
Add test_weak_cipher_evidence_capped_at_64_with_elision: builds a
ClientHello with 65 distinct weak cipher IDs (all confirmed to match
is_weak_cipher via NULL/ANON/EXPORT name check) and asserts the
resulting finding's evidence vec is bounded at 65 entries — 64 cipher
names plus a "(+1 more)" elision marker.

Currently RED: the uncapped implementation produces 65 cipher-name
entries and no elision marker, failing the last-entry elision check.

Classification: LOW-SEVERITY HARDENING (CWE-405-adjacent but not
CWE-405; bounded linear transient allocation, not asymmetric
amplification).
…hardening)

Cap the weak-cipher evidence collection to WEAK_CIPHER_EVIDENCE_CAP=64
entries.  When the ClientHello offers more than 64 weak ciphers, append
a single "(+N more)" elision marker as the 65th evidence entry instead
of collecting all names.

The total weak count is computed with a separate .count() pass (one
extra scan of the cipher slice, bounded by MAX_RECORD_PAYLOAD=18,432),
so the cap-plus-elision branch is O(n) in the cipher list length.

Classification: LOW-SEVERITY HARDENING.  Input is bounded upstream
(MAX_RECORD_PAYLOAD=18,432 bytes → ≤9,216 cipher IDs), so the
uncapped allocation was a bounded linear transient allocation, NOT
asymmetric amplification.  This does NOT meet CWE-405.  The cap simply
avoids a few-hundred-KB String allocation on crafted maximal inputs.

Existing behavior for ≤64 weak ciphers is unchanged (no elision entry
appended, output identical to before).

Also applies rustfmt formatting to the failing test committed in the
previous commit.
@Zious11 Zious11 merged commit 153f225 into develop Jun 8, 2026
8 checks passed
@Zious11 Zious11 deleted the fix/weak-cipher-cap branch June 8, 2026 19:30
Zious11 added a commit that referenced this pull request Jun 8, 2026
…positions (D-022)

Cohort A (#100-#104) re-validated under DF-VALIDATION-001 (all 5 STILL-VALID).
- #104 CLOSED: SNI control-byte summary for mixed control+non-ASCII (BC-TLS-037, PR #194 cf21168)
- #102 CLOSED: cap weak-cipher evidence vec at 64 + elision; reclassified low-severity
  hardening NOT CWE-405/DoS (PR #195 153f225)
- #100: FEATURE-MODE — breaks StreamHandler::on_data + ~22 emission sites; pending feature scope
- #101: OPEN-DEBT — corpus-dependent FP/TP measurement; blocks #103
- #103: DEFERRED — wirerust-original size-symmetry discriminator; sequence with #101
Dependabot live; PR #193 (actions/checkout 6→6.0.2) pending disposition.
develop_head updated: 2fe3440153f225.
Count-propagation sweep: develop_head updated in STATE.md frontmatter + session checkpoint.
Old SHA 2fe3440 retained only in historical D-021 and DI-001 drift rows (immutable audit trail).
Zious11 added a commit that referenced this pull request Jun 8, 2026
…gate (D-023)

D-023: hybrid Claude+Gemini adversarial review of dependabot PR #193; both
converged on SHA-pin verdict. Hallucination sweep: 2 Gemini fabrications
discarded. Prior pass (D-021) was 3/8 complete; now 7/8 pinned (checkout,
rust-cache, cargo-deny, semantic-pr + prior upload-artifact, download-artifact,
gh-release). Action pin gate CI job added; CLAUDE.md policy documented.
PR #196 → develop 77fd45f.

PG-4 [codified]: dependabot tag→SHA gap now enforced by pin gate.
ACTION-PIN-001: dtolnay/rust-toolchain exempt (intentional).
develop_head: 153f22577fd45f.

Count-propagation sweep: develop_head updated in STATE.md only (single source
of truth; no index files reference this SHA). Historical 153f225 cite in D-022
row retained (immutable audit trail — PR #195 merge commit).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tls): cap weak-cipher evidence Vec length in ClientHello analyzer (CWE-405 hardening)

1 participant