Re: [PATCH 0/1] Sequence number out of range fix

From: "Gix, Brian" <brian.gix@intel.com>
To: "jakub.witowski@silvair.com" <jakub.witowski@silvair.com>,
	"linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>
Cc: "istotlan@ingas-xps13.amr.corp.intel.com" 
	<istotlan@ingas-xps13.amr.corp.intel.com>,
	"michal.lowas-rzechonek@silvair.com" 
	<michal.lowas-rzechonek@silvair.com>
Subject: Re: [PATCH 0/1] Sequence number out of range fix
Date: Wed, 15 Jan 2020 18:35:05 +0000	[thread overview]
Message-ID: <4da92cbf6134822629bd2204fb7161c04008780e.camel@intel.com> (raw)
In-Reply-To: <20200115170830.7479-1-jakub.witowski@silvair.com>

Hi Jakub,

On Wed, 2020-01-15 at 18:08 +0100, Jakub Witowski wrote:
> During sending messages with high speed, the 'cached' value may have exceeded the 24-bit range
> and this value will save into storage.
> 
> The current implementation protects us against exceeding max sequence number (0xFFFFFF).
> 
> Jakub Witowski (1):
>   mesh: sequence number out of range fix
> 
>  mesh/mesh-config-json.c | 4 ++++
>  mesh/net.c              | 3 +++
>  2 files changed, 7 insertions(+)
> 

This is a very dangerous course of action.  The suggested patch might potentially cause a node to re-use a
sequence number more than once, which would cause a "dirty nonce" condition, and allow unauthorized entities to
derive the encrypted data without the keys.

The Mesh working group put a lot of work into determining what should be range of Sequence Numbers and IV
Indexes, and came up with the 24 bits of sequence numbers, which can be reset to 0 with an IV Index update
every 192 hours.  This will allow a node to send 24 packets (every 42ms) a second constantly without running
out of sequence numbers.  While this is technically possible, especially with a daemon that might be looping
back messages internally without ever using the OTA interface, it is not really possible in an actual BT driven
real life system.

So here is the solution I would suggest:

Beacuse we store sequence numbers internally with a u32 sized data type, we should *let* the value go over the
max legal sequence nunmbver of 0xFFFFFF (perhaps capping it at 0x01000000 to prevent "super overflows").  Then
we *reject* all send requests with a sequence number > 0xFFFFFF.

Assuming the IV Index update logic is working correctly (triggered internally when the Sequence number goes
over 0x00800000... half the max) the node will work just fine as long as it never tries to send more than one
packet every 42ms for 192 hours without rest...  Just as the specification intended.  If it does, then the
specification and code will disallow it. 

BR,
Brian