33735 - [BC - Insight] Network split due to the sync issue in PP modul...
Submitted on Jul 27th 2024 at 23:33:17 UTC by @ret2happy for Boost | Shardeum: Core
Report ID: #33735
Report type: Blockchain/DLT
Report severity: Insight
Target: https://github.com/shardeum/shardus-core/tree/dev
Impacts:
Unintended chain split (network partition)
Description
Brief/Intro
Incorrect and insufficient type validation for the sync-cycles P2P http response, leading to the sync issue and the network partition during the sync.
Vulnerability Details
When the node tries to get the cycles with counter from start to end inclusively, it iter each p2p node with the sync-cycles http POST request, and parse the request to get the cycles information. It validate the response from the HTTP POST response in [1].
const queryFn = async (node: SyncNode) => {
const ip = node.ip ? node.ip : node.externalIp
const port = node.port ? node.port : node.externalPort
const resp = await http.post(`${ip}:${port}/sync-cycles`, data)
return resp
}
/* prettier-ignore */ if (logFlags.p2pNonFatal) info(`getCycles: ${start} - ${end}...`)
// use robust query so we can ask less nodes to get the cycles
let redundancy = 1
if (activeNodes.length > 5) redundancy = 2
if (activeNodes.length > 10) redundancy = 3
const { topResult: response, winningNodes: _responders } = await robustQuery(
activeNodes,
queryFn,
util.isDeepStrictEqual,
redundancy,
true
) // [1] get response from the `sync-cycles` endpoint.
// [TODO] Validate whatever came in
const cycles = response as P2P.CycleCreatorTypes.CycleRecord[]
const valid = validateCycles(cycles) // [1] validate response by the validateCycles function
if (valid) return cycles
}
}However, there's incorrect and insufficient type validation in the validateCycles functions.
In the validateCycles function, line 555 of the Sync.ts, it lacks checking whether cycles is iterable. Hence iter the cycles will result the js runtime exception. (Not that the previous response is unknown type and is force cast dynamically to the P2P.CycleCreatorTypes.CycleRecord, which doesn't necessarily to be the array type. )
Moreover, in line 579 - line 581, when the type is mismatched, it only log a warning without directly return false, leading to the execution on the wrongly typed cycles data. Hence in the following content of this function, each field is not type validated at all, and could cause many type iter exception and thus fail to validate the type as the correct one.
Impact Details
The malicious P2P response can stuck and cause exception during the sync since there's no error catching for the cycle sync function. Moreover, there's missing validation on the wrong P2P response, which also create consensus issue among the nodes via p2p request.
References
[1] https://github.com/shardeum/shardus-core/blob/4d75f797a9d67af7a94dec8860220c4e0f9ade3c/src/p2p/Sync.ts#L495-L507
[2] https://github.com/shardeum/shardus-core/blob/4d75f797a9d67af7a94dec8860220c4e0f9ade3c/src/p2p/Sync.ts#L548-L582
Proof of Concept
I create a unit test case under the shardus-core repo, the file is named as test/unit/src/ValidateCycles.test.ts.
These PoC demonstrates three different case which validateCycles failed to validate.
The first case is that the map type (the cycles data type itself), (default by json response) is not handled by validateCycles, hence it cause exception rather than return false.
The second case is that the wrong type of refreshedArchivers field cause the exception rather than the false return value from the validateCycles. This is because the validateCycles doesn't return false even it checks the field types.
The third case is that the wrong types of unchecked/un-itered field such as safetyMode/safetyNum/networkStateHash/desired are not type checked. However, these field are later used in the cycle calculation, resulting the insufficient field checking. These arbitrary unchecked field can further break the p2p consensus.
Running these PoC by
You will get the console output:
Last updated
Was this helpful?