Hello,
Upgrading nettle from 3.6 to 3.7 triggers a GnuTLS 3.7.0 testsuite error on both ppc64 and ppc64el:
(sid_ppc64el-dchroot)ametzler@plummer:~/GNUTLS/gnutls28-3.7.0/b4deb/tests$ ./min i-record-2 testing aes-cbc testing aes-cbc-sha256 testing aes-gcm testing aes-ccm testing aes-ccm-8 testing null-sha1 testing arcfour-sha1 testing arcfour-md5 testing chacha20-poly1305 testing tls13-chacha20-poly1305 server:330: client: Error: An unexpected TLS packet was received.
Running the same binary dynamically (with LD_LIBRARY_PATH setting) linked against nettle 3.6 succeeds.
--verbose logs are huge (5-7 MB xz-compressed), I have uploaded them to https://people.debian.org/~ametzler/tmp/
cu Andreas
On 2021-02-09 Andreas Metzler ametzler@bebt.de wrote:
Hello,
Upgrading nettle from 3.6 to 3.7 triggers a GnuTLS 3.7.0 testsuite error on both ppc64 and ppc64el:
(sid_ppc64el-dchroot)ametzler@plummer:~/GNUTLS/gnutls28-3.7.0/b4deb/tests$ ./min
[...]
testing chacha20-poly1305 testing tls13-chacha20-poly1305 server:330: client: Error: An unexpected TLS packet was received.
Running the same binary dynamically (with LD_LIBRARY_PATH setting) linked against nettle 3.6 succeeds.
--verbose logs are huge (5-7 MB xz-compressed), I have uploaded them to https://people.debian.org/~ametzler/tmp/
Hello,
I have bisected this[1] in nettle git and found
58a0301437e9beb23130423ff1063a67b6f2b43b ppc: New assembly for chacha_core4, doing four blocks in parallel.
as first bad commit, the previous one (58c55046beda976b10ac3ce930696d172e5e5038 Fix a ChangeLog typo.) works.
cu Andreas
[1] twice :-( First try found d56503602134442832e73bc40a657954d3f8db8f - "Enable fat build by default." so I did a second run with --enable-fat.
Andreas Metzler ametzler@bebt.de writes:
I have bisected this[1] in nettle git and found
58a0301437e9beb23130423ff1063a67b6f2b43b ppc: New assembly for chacha_core4, doing four blocks in parallel.
This is indeed new code in nettle-3.7, and particularly suspect since the test fails only on ppc. Do you know what the code path is? Is GnuTLS using Nettle's chacha_poly1305_* functions, or is it calling chacha and poly1305 functions separately?
Regards, /Niels
On 2021-02-09 Niels Möller nisse@lysator.liu.se wrote:
Andreas Metzler ametzler@bebt.de writes:
I have bisected this[1] in nettle git and found
58a0301437e9beb23130423ff1063a67b6f2b43b ppc: New assembly for chacha_core4, doing four blocks in parallel.
This is indeed new code in nettle-3.7, and particularly suspect since the test fails only on ppc. Do you know what the code path is? Is GnuTLS using Nettle's chacha_poly1305_* functions, or is it calling chacha and poly1305 functions separately?
Hello,
Afaict from https://gitlab.com/gnutls/gnutls/-/blob/master/lib/nettle/cipher.c#L815 it does use chacha_poly1305_encrypt/decrypt/update/digest/set_key/set_nonce.
I am not 100% sure. - I thought I could brute-force this with ltrace, but I only got it to show direct library calls to gnutls_* but not the indirect ones (gnutls calling nettle).
cu Andreas
Andreas Metzler ametzler@bebt.de writes:
Afaict from https://gitlab.com/gnutls/gnutls/-/blob/master/lib/nettle/cipher.c#L815 it does use chacha_poly1305_encrypt/decrypt/update/digest/set_key/set_nonce.
I see. (But closer to line 857). I wonder what the precise message size was. Log says
server|<5>| REC[0x100372bc820]: Received Packet Application Data(23) with length: 209 server|<10>| READ: Got 209 bytes from 0x3 server|<10>| READ: read 209 bytes from 0x3 server|<10>| RB: Have 5 bytes into buffer. Adding 209 bytes. server|<10>| RB: Requested 214 bytes server|<5>| REC[0x100372bc820]: Decrypted Packet[191] Unknown Packet(196) with length: 192 server|<5>| REC[0x100372bc820]: Received unexpected packet 196 (Unknown Packet) expecting 23 (Application Data)
I would guess that means that we got 209 bytes, including the 16-byte poly1305 authentication tag. Message size is then 209 - 16 = 193 bytes. If the first byte is a TLS packet type, the "length: 192" in the next to last line makes sense if the packet type byte is excluded. Right?
And preceding packets, which are smaller (packet size increasing one byte per packet), appear to be decrypted correctly. I'll investigate.
What is the source of the incoming packets? GnuTLS of the same version, also using Nettle-3.7, or the previous version, or some prerecorded data? It's not obvious to me if the error is on the sender or the receiver side.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
I would guess that means that we got 209 bytes, including the 16-byte poly1305 authentication tag. Message size is then 209 - 16 = 193 bytes. If the first byte is a TLS packet type, the "length: 192" in the next to last line makes sense if the packet type byte is excluded. Right?
I've found one problem, although I don't see that it would cause precisely the reported problem. It would result in incorrect encrypt/decrypt of the data immediately after a call to chacha_crypt or chacha_crypt32 with 129 <= (length % 256) <= 192. In code used only on ppc64 with the new altivec chacha code enabled.
Tentative patch below, but I need to extend the tests to get proper test coverage of this case.
Regards, /Niels
diff --git a/chacha-crypt.c b/chacha-crypt.c index 081ebcf4..9db13183 100644 --- a/chacha-crypt.c +++ b/chacha-crypt.c @@ -80,13 +80,16 @@ _nettle_chacha_crypt_4core(struct chacha_ctx *ctx, while (length > 2*CHACHA_BLOCK_SIZE) { _nettle_chacha_4core (x, ctx->state, CHACHA_ROUNDS); - ctx->state[12] += 4; - ctx->state[13] += (ctx->state[12] < 4); if (length <= 4*CHACHA_BLOCK_SIZE) { + uint32_t blocks = 3 + (length > 3*CHACHA_BLOCK_SIZE); + ctx->state[12] += blocks; + ctx->state[13] += (ctx->state[12] < blocks); memxor3 (dst, src, x, length); return; } + ctx->state[12] += 4; + ctx->state[13] += (ctx->state[12] < 4); memxor3 (dst, src, x, 4*CHACHA_BLOCK_SIZE);
length -= 4*CHACHA_BLOCK_SIZE; @@ -200,12 +203,13 @@ _nettle_chacha_crypt32_4core(struct chacha_ctx *ctx, while (length > 2*CHACHA_BLOCK_SIZE) { _nettle_chacha_4core32 (x, ctx->state, CHACHA_ROUNDS); - ctx->state[12] += 4; if (length <= 4*CHACHA_BLOCK_SIZE) { + ctx->state[12] += 3 + (length > 3*CHACHA_BLOCK_SIZE); memxor3 (dst, src, x, length); return; } + ctx->state[12] += 4; memxor3 (dst, src, x, 4*CHACHA_BLOCK_SIZE);
length -= 4*CHACHA_BLOCK_SIZE;
On Tue, Feb 9, 2021 at 3:07 PM Niels Möller nisse@lysator.liu.se wrote:
nisse@lysator.liu.se (Niels Möller) writes:
I would guess that means that we got 209 bytes, including the 16-byte poly1305 authentication tag. Message size is then 209 - 16 = 193 bytes. If the first byte is a TLS packet type, the "length: 192" in the next to last line makes sense if the packet type byte is excluded. Right?
I've found one problem, although I don't see that it would cause precisely the reported problem. It would result in incorrect encrypt/decrypt of the data immediately after a call to chacha_crypt or chacha_crypt32 with 129 <= (length % 256) <= 192. In code used only on ppc64 with the new altivec chacha code enabled.
Tentative patch below, but I need to extend the tests to get proper test coverage of this case.
If needed, you can find Bernstein's reference implementation at https://cr.yp.to/chacha.html. Modify it for IETF's ChaCha and use it to generate test vectors. A set of 12x blocks would probably be a good choice.
Or you can use the test vectors from Wei Dai's Crypto++. The project already generated test vectors for 1x, 4x and 12x blocks. The test vectors include Bernstein's ChaCha and the IETF version. Also see https://github.com/weidai11/cryptopp/blob/master/TestVectors/chacha.txt .
You might also consider changing the project's governance to require a complete set of test vectors for each algorithm. If you are doing 4x blocks, then you need test vectors covering them. You should also use an independent program to generate them, like Bernstein's reference implementation. (I don't believe the IETF provides a reference implementation).
Jeff
Jeffrey Walton noloader@gmail.com writes:
Or you can use the test vectors from Wei Dai's Crypto++. The project already generated test vectors for 1x, 4x and 12x blocks. The test vectors include Bernstein's ChaCha and the IETF version. Also see https://github.com/weidai11/cryptopp/blob/master/TestVectors/chacha.txt
Thanks, I've copied one of the 1024 byte test vectors from there.
You might also consider changing the project's governance to require a complete set of test vectors for each algorithm. If you are doing 4x blocks, then you need test vectors covering them. You should also use an independent program to generate them, like Bernstein's reference implementation. (I don't believe the IETF provides a reference implementation).
In this case, the coverage problem wasn't mainly lack of authoritative test vectors, but missing coverage for sequences of calls to chacha_crypt/chacha_crypt32. The bug was in the counter update at the very end of the processing, for certain data sizes, and would not cause obviously incorrect results until the next call.
For tests that vary things like alignment, message size, how to split a message into multiple calls, etc, I think it's usually good enough to check that the result always is identical to the simplest way to do it (say, using a single call for the complete message, friendly alignment, and without involving any assembly code). I think of that kind of tests as mostly orthogonal to tests using authoritative test vectors.
I've pushed test updates to the branch fix-chacha-counter, and ci builds now fail on ppc64. The fix posted to the list appears to work, I'll push that to the branch in a moment.
Regards, /Niels
Hello,
nisse@lysator.liu.se (Niels Möller) writes:
nisse@lysator.liu.se (Niels Möller) writes:
I would guess that means that we got 209 bytes, including the 16-byte poly1305 authentication tag. Message size is then 209 - 16 = 193 bytes. If the first byte is a TLS packet type, the "length: 192" in the next to last line makes sense if the packet type byte is excluded. Right?
That's exactly the case (under TLS 1.3). As the protocol uses separate keys for handshake and application traffic (and the error happens in sending application data and no post-handshake messages are exchanged), I guess you can also assume that the same data (all bytes are 0x2) with different lengths is fed into chacha_crypt* in the failing test case (mini-record-2).
I've found one problem, although I don't see that it would cause precisely the reported problem. It would result in incorrect encrypt/decrypt of the data immediately after a call to chacha_crypt or chacha_crypt32 with 129 <= (length % 256) <= 192. In code used only on ppc64 with the new altivec chacha code enabled.
Tentative patch below, but I need to extend the tests to get proper test coverage of this case.
Thank you so much! The patch fixes the issue (tested on gcc cfarm).
In the gdb trace, I see nettle_chacha_poly1305_encrypt() is called with the following length pattern: 128, 63, 128, 64, 192, 1, 192, 2. I can try to create a test case if necessary.
Also: thank you Andreas for looking into it.
Regards,
Daiki Ueno ueno@gnu.org writes:
Thank you so much! The patch fixes the issue (tested on gcc cfarm).
Thanks for testing. Pushed to master branch now. BTW, I could test ppc64el locally on my laptop fairly easily, I used:
# apt-get install -t testing gcc-powerpc64le-linux-gnu # dpkg --add-architecture ppc64el # apt-get update # apt-get install libc6:ppc64el
(I already had qemu-user and binfmt magic installed)
$ ~/hack/nettle/configure --host=powerpc64le-linux-gnu --enable-mini-gmp CXX=/bin/false $ make -j10 && make -j10 check
In the gdb trace, I see nettle_chacha_poly1305_encrypt() is called with the following length pattern: 128, 63, 128, 64, 192, 1, 192, 2. I can try to create a test case if necessary.
I see. And then it's the first call with length 192 that updates the counter value incorrectly (incrementing it by 4 instead of 3), with incorrect encryption on the next call. No calls with length 129, which would be the smallest one to trigger the bug.
You can have a look at the updated test and see if you think an additional test would be worthwhile. The loop testing various lengths start at https://git.lysator.liu.se/nettle/nettle/-/blob/master/testsuite/chacha-test..., and the code from line 219 and on is new.
Regards, /Niels
On 2021-02-10 Daiki Ueno ueno@gnu.org wrote:
nisse@lysator.liu.se (Niels Möller) writes:
[...]
Tentative patch below, but I need to extend the tests to get proper test coverage of this case.
Thank you so much! The patch fixes the issue (tested on gcc cfarm).
Hello,
Just for completeness sake I can confirm that nettle Git master 64837b2e433e2b99b893683949bad3a99acab38f lets gnutls testsuite succeed on Debian's ppc64el porter machine.
Thank you very much for fixing this so quickly.
cu Andreas
nettle-bugs@lists.lysator.liu.se
-
Andreas Metzler -
Daiki Ueno -
Jeffrey Walton -
nisse＠lysator.liu.se