LowerTransportLayer.checkAgainstReplayAttack Improperly Records Sequence Number for Segmented Messages #345
Comments
|
I think the problem lies in this extension: private extension NetworkPdu {
/// Whether the Network PDU contains a segmented Lower Transport PDU,
/// or not.
var isSegmented: Bool {
return transportPdu[0] & 0x80 > 1
}
/// The 24-bit Seq Auth used to transmit the first segment of a
/// segmented message, or the 24-bit sequence number of an unsegmented
/// message.
var messageSequence: UInt32 {
if isSegmented {
let sequenceZero = (UInt16(transportPdu[1] & 0x7F) << 6) | UInt16(transportPdu[2] >> 2)
return (sequence & 0xFFE000) | UInt32(sequenceZero)
} else {
return sequence
}
}
}Because later on this is how it gets used: func checkAgainstReplayAttack(_ networkPdu: NetworkPdu) -> Bool {
// Don't check messages sent to other Nodes.
guard !networkPdu.destination.isUnicast ||
meshNetwork.localProvisioner?.node?.hasAllocatedAddress(networkPdu.destination) ?? false else {
return true
}
let sequence = networkPdu.messageSequence
let receivedSeqAuth = (UInt64(networkPdu.ivIndex) << 24) | UInt64(sequence)
...
...
...
// Validate.
guard receivedSeqAuth > localSeqAuth || missed ||
(reassemblyInProgress && receivedSeqAuth == localSeqAuth) else {
// Ignore that message.
logger?.w(.lowerTransport, "Discarding packet (seqAuth: \(receivedSeqAuth), expected > \(localSeqAuth))")
return false
}
// The message is valid. Remember the previous SeqAuth.
let newPreviousSeqAuth = min(receivedSeqAuth, localSeqAuth)
...
...
...
}Somehow the segmented |
|
In fact, the "Mesh Profile Section 3.5.3.1 Segmentation" states:
When using the example values from above in the messageSequence computed variable, the wrong result is yielded on both counts. Taking the implementation from the nRF5-SDK-for-Mesh's implementation, you can see that it is quite different. It also yields the correct results. static uint32_t seqauth_sequence_number_get(uint32_t seqnum, uint16_t seqzero)
{
if ((seqnum & TRANSPORT_SAR_SEQZERO_MASK) < seqzero)
{
return ((seqnum - ((seqnum & TRANSPORT_SAR_SEQZERO_MASK) - seqzero) - (TRANSPORT_SAR_SEQZERO_MASK + 1)));
}
else
{
return ((seqnum - ((seqnum & TRANSPORT_SAR_SEQZERO_MASK) - seqzero)));
}
}Note: |
|
Thank you for the bug report and detailed explanation. I'll look into it now. |
|
The issue was, as you said, related to how the message sequence was calculated. |
|
Released in 3.1.2. |
|
Hi, could you, please, verify that it's working now with 3.1.2? |
Describe the bug
If you send both segmented and unsegmented messages from a node to the app, eventually the sequence number will be malformed and much greater than the actual sequence number send by the device. All messages between this event and the sequence number catching up will be discarded.
This results in my device being unusable with your library, which we have already invested a lot of time and money in.
To Reproduce
Steps to reproduce the behavior:
receivedSeqAuthandlocalSeqAuthinLowerTransportLayer.checkAgainstReplayAttacklocalSeqAuthis suddenly thousands higher than it should be (message it is generated from always seems to be a segmented message)receivedSeqAuthcatches up with insanely largelocalSeqAuththousands of messages later.Expected behavior
Generally, as messages are received, the
localSeqAuthshould increment by 1 or 2 points with each message, regardless of segmentation.Logs / Screenshots
The important fields to note above are
receivedSeqAuth,Network PDU.ivi,Network PDU.seq, andlocalSeqAuth.uint64ValueThe text was updated successfully, but these errors were encountered: