Skip to content

0.18.1: the tier system fails OPEN, ALL has no completeness test, and the wording guard misses additive widening (proven) #19

Description

@MichaelTaylor3d

Task

Three residual findings from the 0.18.0 security gate, all additively fixable in a 0.18.1 patch — no
breaking change. Filed together because they are one theme: the tier system and its guards do not fail
closed.

Parent: https://github.com/DIG-Network/dig_ecosystem/issues/2870

1. requires_master_token() fails OPEN

It is a matches! allowlist defaulting to false, so a new ControlMethod variant lands at ordinary
tier by default.
The analogy to is_open_read — which the gate initially reached for — breaks:
there, the default means more auth; here it means less.

name() / routing() / category() / summary() are exhaustive matches, so a new variant is a compile
error in those. Nothing forces a tier decision. Fix: make it an exhaustive match self, so adding a
variant cannot compile without choosing a tier. Internal implementation change — not a breaking one.

No such variant exists today, which is why this is defense-in-depth rather than a live hole. It is also
exactly the kind of gap that is free now and expensive after the next method lands.

2. ControlMethod::ALL has no completeness test

It is a hand-maintained array. every_method_has_a_unique_wire_name compares lengths, which detects
duplicates but not omissions — a variant missing from ALL is invisible. Every test that iterates
ALL (including the new tier-set test) silently under-covers by exactly the methods someone forgot.

Fix: derive or assert completeness so a variant absent from ALL fails the build.

3. The F3 wording guard is blind to ADDITIVE widening — PROVEN

The guard asserts the operator-run scope is present and bans vouch / otherwise trust /
trust yourself / recommend. The gate ran this string through it:

"Add only a node you run yourself, or one a person you trust operates."

142 passed, 0 failed. That sentence is precisely the NC-12 widening F3 exists to prevent — it
authorises trusting a node someone else runs — and the guard waves it through because the banned tokens
are absent and the required phrase is still present.

The guard catches replacement and deletion. It does not catch extension. Same shape as the installer
guard that covered one of the corruption's two shapes: a check believed to close a class while covering
part of it, which is worse than no check because it manufactures confidence.

Fix: assert the whole authorising sentence, or ban the widening constructions (, or , or one,
operates) in that specific string rather than only the four tokens.

Two smaller items, worth folding in

  • The justification beside MAX_BANNED_CHIA_PEERS is wrong. It reasons the cap prevents an attacker
    growing the list "for free"; the sole ban write path is remove {ban: true}, which is master-tier,
    so it costs master. The cap is fine; the stated reason is not, and a wrong rationale invites someone to
    "fix" the wrong thing later.
  • Eviction silently un-bans without reporting, which contradicts this very diff's own principle that
    a remedy must be able to report its own failure — the reasoning that produced ChiaPeerRemovalOutcome.
    Also undefined: whether remove {ban:true} on an untracked address writes a ban. MockNode says
    no; SPEC is silent. Pick one and state it.

Not in scope here

control.config.setUpstream's tier is DIG-Network/dig-node#255 — a consumer-side security issue, not
a contract one.

Evidence

Item 1: a new variant must fail to compile without a tier decision. Item 2: removing a variant from ALL
must fail a test. Item 3: the exact sentence above must go RED.

Effort

Low-medium, all three. 0.18.1 patch.

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind:maintenancereal work that changes nothing a user can perceive

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions