fix(sync): reject out-of-range GCS parameters instead of decoding garbage - #894
Open
Chessing234 wants to merge 2 commits into
Open
fix(sync): reject out-of-range GCS parameters instead of decoding garbage#894Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
…bage decodeToSortedSet takes p and m straight off the wire - REQUEST_SYNC carries P as a uint8, so 0..255 all arrive - and decodes with them regardless. Kotlin's shift operators use the low 6 bits of the count, so p >= 64 wraps and the decoder emits values that were never in the filter; p = 0 turns every remainder into a zero-width read and manufactures a value per bit. On today's 20-id filter, p = 0 decodes 105 values. Those values are read as "the peer already has this packet", so the responder withholds packets the peer actually lacks - the failure direction that loses messages. Guard the parameters as iOS does in GCSFilter.decodeToSortedSet: p in 1..32 and m > 1, otherwise decode to nothing, which callers read as "peer has nothing" and answer by sending the data.
Round trip with the built parameters, out-of-range p (0, 33, 64, 200, 255) decoding to nothing, and degenerate m (0, 1) decoding to nothing. The p case fails without the guard: p = 0 yields 105 fabricated values.
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.
GCSFilter.decodeToSortedSettakespandmstraight off the wire — a REQUEST_SYNC carries P as a uint8, so anything from 0 to 255 arrives — and decodes with them regardless.p >= 64wraps and the decoder emits values that were never in the filter;p = 0makes every remainder a zero-width read, so the decoder manufactures a value per bit. On a 20-id filter that is 105 fabricated values.The decoded set is what
GossipSyncManager.handleRequestSyncconsults to decide whether a peer already has a packet, so fabricated values mean the responder withholds packets the peer actually lacks — the direction that loses messages.iOS already guards this, with the reasoning spelled out in
GCSFilter.swift:This is the same guard, with the same bound (
MAX_P = 32), so both platforms agree on what a filter means.Verified locally with
./gradlew :app:testDebugUnitTest --tests "com.bitchat.android.sync.*":an out-of-range p decodes to nothing rather than to garbagefails withp=0 should decode to nothing, got 105 values;GCSFilterParameterTestistests="3" failures="0"and the existingGCSFilterTeststill passestests="3" failures="0".