Re: [PATCH 09/11] nvmet: Implement basic In-Band Authentication

[Simo Sorce <simo@redhat.com>]

On Tue, 2021-07-20 at 12:14 +0200, Hannes Reinecke wrote:
> On 7/19/21 1:52 PM, Stephan Mueller wrote:
> > Am Montag, dem 19.07.2021 um 13:10 +0200 schrieb Hannes Reinecke:
> > > On 7/19/21 12:19 PM, Stephan Mueller wrote:
> > > > Am Montag, dem 19.07.2021 um 11:57 +0200 schrieb Hannes Reinecke:
> > > > > On 7/19/21 10:51 AM, Stephan Mueller wrote:
> [ .. ]
> > > > > > 
> > > > > > Thank you for clarifying that. It sounds to me that there is no
> > > > > > defined protocol (or if there, I would be wondering how the code would have
> > > > > > worked
> > > > > > with a different implementation). Would it make sense to first specify
> > > > > > a protocol for authentication and have it discussed? I personally think
> > > > > > it is a bit difficult to fully understand the protocol from the code and
> > > > > > discuss protocol-level items based on the code.
> > > > > > 
> > > > > Oh, the protocol _is_ specified:
> > > > > 
> > > > >  
> > > > > https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-2_0-2021.06.02-Ratified-5.pdf
> > > > > 
> > > > > It's just that I have issues translating that spec onto what the kernel
> > > > > provides.
> > > > 
> > > > according to the naming conventions there in figures 447 and following:
> > > > 
> > > > - x and y: DH private key (kernel calls it secret set with dh_set_secret
> > > > or
> > > > encoded into param.key)
> > > > 
> > > 
> > > But that's were I got confused; one needs a private key here, but there
> > > is no obvious candidate for it. But reading it more closely I guess the
> > > private key is just a random number (cf the spec: g^y mod p, where y is
> > > a random number selected by the host that shall be at least 256 bits
> > > long). So I'll fix it up with the next round.
> > 
> > Here comes the crux: the kernel has an ECC private key generation function
> > ecdh_set_secret triggered with crypto_kpp_set_secret using a NULL key, but it
> > has no FFC-DH counterpart.
> > 
> > That said, generating a random number is the most obvious choice, but not the
> > right one.
> > 
> > The correct one would be following SP800-56A rev 3 and here either section
> > 5.6.1.1.3 or 5.6.1.1.4.
> > 
> Hmm. Okay. But after having read section 5.6.1.1.4, I still do have some
> questions.
> 
> Assume we will be using a bit length of 512 for FFDHE, then we will
> trivially pass Step 2 for all supported FFDHE groups (the maximum
> symmetric-equivalent strength for ffdhe8192 is 192 bits).

N = 512 is not a good choice, minimum length these days for DH should
be 2048 or more.

> From my understanding, the random number generator will fill out all
> available bytes in the string (and nothing more), so we trivially
> satisfy step 3 and 4.
> 
> And as q is always larger than the random number, step 6 reduces to
> 'if (c > 2^N - 2)',

Where is this coming from ?
It seem you assume M = 2^N but M = min(2^N, q)

The point here is to make sure the number X you return is:
0 < X < (q-1)

>  ie we just need to check if the random number is a
> string of 0xff characters. Which hardly is a random number at all, so
> it'll be impossible to get this.
> 
> Which then would mean that our 'x' is simply the random number + 1,

This is an artifact due to the random number being 0 <= c < 2^N - 1,
therefore 1 needs to be added to make sure you never return 0.

> which arguably is slightly pointless (one more than a random number is
> as random as the number itself), so I do feel justified with just
> returning a random number here.
> 
> Am I wrong with that reasoning?

Looks to me you are not accounting for the fact that N = 512 is too
small and a random number falling in the interval (q - 2) < X < 2^N is
unsuitable?

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc