The Raft consensus loop took the bytes of a committed log entry (or s…

[neeelkhadwal]

• Bug fix

Description

The etcdraft consensus chain calls protoutil.UnmarshalBlockOrPanic(...) in three places when decoding committed Raft entries and snapshot data:

• orderer/consensus/etcdraft/chain.go:255 — in NewChain, on snapshot data read at startup
• orderer/consensus/etcdraft/chain.go:1192 — in apply, on each committed EntryNormal
• orderer/consensus/etcdraft/chain.go:1253 — in apply, when preparing a snapshot trigger

If the bytes do not decode as a common.Block protobuf, UnmarshalBlockOrPanic raises a Go panic and terminates the entire orderer process — taking down every channel hosted by that orderer, not just the affected chain.

This is a critical denial-of-service vector for two reasons:

1. Cluster-wide crash from a single bad entry. Raft replicates committed entries byte-for-byte to every follower. A byzantine proposer (or any node compromised enough to get bytes accepted as a Raft proposal — cluster TLS authenticates the sender, not the payload)
   can cause every follower in the cluster to panic in lockstep on the same apply call.
2. Permanent crashloop on corrupted local state. A power loss or disk fault that corrupts the on-disk snapshot/WAL puts the node into an unrecoverable startup crashloop, because NewChain re-reads the same bad snapshot on every restart and panics.

The fix replaces all three call sites with protoutil.UnmarshalBlock (the error-returning variant, which the same file already uses on line 1052) and handles the error path appropriately for each context:

• In NewChain, the wrapped error is returned to the caller so the operator sees a real startup error instead of a crashloop.
• In apply, since the function has no return and skipping a committed entry would silently fork this node's state from the rest of the cluster, the chain is halted via the existing c.halt() graceful-shutdown path. halt is invoked on a goroutine to avoid deadlocking
  against the serve loop — the same pattern is used a few lines below for the conf-change halt path. Other channels in the same orderer process are unaffected.

CWE-248 (Uncaught Exception) / CWE-754 (Improper Check for Unusual or Exceptional Conditions).

Additional details

Diff is 24 insertions / 3 deletions across a single file; no new imports, no API changes. protoutil.UnmarshalBlock is already used elsewhere in the same file, so this aligns the three holdouts with the established pattern.

Behavior change summary:

Scenario	Before	After
Malformed committed Raft entry	Orderer process panics; all channels go down	Affected chain logs the raft index, halts via haltCallback; other channels continue
Corrupted on-disk snapshot at startup	NewChain panics → systemd/k8s crashloop	NewChain returns a wrapped error; operator-visible failure
Healthy operation	unchanged	unchanged

Testing notes for reviewers:

• Existing etcdraft unit tests should continue to pass; they exercise the success path.
• Suggested new tests (not yet added): (a) unit test calling apply with an EntryNormal whose Data is random non-protobuf bytes and asserting haltCallback is invoked rather than panic; (b) unit test constructing NewChain against a MemoryStorage seeded with a snapshot
  containing garbage Data and asserting an error return.

Related issues

None — surfaced during a security review of the consensus path; no public issue filed yet.

[Atishyy27]

The NewChain change (returning an error instead of panicking on a corrupt snapshot) looks like a clear improvement.

On the apply() changes, I think there's a window worth closing: haltC is serviced by the node goroutine (node.go L179) while run() keeps consuming applyC. Since we return from apply() without advancing appliedIndex past the bad entry, a subsequent Ready batch delivered before the halt lands passes the Index <= appliedIndex guard and reaches writeBlock, which panics on the block-number gap (chain.go L890: "Got block [%d], expect block [%d]") — i.e. under this race we still panic, just with a more confusing message. The existing go c.halt() in the remove-node path doesn't have this issue because it halts after cleanly applying the entry. A "halting" flag checked at the top of apply() (or draining further applyC batches once corruption is detected) would make the halt deterministic.

Bigger-picture question for maintainers: since the corrupt entry persists in the WAL, the chain will re-hit it and re-halt on every restart — channel down while the process looks healthy. Is chain-halt preferable here to the current fail-fast panic, which at least produces an unmissable ops signal for storage corruption? If halt is the direction, a doc note on operator recovery would help.

[tock-ibm]

Cluster-wide crash from a single bad entry. Raft replicates committed entries byte-for-byte to every follower. A byzantine proposer (or any node compromised enough to get bytes accepted as a Raft proposal — cluster TLS authenticates the sender, not the payload)

Note that Raft is not BFT. It protects only from crash failures (CFT). If your system requires BFT use the BFT consensus protocol. A malicious leader or node can create much more sinister effects than just corrupt the message, it can send different messages to different followers and silently fork the network.

Corrupted on-disk snapshot at startup

Corrupt storage due to a hardware failure is also not covered by Raft (CFT).

The fact that an ordering node hosts multiple chains is a deployment issue. One can easily deploy each chain on mutually exclusive clusters of processes.

If hardware failed, the standard remedy is to add a new node (new raft-id) and remove the old.

Some would argue that when a chain in a node fails "silently" via "halt", it is more dangerous than failing fast, as the admin is not aware of this. If implemented, this must be accompanied by a way to monitor the state of a chain in a node and mark it as "errored" or "failed".

I think this must me reflected with an issue or even an RFC as it changes the observable behavior of a node.

[neeelkhadwal]

The NewChain change (returning an error instead of panicking on a corrupt snapshot) looks like a clear improvement.

On the apply() changes, I think there's a window worth closing: haltC is serviced by the node goroutine (node.go L179) while run() keeps consuming applyC. Since we return from apply() without advancing appliedIndex past the bad entry, a subsequent Ready batch delivered before the halt lands passes the Index <= appliedIndex guard and reaches writeBlock, which panics on the block-number gap (chain.go L890: "Got block [%d], expect block [%d]") — i.e. under this race we still panic, just with a more confusing message. The existing go c.halt() in the remove-node path doesn't have this issue because it halts after cleanly applying the entry. A "halting" flag checked at the top of apply() (or draining further applyC batches once corruption is detected) would make the halt deterministic.

Bigger-picture question for maintainers: since the corrupt entry persists in the WAL, the chain will re-hit it and re-halt on every restart — channel down while the process looks healthy. Is chain-halt preferable here to the current fail-fast panic, which at least produces an unmissable ops signal for storage corruption? If halt is the direction, a doc note on operator recovery would help.

Because apply() returns without advancing appliedIndex and halt() is async, the next applyC batch races in and still panics (top-of-apply guard or the writeBlock gap). I'll add a synchronously-set halting flag checked at the top of apply() so the halt is deterministic. On direction: I'll pair the halt with StatusFailed (surfaced via the participation API + consensus_relation_and_status metric) plus a doc note on operator recovery, so it's an observable failure rather than a silent one / confusing crash. Tracking the behavior change in an issue per tock-ibm's note.

[neeelkhadwal]

Cluster-wide crash from a single bad entry. Raft replicates committed entries byte-for-byte to every follower. A byzantine proposer (or any node compromised enough to get bytes accepted as a Raft proposal — cluster TLS authenticates the sender, not the payload)

Note that Raft is not BFT. It protects only from crash failures (CFT). If your system requires BFT use the BFT consensus protocol. A malicious leader or node can create much more sinister effects than just corrupt the message, it can send different messages to different followers and silently fork the network.

Corrupted on-disk snapshot at startup

Corrupt storage due to a hardware failure is also not covered by Raft (CFT).

The fact that an ordering node hosts multiple chains is a deployment issue. One can easily deploy each chain on mutually exclusive clusters of processes.

If hardware failed, the standard remedy is to add a new node (new raft-id) and remove the old.

Some would argue that when a chain in a node fails "silently" via "halt", it is more dangerous than failing fast, as the admin is not aware of this. If implemented, this must be accompanied by a way to monitor the state of a chain in a node and mark it as "errored" or "failed".

I think this must me reflected with an issue or even an RFC as it changes the observable behavior of a node.

On CFT vs BFT: agreed. Raft here is CFT, so I'm dropping the byzantine-proposer language entirely — a malicious node can silently fork the network regardless, and hardening one unmarshal path buys nothing against that adversary. Same for corrupt on-disk storage: that's a hardware-fault case outside the CFT model, and the right remedy is add a new raft-id / remove the old node. I'll also drop the "multiple chains per process" argument since chain isolation is a deployment choice.

On silent halt being more dangerous than fail-fast: this is the point I want to get right, and I agree a silent halt() is the wrong outcome. Rather than fail the whole process, the intent is a contained and observable failure of the single affected chain. Fabric already has the mechanism for the "mark it as failed" part you're asking for: on this path I'll set the chain's status to types.StatusFailed, which surfaces through the Channel Participation API (StatusReport) and the consensus_relation_and_status metric, plus a doc note on operator recovery. So it's not a silent death — an operator gets a clear "failed" signal on that channel while the other channels keep serving.

On process: agreed this changes the observable behavior of a node, so I'll open an issue documenting the behavior change (panic → failed-and-halted chain, surfaced via participation status + metric) and link it here before proceeding on the runtime apply() portion. The NewChain startup change (error return instead of panic-crashloop) doesn't change steady-state behavior, so I'd propose keeping that piece as-is.

Does gating the apply() changes on that issue, with StatusFailed as the observability mechanism, sound like the right direction to you?

[neeelkhadwal]

Note that Raft is not BFT. It protects only from crash failures (CFT)... A malicious leader or node can create much more sinister effects than just corrupt the message, it can send different messages to different followers and silently fork the network.

Thanks — you're right on the threat model, and I'll rework the framing accordingly.

On CFT vs BFT: agreed. Raft here is CFT, so I'm dropping the byzantine-proposer language entirely — a malicious node can silently fork the network regardless, and hardening one unmarshal path buys nothing against that adversary. Same for corrupt on-disk storage: that's a hardware-fault case outside the CFT model, and the right remedy is add a new raft-id / remove the old node. I'll also drop the "multiple chains per process" argument since chain isolation is a deployment choice.

On silent halt being more dangerous than fail-fast: this is the point I want to get right, and I agree a silent halt() is the wrong outcome. Rather than fail the whole process, the intent is a contained and observable failure of the single affected chain. Fabric already has the mechanism for the "mark it as failed" part you're asking for: on this path I'll set the chain's status to types.StatusFailed, which surfaces through the Channel Participation API (StatusReport) and the consensus_relation_and_status metric, plus a doc note on operator recovery. So it's not a silent death — an operator gets a clear "failed" signal on that channel while the other channels keep serving.

On process: agreed this changes the observable behavior of a node, so I'll open an issue documenting the behavior change (panic → failed-and-halted chain, surfaced via participation status + metric) and link it here before proceeding on the runtime apply() portion. The NewChain startup change (error return instead of panic-crashloop) doesn't change steady-state behavior, so I'd propose keeping that piece as-is.

Does gating the apply() changes on that issue, with StatusFailed as the observability mechanism, sound like the right direction to you?