Skip to content

fix: bounds check the lookup table indexes in gossip - #3508

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:validate-gossip-indexes
Open

fix: bounds check the lookup table indexes in gossip#3508
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:validate-gossip-indexes

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Motivation

Gossip interns addresses, roles, hashes and app versions into tables and refers to them by
index. Every one of those indexes went straight from the wire into Vector.apply with no range
check:

Site Table
ClusterMessageSerializer:551,554 observer and subject addresses in reachability
:570 member address
:578 member role
:572 member app version
:588 tombstone address
:596 seen addresses
:605 vector clock hash, reached from both Gossip and GossipStatus

A negative or out of range index raises IndexOutOfBoundsException rather than a serialization
failure. For a GossipEnvelope this lands somewhere unexpected: gossipEnvelopeFromProto
(:608-615) defers the whole parse into a () => thunk, so the throw happens inside
ClusterCoreDaemon when the gossip is read, not on a deserialization thread where a failure
would be contained.

Modification

Look the indexes up through a helper that range checks first and reports a
NotSerializableException naming the index and the table size. Every index gossipToProto
writes is in range by construction, so nothing a peer legitimately sends is affected.

Correction to what I reported earlier. I had also flagged memberStatusFromInt and
reachabilityStatusFromInt as reachable NoSuchElementExceptions on an unknown enum number.
That is wrong: ClusterMessages.proto is proto2, where an unrecognised enum value is moved to
the unknown fields and the accessor returns the default, so getStatus.getNumber is always one
of the declared values and both maps are total over it. I had a check written for those two and
removed it rather than ship defensive code that cannot fire and cannot be tested.

Result

Gossip that refers to a table entry the sender did not include is reported as a serialization
failure naming the index and the size.

Tests

  • sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.*" — 12 passed

Three new tests, each checked to discriminate by reverting the production file and re-running;
all three then fail with IndexOutOfBoundsException was thrown:

  • reject gossip that refers to a lookup table entry it did not send — takes a real Welcome,
    decompresses it, and tampers one index at a time: an address index past the end, a negative
    address index, a role index, a seen entry, and a vector clock hash index

  • reject a gossip status that refers to a hash it did not sendGossipStatus reaches
    vectorClockFromProto without going through Gossip, and is not compressed

  • reject a gossip envelope with a bad index when the gossip is read — asserts the failure
    surfaces from envelope.gossip, not from fromBinary, which is where the deferred parse puts it

  • sbt "cluster/mimaReportBinaryIssues" — no issues

  • sbt "cluster/scalafmtCheckAll" headerCreateAll — clean

References

None.

Motivation:
Gossip interns addresses, roles, hashes and app versions into tables and
refers to them by index. Every index in gossipFromProto came straight off
the wire into Vector.apply with no range check, so a negative or out of
range one raised IndexOutOfBoundsException instead of a serialization
failure.

For a GossipEnvelope this matters more than usual: gossipEnvelopeFromProto
defers the parse into a thunk, so the throw happens inside ClusterCoreDaemon
when the gossip is read rather than on a deserialization thread.

Modification:
Look the indexes up through a helper that range checks first and reports a
NotSerializableException naming the index and the table size. Every index
gossipToProto writes is in range, so nothing a peer legitimately sends is
affected.

Result:
Gossip that refers to a table entry the sender did not include is reported
as a serialization failure.
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.

1 participant