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

[Hannes Reinecke <hare@suse.de>]

On 7/20/21 12:49 PM, Simo Sorce wrote:
> 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.
> 

According to RFC7919:
Peers using ffdhe8192 that want to optimize their key exchange with a
short exponent (Section 5.2) should choose a secret key of at least
400 bits.

So what is wrong with 512 bits?

>> 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)
> 

Which is what I've tried to argue. For 512 bits private key and the
smallest possible FFDHE group (which has 2048 bits, with the top bit
non-zero) 2^N is always smaller than (q - 1).
As the other FFHDE groups are using even larger 'q' values, this is true
for all FFHDE groups.

>>  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.
> 

And my argument here is that all zeros (and all ones) are not a value I
would expect from our RNG.

>> 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?
> 

Only if (q - 2) < 2^N. And my point is that it's not.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer