netdev - Re: [PATCH v2] net: ipv4: move tcp_fastopen server side code to SipHash library

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jun 2019 11:42:40 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     netdev <netdev@...r.kernel.org>, linux-crypto@...r.kernel.org,
        Herbert Xu <herbert@...dor.apana.org.au>, ebiggers@...nel.org,
        David Miller <davem@...emloft.net>,
        Christoph Paasch <cpaasch@...le.com>,
        David Laight <David.Laight@...lab.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH v2] net: ipv4: move tcp_fastopen server side code to
 SipHash library



On 6/14/19 11:34 AM, Eric Dumazet wrote:
> On Fri, Jun 14, 2019 at 7:01 AM Ard Biesheuvel
> <ard.biesheuvel@...aro.org> wrote:
>>
>> Using a bare block cipher in non-crypto code is almost always a bad idea,
>> not only for security reasons (and we've seen some examples of this in
>> the kernel in the past), but also for performance reasons.
>>
>> In the TCP fastopen case, we call into the bare AES block cipher one or
>> two times (depending on whether the connection is IPv4 or IPv6). On most
>> systems, this results in a call chain such as
>>
>>   crypto_cipher_encrypt_one(ctx, dst, src)
>>     crypto_cipher_crt(tfm)->cit_encrypt_one(crypto_cipher_tfm(tfm), ...);
>>       aesni_encrypt
>>         kernel_fpu_begin();
>>         aesni_enc(ctx, dst, src); // asm routine
>>         kernel_fpu_end();
>>
>> It is highly unlikely that the use of special AES instructions has a
>> benefit in this case, especially since we are doing the above twice
>> for IPv6 connections, instead of using a transform which can process
>> the entire input in one go.
>>
>> We could switch to the cbcmac(aes) shash, which would at least get
>> rid of the duplicated overhead in *some* cases (i.e., today, only
>> arm64 has an accelerated implementation of cbcmac(aes), while x86 will
>> end up using the generic cbcmac template wrapping the AES-NI cipher,
>> which basically ends up doing exactly the above). However, in the given
>> context, it makes more sense to use a light-weight MAC algorithm that
>> is more suitable for the purpose at hand, such as SipHash.
>>
>> Since the output size of SipHash already matches our chosen value for
>> TCP_FASTOPEN_COOKIE_SIZE, and given that it accepts arbitrary input
>> sizes, this greatly simplifies the code as well.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> 
> While the patch looks fine (I yet have to run our tests with it), it
> might cause some deployment issues
> for server farms.
> 
> They usually share a common fastopen key, so that clients can reuse
> the same token for different sessions.
> 
> Changing some servers in the pool will lead to inconsistencies.

The inconsistencies coming from kernel version skew with some servers
being on the old hash and some on the newer one? Or is there another
source for the inconsistency you are referring to?

Thanks,

-Jason

> 
> Probably not a too big deal, but worth mentioning.
>