[openssl.org #3602] [PATCH]
Error codes aren't part of the API. It's a bit of a grey area in some cases, but for EVP_DecryptFinal_ex, you really should be checking the return value and not relying on errors left on stack. In particular, reporting detailed decryption errors was a historical mistake that has led to serious attacks; it's a coding pattern we should eradicate. This particular change was made to avoid a timing leak and provide callers an opportunity to proceed in constant time. (But most callers won't care, it only affects mac-then-encrypt.) In your case, it sounds like the right thing to do is fix the unittest to be more robust. I'm going to reject this for now; I'll revisit if we get reports that this is causing more widespread problems, though. Cheers, Emilia __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl.org #3602] [PATCH]
Dear all,
I would like to submit a patch to the current sources of openssl. This
patch is fixing a missing error code in the EVP_DecryptFinal_ex function.
During the latest Debian Bug Squashing Party i was working on NodeJS
packaging and trying to fix an issue. I noticed a unit test failure (on
NodeJS side) because of an unexpected openssl return value.
Unit test is simple/test-crypto-stream, and is based on aes-128-cbc
encryption and decryption with two different keys. This test should fail
with the error code :
[TypeError: error:06065064:digital envelope
routines:EVP_DecryptFinal_ex:bad decrypt]
But the latest stable version returns
[TypeError: error::lib(0):func(0):reason(0)]
This seems to come from some modification made in the
EVP_DecryptFinal_ex function. When returning padding_good, the EVPerr is
not called before returning zero, leading to an undefined error code.
Here attached is a patch fixing this.
I hope this will help, don't hesitate to ask me for more information
Kind regards,
--
William http://www.wbonnet.net
http://france.debian.netAssociation Debian France
http://www.opencsw.org Community SoftWare for Solaris
diff -ur openssl.orig/crypto/evp/evp_enc.c openssl.git/crypto/evp/evp_enc.c
--- openssl.orig/crypto/evp/evp_enc.c 2014-11-16 13:09:34.408545924 +0100
+++ openssl.git/crypto/evp/evp_enc.c 2014-11-16 13:08:21.055224344 +0100
@@ -546,6 +546,16 @@
out[i] = ctx-final[i] padding_good;
/* Safe cast: for a good padding, EVP_MAX_IV_LENGTH = b = pad */
*outl = padding_good ((unsigned char)(b - pad));
+
+ /*
+ * If the padding_good variable is 0 then a decryption problem occured
+ * and we have to call EVPerr before returning 0
+ */
+ if ((padding_good 1) == 0)
+ {
+EVPerr(EVP_F_EVP_DECRYPTFINAL_EX, EVP_R_BAD_DECRYPT);
+ }
+
return padding_good 1;
}
else
[openssl.org #3602] [PATCH]
Unfortunately I don't think it is as simple as that. If I understand the previous change correctly, Emilia has deliberately removed the error message as part of work to protect against timing attacks. The very act of adding an error to the error queue could introduce a measurable timing difference which (theorectically) could be exploited. Matt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3602] [PATCH]
On Sun, Nov 16, 2014 at 09:11:42PM +0100, Matt Caswell via RT wrote: Unfortunately I don't think it is as simple as that. If I understand the previous change correctly, Emilia has deliberately removed the error message as part of work to protect against timing attacks. The very act of adding an error to the error queue could introduce a measurable timing difference which (theorectically) could be exploited. I think we need to clarify the documentation about what we expect people to do when things like a padding error are detected. They too need to respond in a way that doesn't leak any information like time or which error was detected, or even that an error was detected. Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3602] [PATCH]
On Sun, Nov 16, 2014 at 09:11:42PM +0100, Matt Caswell via RT wrote: Unfortunately I don't think it is as simple as that. If I understand the previous change correctly, Emilia has deliberately removed the error message as part of work to protect against timing attacks. The very act of adding an error to the error queue could introduce a measurable timing difference which (theorectically) could be exploited. I think we need to clarify the documentation about what we expect people to do when things like a padding error are detected. They too need to respond in a way that doesn't leak any information like time or which error was detected, or even that an error was detected. Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3602] [PATCH]
Hi Unfortunately I don't think it is as simple as that. If I understand the previous change correctly, Emilia has deliberately removed the error message as part of work to protect against timing attacks. The very act of adding an error to the error queue could introduce a measurable timing difference which (theorectically) could be exploited. I think we need to clarify the documentation about what we expect people to do when things like a padding error are detected. They too need to respond in a way that doesn't leak any information like time or which error was detected, or even that an error was detected. This modification had some impact on softwares expecting to have an error code (in this case 06065064 but got only zeros). Of course it can be modified on the software side, but it is quite surprising to move to a fully undefined error code. My use case (actually node.js use case) was not directly a padding error, but a unit test checking the case of encrypting and decrypting with different keys. Having no Bad Decrypt error code is quite misleading. Since if this function fails under different circumstances (final block length error, etc.), information about the reason of the failure are given to the caller it was surprising for a non crypto guy to have no error code in this specific case. Having return code A or return code B seems the same since this error code (A vs B, or 06065064 vs ) can happen in only one case in this function. An error detected after the padding check loop. On the other hand there is the timing argument. I fully understand that pushing the error code before returning zero will slightly change the duration of the function. I was just hoping that impact would be negligible since it is outside of the for loop (which will have a constant execution time). I was certainly wrong and forgetting to take things in consideration, but if not I would prefer to have an error code I can use in my software :) Thus a last question... Would it be possible to have a constant time way to end this function by pushing or not an error message in queues ? I mean something like calling EVPerr anytime, pushing or not an actual error code. So it could return zero or not, having an error code in the queue if needed. And execution time would always be the same. I think this would help solving this issue, and this could help on situation like having a method doing something like : 1. call EVP_DecryptFinal_ex 2. check if return code is zero 3. push an error code to the queue 4. and return to its caller Such case would certainly lead to the same security risk as pushing the error from within EVP_DecryptFinal_ex method, right ? And it looks like it happens at least in function PEM_do_header (pem_lib.c at line 476 and following). Should this be removed also ? Kind regards, -- William http://www.wbonnet.net http://france.debian.netAssociation Debian France http://www.opencsw.org Community SoftWare for Solaris __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
