Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 15 Jan 2020 22:04:14 +0100
From:   "michal.lowas-rzechonek@silvair.com" 
        <michal.lowas-rzechonek@silvair.com>
To:     "Gix, Brian" <brian.gix@intel.com>
Cc:     "jakub.witowski@silvair.com" <jakub.witowski@silvair.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "istotlan@ingas-xps13.amr.corp.intel.com" 
        <istotlan@ingas-xps13.amr.corp.intel.com>
Subject: Re: [PATCH 0/1] Sequence number out of range fix
Message-ID: <20200115210414.fvywao5evmbx5725@kynes>
Mail-Followup-To: "Gix, Brian" <brian.gix@intel.com>,
        "jakub.witowski@silvair.com" <jakub.witowski@silvair.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "istotlan@ingas-xps13.amr.corp.intel.com" <istotlan@ingas-xps13.amr.corp.intel.com>
References: <20200115170830.7479-1-jakub.witowski@silvair.com>
 <4da92cbf6134822629bd2204fb7161c04008780e.camel@intel.com>
 <20200115190904.qcusabsj3ti7sid4@kynes>
 <4C48D122-7887-4A5E-9E3C-0A2FE15D6A1A@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4C48D122-7887-4A5E-9E3C-0A2FE15D6A1A@intel.com>
User-Agent: NeoMutt/20180716
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

On 01/15, Gix, Brian wrote:
> > Good point. Note that his is also possible in the current
> > implementation: since seq_num is applied to nonce with a 24bit mask,
> > it's going to wrap around.
> 
> The full IV Index is in the nonce, and at 192 hours per IV index, will
> be unique for something like 1.4 million years.

Yes it is, but that's not the point.

At the moment, net->seq_num is a 32 bit value that *can* exceed 24bit
range, because mesh_net_next_seq_num() doesn't check ranges. So the
raw value can reach 0x1000000 and above.

Now, this raw value is used in send_seg, passed to
mesh_crypto_packet_build, which effectively applies a 24bit mask in line
640:

	l_put_be32(seq, packet + 1);
	packet[1] = (ctl ? CTL : 0) | (ttl & TTL_MASK);

So this means that when:

 - the network is already in iv update (so that you can't increase the
   iv_index, maybe you even started the procedure yourself because your
   seq_num is above the threshold)

 - your sequence number is sufficiently large (because of the "repeated
   crash" scenario described below)

Then the actual value used in the nonce will be net->seq_num & 0xffffff,
which is something you have *already* used before. All of that happens
with the same IV index.

> The over commit is calculated based on the usage rate, and the daemon
> would need to unexpectedly abort (not just ctrl-c or exit) for us to
> use the over-commit value

Indeed, that's precisely what I'm talking about - repeated, unhandled
process terminations. We're trying to come up with the patch simply
because this situation has *already happened* on one of our instances.

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND