RE: [PATCH] PURIFY and valgrind
re: http://www.mail-archive.com/openssl-dev@openssl.org/msg24270.html I don't quite approve of the established openssl tradition of using uninitialized memory for entropy, but I wanted to point out that if you want to do that, and you want valgrind to understand that those bits count as valid and should not trigger warnings when your control flow branches based on those bits, or when you use those bits are arguments to a system call, then you can just call VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE, as described in the valgrind manual on this page: [1]. A nice explanation of what this means in valgrind's elegant error detection scheme is on this page: [2]. I haven't tried this myself, but if it works as advertised then it allows openssl to continue functioning the same way when in valgrind mode, and completely suppresses the false alarms without suppressing any other weirdness that might show up. Regards, Zooko [1] http://valgrind.org/docs/manual/mc-manual.html#mc-manual.clientreqs [2] http://valgrind.org/docs/manual/mc-manual.html#mc-manual.machine __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
Re: [PATCH] PURIFY and valgrind, 2nd round
Kyle Hamilton wrote: Debian c suffered from simply removing all calls to seed the random number generator with enough entropy to make it secure. When it comes to entropy, every little bit helps. The calls to add uninitialized static variable locations are never relied upon to seed the PRNG with enough entropy, but they are used to stir the pot just a little bit more. This is probably one of the most-frequently asked questions. http://openssl.org/support/faq.html#PROG14 (I think the 'answer' for that needs to be changed to include some of the prior discussion on the topic...) Beware, the current situation is that enabling -DPURIFY no longer makes openssl valgrind free as it used to be. The patch just makes it work again, nothing more, nothing less. Best Regards, Frederic Heem. This is probably also one of the most-frequently answered questions. Look in the openssl-users and openssl-dev archives for the past year and you'll see people with much more knowledge of entropy than me discussing this topic, at length. -Kyle H On Fri, Jul 18, 2008 at 9:47 AM, Frederic Heem [EMAIL PROTECTED] wrote: Hi, The previous patch didn't fully work due a mysterious valgrind issue (something related to loading libssl multiple time through dl_open). This patch is simply what has Robert suggested. By the way, can someone explain me why some uninitialized static variables are used create a random number ? At least, on some embedded system, static variables are initialized to 0, therefore, can the keys be weak on these system ? I just want to avoid the security hole DebianCo have suffered recently. Best Regards, Frederic Heem __ --- NOTICE --- This email and any attachments are confidential and are intended for the addressee only. If you have received this message by mistake, please contact us immediately and then delete the message from your system. You must not copy, distribute, disclose or act upon the contents of this email. Personal and corporate data submitted will be used in a correct, transparent and lawful manner. The data collected will be processed in paper or computerized form for the performance of contractual and lawful obligations as well as for the effective management of business relationship. The data processor is Telsey S.p.A. The data subject may exercise all the rights set forth in art. 7 of Law by Decree 30.06.2003 n. 196 as reported in the following url http://www.telsey.com/privacy.asp. __ 798t8RfNa6Dl8Ilf __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED] -- --- Frederic Heem - Innovation Hub Senior Engineer Telsey Telecommunications S.p.A. email: [EMAIL PROTECTED] Departement: Innovation Hub UK: Vanguard Centre, ATU II, Unit 8Sir William Lyons Road University of Warwick Science ParkCV4 7EZ Coventry Tel. direct phone (+39) 0422 377760 Central Bureau Viale dell'Industria, 1 31055 Quinto di Treviso (TV) - Italy Tel.: +39 0422470840 Fax: +39 0422470920 Web: www.telsey.com __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
[PATCH] PURIFY and valgrind
Dear,
Please find attached a patch which makes valgrind and friends happy.
Some changes had been done in md_rand.c which broke the purpose of
PURIFY. Needless to say that the define PURIFY is *not* for production
system...
Best Regards,
Frederic Heem
__
--- NOTICE ---
This email and any attachments are confidential and are intended for the
addressee only. If you have received this message by mistake, please contact
us immediately and then delete the message from your system. You must not
copy, distribute, disclose or act upon the contents of this email. Personal
and corporate data submitted will be used in a correct, transparent and lawful
manner. The data collected will be processed in paper or computerized form for
the performance of contractual and lawful obligations as well as for the
effective management of business relationship. The data processor is Telsey
S.p.A. The data subject may exercise all the rights set forth in art. 7 of
Law by Decree 30.06.2003 n. 196 as reported in the following url
http://www.telsey.com/privacy.asp.
__
798t8RfNa6Dl8Ilf
--- ../../../tmp/openssl-0.9.8h/crypto/rand/md_rand.c 2007-01-21 13:16:36.0 +
+++ md_rand.c 2008-07-17 17:53:01.0 +0100
@@ -331,7 +331,27 @@
pid_t curr_pid = getpid();
#endif
int do_stir_pool = 0;
-
+#ifdef PURIFY
+ /* DO NOT USE PURIFY FOR PRODUCTION SYSTEM
+ this makes valgrind and friends happy.
+ Numbers below doesn't follow any particular rule, they are just not equal to 0 */
+ static int once = 0;
+ if(once == 0){
+once = 1;
+md_count[0] = 3;
+md_count[0] = 7;
+for(i = 0; i MD_DIGEST_LENGTH; i++){
+ md[i] = i * i + 1;
+}
+for(i = 0; i num; i++){
+ buf[i] = i * i + 2;
+}
+for(i = 0; i (STATE_SIZE + MD_DIGEST_LENGTH); i++){
+ state[i] = i * i + 3;
+}
+ }
+ memset(buf, 1, num);
+#endif
#ifdef PREDICT
if (rand_predictable)
{
@@ -464,9 +484,7 @@
#endif
MD_Update(m,local_md,MD_DIGEST_LENGTH);
MD_Update(m,(unsigned char *)(md_c[0]),sizeof(md_c));
-#ifndef PURIFY
MD_Update(m,buf,j); /* purify complains */
-#endif
k=(st_idx+MD_DIGEST_LENGTH/2)-st_num;
if (k 0)
{
Re: [PATCH] PURIFY and valgrind
Leave everything as all zero's; that will make it real obvious not to use this in production code. #ifdef PURIFY memset(buf, 0, num); memset(md_c, 0, sizeof md_c); memset(local_md, 0, sizeof local_md); #endif -- STSM, DataPower Chief Programmer WebSphere DataPower SOA Appliances http://www.ibm.com/software/integration/datapower/ __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
Re: [PATCH] PURIFY and valgrind
On Thu, Jul 17, 2008 at 7:07 PM, Frederic Heem [EMAIL PROTECTED] wrote: Please find attached a patch which makes valgrind and friends happy. Some changes had been done in md_rand.c which broke the purpose of PURIFY. Needless to say that the define PURIFY is *not* for production system... Defining PURIFY should never make the PRNG weak. If Valgrind finds data that is used uninitialized, then a PURIFY patch should only ensure that those exact bytes of data are initialized with some data. Never overwrite a byte that actually may have been initialized. Bodo __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
Re: [PATCH] PURIFY and valgrind
On Friday 18 July 2008 10:57:50 Bodo Moeller wrote: On Thu, Jul 17, 2008 at 7:07 PM, Frederic Heem [EMAIL PROTECTED] wrote: Please find attached a patch which makes valgrind and friends happy. Some changes had been done in md_rand.c which broke the purpose of PURIFY. Needless to say that the define PURIFY is *not* for production system... Defining PURIFY should never make the PRNG weak. If Valgrind finds data that is used uninitialized, then a PURIFY patch should only ensure that those exact bytes of data are initialized with some data. Never overwrite a byte that actually may have been initialized. Agreed, though where possible it's preferable for PURIFY-handling to simply not use the uninitialised data at all, rather than initialising it before use. (NB, I know this yields the same quality result, but appearances in the code are often as important as the outcome of the executable - particularly where package maintainers are concerned...) IIRC, it's complicated to remove the (uninitialised) loads here without also removing the desired stores, in which case I totally agree that memset(,0,) is a more obvious PURIFY(/valgrind) workaround. But where it's possible to trim our use of an input buffer so that we don't take some uninitialised trimmings, then the intent of the code would be much clearer. Moreover, it's more likely valgrind would then find bugs in the PRNG code (if ever we accidently debianise it ourselves) - whereas wholesale memset()s kind of limit the effectiveness of such debugging tools. Or put another way, -1 to the suggested patch. Also, -1 to the general statement that PURIFY is *not* for production system. That is not our call to make! If the production system is experiencing problems and is being diagnosed - it may very well make sense to run it under PURIFY/valgrind to help track down problems that have only been observed in production. (Anyone who has ever tried to gaze into a crystal ball and invent a test-case to reproduce sporadic problems from a production system will know what I'm talking about). Cheers, Geoff -- Un terrien, c'est un singe avec des clefs de char... __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
Re: [PATCH] PURIFY and valgrind
On Fri, Jul 18, 2008 at 6:00 PM, Geoff Thorpe [EMAIL PROTECTED] wrote: On Friday 18 July 2008 10:57:50 Bodo Moeller wrote: On Thu, Jul 17, 2008 at 7:07 PM, Frederic Heem [EMAIL PROTECTED] wrote: Please find attached a patch which makes valgrind and friends happy. Some changes had been done in md_rand.c which broke the purpose of PURIFY. Needless to say that the define PURIFY is *not* for production system... Defining PURIFY should never make the PRNG weak. If Valgrind finds data that is used uninitialized, then a PURIFY patch should only ensure that those exact bytes of data are initialized with some data. Never overwrite a byte that actually may have been initialized. Agreed, though where possible it's preferable for PURIFY-handling to simply not use the uninitialised data at all, rather than initialising it before use. Absolutely true! Thanks for adding this aspect to the picture. Bodo __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
[PATCH] PURIFY and valgrind, 2nd round
Hi,
The previous patch didn't fully work due a mysterious valgrind issue
(something related to loading libssl multiple time through dl_open).
This patch is simply what has Robert suggested.
By the way, can someone explain me why some uninitialized static
variables are used create a random number ? At least, on some embedded
system, static variables are initialized to 0, therefore, can the keys
be weak on these system ? I just want to avoid the security hole
DebianCo have suffered recently.
Best Regards,
Frederic Heem
__
--- NOTICE ---
This email and any attachments are confidential and are intended for the
addressee only. If you have received this message by mistake, please contact
us immediately and then delete the message from your system. You must not
copy, distribute, disclose or act upon the contents of this email. Personal
and corporate data submitted will be used in a correct, transparent and lawful
manner. The data collected will be processed in paper or computerized form for
the performance of contractual and lawful obligations as well as for the
effective management of business relationship. The data processor is Telsey
S.p.A. The data subject may exercise all the rights set forth in art. 7 of
Law by Decree 30.06.2003 n. 196 as reported in the following url
http://www.telsey.com/privacy.asp.
__
798t8RfNa6Dl8Ilf
--- openssl-0.9.8h-old/crypto/rand/md_rand.c 2007-01-21 13:16:36.0 +
+++ openssl-0.9.8h/crypto/rand/md_rand.c 2008-07-18 16:34:18.0 +0100
@@ -332,6 +332,13 @@
#endif
int do_stir_pool = 0;
+#ifdef PURIFY
+ /* DO NOT USE PURIFY FOR PRODUCTION SYSTEM
+ this makes valgrind and friends happy. */
+ memset(state, 0, STATE_SIZE + MD_DIGEST_LENGTH);
+ memset(md, 0, MD_DIGEST_LENGTH);
+ memset(buf, 0, num);
+#endif
#ifdef PREDICT
if (rand_predictable)
{
Re: [PATCH] PURIFY and valgrind, 2nd round
Debian c suffered from simply removing all calls to seed the random number generator with enough entropy to make it secure. When it comes to entropy, every little bit helps. The calls to add uninitialized static variable locations are never relied upon to seed the PRNG with enough entropy, but they are used to stir the pot just a little bit more. This is probably one of the most-frequently asked questions. http://openssl.org/support/faq.html#PROG14 (I think the 'answer' for that needs to be changed to include some of the prior discussion on the topic...) This is probably also one of the most-frequently answered questions. Look in the openssl-users and openssl-dev archives for the past year and you'll see people with much more knowledge of entropy than me discussing this topic, at length. -Kyle H On Fri, Jul 18, 2008 at 9:47 AM, Frederic Heem [EMAIL PROTECTED] wrote: Hi, The previous patch didn't fully work due a mysterious valgrind issue (something related to loading libssl multiple time through dl_open). This patch is simply what has Robert suggested. By the way, can someone explain me why some uninitialized static variables are used create a random number ? At least, on some embedded system, static variables are initialized to 0, therefore, can the keys be weak on these system ? I just want to avoid the security hole DebianCo have suffered recently. Best Regards, Frederic Heem __ --- NOTICE --- This email and any attachments are confidential and are intended for the addressee only. If you have received this message by mistake, please contact us immediately and then delete the message from your system. You must not copy, distribute, disclose or act upon the contents of this email. Personal and corporate data submitted will be used in a correct, transparent and lawful manner. The data collected will be processed in paper or computerized form for the performance of contractual and lawful obligations as well as for the effective management of business relationship. The data processor is Telsey S.p.A. The data subject may exercise all the rights set forth in art. 7 of Law by Decree 30.06.2003 n. 196 as reported in the following url http://www.telsey.com/privacy.asp. __ 798t8RfNa6Dl8Ilf __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
RE: [PATCH] PURIFY and valgrind
Agreed, though where possible it's preferable for PURIFY-handling to simply not use the uninitialised data at all, rather than initialising it before use. (NB, I know this yields the same quality result, but appearances in the code are often as important as the outcome of the executable - particularly where package maintainers are concerned...) IIRC, it's complicated to remove the (uninitialised) loads here without also removing the desired stores, in which case I totally agree that memset(,0,) is a more obvious PURIFY(/valgrind) workaround. But where it's possible to trim our use of an input buffer so that we don't take some uninitialised trimmings, then the intent of the code would be much clearer. Moreover, it's more likely valgrind would then find bugs in the PRNG code (if ever we accidently debianise it ourselves) - whereas wholesale memset()s kind of limit the effectiveness of such debugging tools. FWIW, I don't agree. Defining PURIFY should not make algorithmic changes. How much data a call passes to the PRNG is an algorithmic change. Initializing data to zeroes that would otherwise be uninitialized (and could be zeroes anyway for all we know) is much more palatable than passing a different length down a call chain. Or put another way, -1 to the suggested patch. Also, -1 to the general statement that PURIFY is *not* for production system. That is not our call to make! If the production system is experiencing problems and is being diagnosed - it may very well make sense to run it under PURIFY/valgrind to help track down problems that have only been observed in production. (Anyone who has ever tried to gaze into a crystal ball and invent a test-case to reproduce sporadic problems from a production system will know what I'm talking about). I agree. PURIFY should not make any algorithmic changes, unless they are absolutely necessary. Ideally, IMO, defining PURIFY should cause all, and only, uninitialized data to be zeroed. That way, if we don't initialize something we meant to, it will still be caught. Just zeroing things we plan to set to other values later is an inferior solution. But changing the amount of data we pass to the PRNG is an inferior solution too. DS __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager [EMAIL PROTECTED]
