mirror of
https://github.com/permissionlesstech/bitchat-android.git
synced 2026-11-01 07:51:15 +00:00
Budget REQUEST_SYNC by ingress link, not by sender ID
SecurityManager.verifyPacketSignature only authenticates ANNOUNCE, MESSAGE, FILE_TRANSFER, VOICE_FRAME and LEAVE; every other type returns early as verified. REQUEST_SYNC is not in that set, so the sender ID is raw bytes off the wire and costs nothing to rotate. Keyed on that, the budget was bypassable: cycling more IDs than the LRU tracks gives every request a brand new bucket with a full allowance, so the flood this was meant to stop still went through. Found by Codex on the first push. Key it on RoutedPacket.ingressLinkID instead — assigned by the transport, never serialized onto the mesh, and distinct per link. A flooder is then bounded by the connections it can actually hold open rather than by the names it is willing to invent. Falls back to the sender ID only when a transport supplies no link identity.
This commit is contained in:
parent
48b85fb498
commit
3c126b20db
@ -654,7 +654,7 @@ class BluetoothMeshService(private val context: Context) : TransportBridgeServic
|
||||
// Decode request and respond with missing packets
|
||||
val fromPeer = routed.peerID ?: return
|
||||
val req = RequestSyncPacket.decode(routed.packet.payload) ?: return
|
||||
gossipSyncManager.handleRequestSync(fromPeer, req)
|
||||
gossipSyncManager.handleRequestSync(fromPeer, req, routed.ingressLinkID)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -525,7 +525,7 @@ class MeshCore(
|
||||
override fun handleRequestSync(routed: RoutedPacket) {
|
||||
val fromPeer = routed.peerID ?: return
|
||||
val req = RequestSyncPacket.decode(routed.packet.payload) ?: return
|
||||
gossipSyncManager.handleRequestSync(fromPeer, req)
|
||||
gossipSyncManager.handleRequestSync(fromPeer, req, routed.ingressLinkID)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -200,11 +200,11 @@ class GossipSyncManager(
|
||||
}
|
||||
|
||||
/**
|
||||
* Charges [fromPeerID] for one request and returns how many packets it may be sent in
|
||||
* response, or 0 when the request must be dropped. Called before any decoding or hashing so
|
||||
* a flood costs us the admission check and nothing else.
|
||||
* Charges [budgetKey] for one request and returns how many packets may be sent in response,
|
||||
* or 0 when the request must be dropped. Called before any decoding or hashing so a flood
|
||||
* costs us the admission check and nothing else.
|
||||
*/
|
||||
private fun admitRequest(fromPeerID: String): Int {
|
||||
private fun admitRequest(budgetKey: String): Int {
|
||||
val capacity = try {
|
||||
configProvider.seenCapacity()
|
||||
} catch (_: Exception) {
|
||||
@ -213,7 +213,7 @@ class GossipSyncManager(
|
||||
val now = nowMs()
|
||||
|
||||
synchronized(requesterBudgets) {
|
||||
val budget = requesterBudgets.getOrPut(fromPeerID) {
|
||||
val budget = requesterBudgets.getOrPut(budgetKey) {
|
||||
RequesterBudget(
|
||||
requestTokens = AppConstants.Sync.REQUEST_BURST.toDouble(),
|
||||
responseTokens = capacity,
|
||||
@ -245,16 +245,28 @@ class GossipSyncManager(
|
||||
requesterBudgets.size
|
||||
}
|
||||
|
||||
private fun chargeResponses(fromPeerID: String, sent: Int) {
|
||||
private fun chargeResponses(budgetKey: String, sent: Int) {
|
||||
if (sent <= 0) return
|
||||
synchronized(requesterBudgets) {
|
||||
val budget = requesterBudgets[fromPeerID] ?: return
|
||||
val budget = requesterBudgets[budgetKey] ?: return
|
||||
budget.responseTokens = (budget.responseTokens - sent).coerceAtLeast(0.0)
|
||||
}
|
||||
}
|
||||
|
||||
fun handleRequestSync(fromPeerID: String, request: RequestSyncPacket) {
|
||||
var remaining = admitRequest(fromPeerID)
|
||||
/**
|
||||
* @param ingressLinkID locally-assigned identity of the link the request arrived on. REQUEST_SYNC
|
||||
* is not in the set of types `SecurityManager.verifyPacketSignature` authenticates, so the
|
||||
* sender ID on the wire is unauthenticated and free to rotate — budgeting by it would let a
|
||||
* peer mint a fresh allowance per request. This value is assigned by the transport and never
|
||||
* serialized onto the mesh, so a flooder is bounded by the links it can actually hold open.
|
||||
*/
|
||||
fun handleRequestSync(
|
||||
fromPeerID: String,
|
||||
request: RequestSyncPacket,
|
||||
ingressLinkID: String? = null
|
||||
) {
|
||||
val budgetKey = ingressLinkID ?: fromPeerID
|
||||
var remaining = admitRequest(budgetKey)
|
||||
if (remaining <= 0) {
|
||||
Log.d(TAG, "Dropping REQUEST_SYNC from $fromPeerID: over budget")
|
||||
return
|
||||
@ -296,7 +308,7 @@ class GossipSyncManager(
|
||||
}
|
||||
}
|
||||
|
||||
chargeResponses(fromPeerID, allowance - remaining)
|
||||
chargeResponses(budgetKey, allowance - remaining)
|
||||
}
|
||||
|
||||
private fun hexStringToByteArray(hexString: String): ByteArray {
|
||||
|
||||
@ -153,6 +153,35 @@ class GossipSyncRequestBudgetTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `rotating the sender id does not mint a fresh budget on one link`() {
|
||||
storeMessages(CAPACITY)
|
||||
|
||||
// REQUEST_SYNC is not in the set SecurityManager.verifyPacketSignature authenticates, so
|
||||
// the sender ID is free for a flooder to rotate. Cycling more IDs than the LRU tracks
|
||||
// would hand out a full allowance per request if the budget were keyed on it.
|
||||
repeat(AppConstants.Sync.MAX_TRACKED_REQUESTERS * 2) { i ->
|
||||
manager.handleRequestSync("%016x".format(i), emptyFilter, ingressLinkID = "link-1")
|
||||
}
|
||||
|
||||
assertEquals(CAPACITY, sentTo.size)
|
||||
assertEquals(1, manager.trackedRequesterCount())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `separate links keep separate budgets`() {
|
||||
storeMessages(CAPACITY)
|
||||
|
||||
manager.handleRequestSync(REQUESTER, emptyFilter, ingressLinkID = "link-1")
|
||||
repeat(50) { manager.handleRequestSync(REQUESTER, emptyFilter, ingressLinkID = "link-1") }
|
||||
sentTo.clear()
|
||||
|
||||
// A different physical link is a different neighbour and must not inherit the throttle.
|
||||
manager.handleRequestSync(REQUESTER, emptyFilter, ingressLinkID = "link-2")
|
||||
|
||||
assertEquals(CAPACITY, sentTo.size)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a backwards clock jump does not hand out free budget`() {
|
||||
storeMessages(CAPACITY)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user