openssl icon indicating copy to clipboard operation
openssl copied to clipboard

"Error finalizing cipher loop" when running openssl speed -evp -decrypt for AES-GCM

Open ravulapx opened this issue 1 year ago • 9 comments

While running speed decrypt for aes-gcm in openssl3.2 getting Error finalizing cipher loop error.

image

Note: Similar issue seen in https://github.com/openssl/openssl/issues/23657 and they mentioned that fix was available in https://github.com/openssl/openssl/pull/23757 and it got merged into master branch. But still seeing an issue with master branch for aes-gcm

image

ravulapx avatar Jun 20 '24 12:06 ravulapx

@tom-cosgrove-arm can you comment on the fix you made in #23757? I'm having trouble seeing how a fake tag can be used with aes in ccm or gcm mode? I think it makes sense in aead mode because (again I think) the tag is validated independently of the decrypt operation, but in ccm mode the tag is never checked in the decrypt operation. Its only checked in gcm mode, and I can't see how an arbitrary tag will pass that check. It seems like instead for gcm mode, we should do an initial encryption of a buffer to obtain a tag and use that computed value consistently during the decrypt operation

nhorman avatar Jun 20 '24 18:06 nhorman

The original failure was due to there not being a tag, and the change I made did fix that. However, it looks rather embarrassingly as if I did the work on an out-of-date branch and then rebased without re-testing. I made a comment about "We don't check the return value" which used to be true, but which certainly isn't true these days.

I think the best fix would be something like this - what do you think?

diff --git a/apps/speed.c b/apps/speed.c
index 1fd7eb26b6..ca87302e22 100644
--- a/apps/speed.c
+++ b/apps/speed.c
@@ -882,10 +882,12 @@ static int EVP_Update_loop(void *args)
             }
         }
     }
-    if (decrypt)
-        rc = EVP_DecryptFinal_ex(ctx, buf, &outl);
-    else
+    if (decrypt) {
+        (void)EVP_DecryptFinal_ex(ctx, buf, &outl);
+        rc = 1; /* DecryptFinal will fail because the tag won't verify */
+    } else {
         rc = EVP_EncryptFinal_ex(ctx, buf, &outl);
+    }
 
     if (rc == 0)
         BIO_printf(bio_err, "Error finalizing cipher loop\n");

tom-cosgrove-arm avatar Jun 21 '24 08:06 tom-cosgrove-arm

It seems like instead for gcm mode, we should do an initial encryption of a buffer to obtain a tag and use that computed value consistently during the decrypt operation

That would definitely be the best option

tom-cosgrove-arm avatar Jun 21 '24 08:06 tom-cosgrove-arm

@ravulapx it seems like the consensus here is that the appropriate fix is, for gcm mode speed tests:

  • [ ] Modify the speed_main function such that, when loopfunction is set to EVP_Update_loop, and -decrypt is selected, that we preform an initial encryption of a data buffer, and extract the tag from that result
  • [ ] Modify EVP_Update_loop such that if a tag is assigned in speed_main, we use that as the input tag for decryption

that should avoid this problem. Unfortunately I don't know when we are going to be able to get to this. Would you be willing to create a PR for this effort?

nhorman avatar Jun 21 '24 14:06 nhorman

@tom-cosgrove-arm your solution would certainly work. I guess the question is "is it ok for us to test algorithm speed on failing inputs"? If so, that would be a faster solution

nhorman avatar Jun 23 '24 00:06 nhorman

@nhorman In general when I've written tests like this for other systems, I've gone in the direction you state (perform a genuine encryption and use that to test decryption performance). However, OpenSSL hasn't done that, so we are purely testing performance here, not correctness (which is why my original fix here, that didn't quite fix it). Since the algorithms we are testing always perform the decryption and tag computations, and only diverge in behaviour when checking the tag at the very last step, performance is still representative. We might want a note to that effect, either as a comment in the code or "elsewhere" (but I not sure where that else might be!)

tom-cosgrove-arm avatar Jun 23 '24 21:06 tom-cosgrove-arm

@ravulapx are you interested in attempting to write a patch for this and submit it as a pull request, guided by the conversation above?

nhorman avatar Jun 24 '24 11:06 nhorman

@ravulapx are you interested in attempting to write a patch for this and submit it as a pull request, guided by the conversation above?

Currently held up with other priorities not much of bandwidth for now

ravulapx avatar Aug 26 '24 06:08 ravulapx

Same happens with chacha20-poly1305:

$ openssl speed -evp chacha20-poly1305 -decrypt 
Doing ChaCha20-Poly1305 ops for 3s on 16 size blocks: Error finalizing cipher loop
78517825 ChaCha20-Poly1305 ops in 2.99s
Doing ChaCha20-Poly1305 ops for 3s on 64 size blocks: Error finalizing cipher loop
33011569 ChaCha20-Poly1305 ops in 3.00s
Doing ChaCha20-Poly1305 ops for 3s on 256 size blocks: Error finalizing cipher loop
27759496 ChaCha20-Poly1305 ops in 2.98s
Doing ChaCha20-Poly1305 ops for 3s on 1024 size blocks: Error finalizing cipher loop
13005016 ChaCha20-Poly1305 ops in 3.00s
Doing ChaCha20-Poly1305 ops for 3s on 8192 size blocks: Error finalizing cipher loop
1807409 ChaCha20-Poly1305 ops in 3.00s
Doing ChaCha20-Poly1305 ops for 3s on 16384 size blocks: Error finalizing cipher loop
917205 ChaCha20-Poly1305 ops in 2.99s
version: 3.3.2
built on: Tue Sep  3 16:32:59 2024 UTC
options: bn(64,64)
compiler: gcc -fPIC -pthread -m64 -Wa,--noexecstack -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions         -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security         -fstack-clash-protection -fcf-protection         -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g -ffile-prefix-map=/build/openssl/src=/usr/src/debug/openssl -flto=auto -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_BUILDING_OPENSSL -DNDEBUG
CPUINFO: OPENSSL_ia32cap=0x7ef8320b078bffff:0x405fdef1bf97a9
The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
ChaCha20-Poly1305   420162.27k   704246.81k  2384708.38k  4439045.46k  4935431.51k  5025915.29k

marek22k avatar Oct 16 '24 16:10 marek22k

Hi @nhorman @tom-cosgrove-arm @ravulapx, I opened a PR to get CCM and GCM through decryption successfully. As far as I believe, CCM, GCM, and OCB require a valid tag which is only computed after encrypting a message. For this reason, I made some modifications to the code in speed.c to encrypt the data first, get the tag, and then use it to decrypt. I haven't taken chacha into account but I tested both encryption and decryption, with and without AAD, using CCM, GCM, OCB, SIV, and GCM-SIV. The PR hasn't been merged yet, so let's see how that goes.

zelda923 avatar Oct 31 '24 13:10 zelda923

Fixed on all the branches thanks to @zelda923

t8m avatar Nov 13 '24 16:11 t8m