Re: tls1_process_heartbeat/dtls1_process_heartbeat don't check RAND_pseudo_bytes return value
On 10 April 2014 18:54, Kylo Ginsberg k...@kylo.net wrote: Looking at the heartbeat code, I notice that neither of the process heartbeat functions check whether RAND_pseudo_bytes returned success when it is generating the heartbeat padding. I don't know if there are real-world scenarios where this could happen Failed memory allocation, typically. A patch might look like this: diff --git a/ssl/d1_both.c b/ssl/d1_both.c + if (RAND_pseudo_bytes(bp, padding) 0) RAND_pseudo_bytes returns -1 or 0 if it fails[1]. This expression should be RAND_pseudo_bytes(...) != 1, which basically equivalent to RAND_bytes(...) != 1. This isn't your fault; the documentation doesn't have any relationship to the actual behaviour, and the many other callers in the library are sloppy like this. Cheers, Joe [1]: http://jbp.io/2014/01/16/openssl-rand-api/#round-up __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
valgrind and ubsan tests
Hi In light of recent bad news I checked openssl master against some of the typical tools. (As many other I guess) Not symbolic sat solvers yet, stp with cryptominisat is used by some fine fuzzers, checkers and other tools already. * valgrind https://gist.github.com/rurban/10414413 valgrind complains about uninitialised value(s) but misses apparently the memset in the *_init() functions. Strange because I thought valgrind is that clever and knows about memset. * ubsan https://gist.github.com/rurban/10424468 clang's recent -fsanitizer=undefined found better problems e.g. c_enc.c:80:5: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int' obj_dat.c:151:21: runtime error: left shift of 2 by 30 places cannot be represented in type 'int' cbc128.c:96:41: runtime error: load of misaligned address 0x6110824f for type 'size_t' (aka 'unsigned long'), which requires 8 byte alignment 0x6110824f: note: pointer points here ... Note that -O3 without -fwrapv and undefined wrap or overflow behavior is rather critical on some compilers, as it may do wrong optimizations. Sooner or later. So I'm a bit worried about missing explicit wrap semantics. Not so about missing alignments. On Intel misaligned access is actually faster mostly, but it points at missing optimizations via SSE intrinsics. And if in case anybody wants to study readable openssl code: my fork at https://github.com/rurban/openssl was processed with astyle --style=bsd as I heard that many people very very upset about the current codingstyle. Of course this will not be changed currently as I guess you are very paranoid currently about so many changes. -- Reini Working towards a true Modern Perl. Slim, functional, unbloated, compile-time optimizable __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl.org #3149] [patch] Fast and side channel protected implementation of the NIST P-256 Elliptic Curve, for x86-64 platforms
For the record, I have reviewed Adam's versions of the code before these were posted here, and Adam has resolved the problems that I pointed out. As of the latest patch, I think the code is suitable for inclusion in OpenSSL. The final missing part is support that makes it easy to build with or without this NIST P-256 implementation, depending on whether the target platform supports it, similar to the enable-ec_nistp_64_gcc_128 config option for the 64-bit optimized implementations using type __uint128_t. (The current patch unconditionally links in the new files, but we may not even be able to process the new assembler files.) Also, it would be nice to have Shay review our changes to his contribution (the March 11 patch.bz2 as further changed by the April 10 patch) to make sure that while fixing the problems we found, we didn't do unwanted changes. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Getting patches applied
On 11 April 2014 00:00, Steve Marquess marqu...@opensslfoundation.com wrote: With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. I have long been of the view that the current process for reviewing patches is broken. Through no individual's fault there just aren't enough people with commit rights to review the number of patches that get submitted. Many of these patches are really quite straight forward and I think we miss out on a lot. I see lots of patches go by which would have added value (e.g. documentation fixes, minor code fixes etc). If someone has gone to the effort of providing a patch, then they really deserve some kind of response (even if that response is thanks but no thanks). Many patches go by without anyone ever looking at them. I could envisage some kind of triage process where patches are classified into different levels or risk, and dependent on that risk a different level of scrutiny is required. E.g. so a patch providing a documentation tweak is treated differently to a patch providing a big piece of new functionality. I could also imagine a hierarchy of comitters with different levels of sign-off - low risk changes can be comitted by a larger group of people - only high risk changes need to go to the core team. The multiple people could be a sufficiently large and diverse group of serious and committed stakeholders, both OpenSSL team members and others. Volunteers? I see many of the same names appear on this list and on the users list from people who clearly know what they are talking about and have a high degree of skill. I am sure some of those could be persuaded to help out. I of course would be happy to volunteer :-) Of course, a process like that wouldn't necessarily prevent future vulnerabilities like the Debian PRNG issue or the heartbeat bug. Even gross bugs are only truly obvious in hindsight. True. And in fact it seems counter-intuitive when faced with a major problem like heartbleed to open up the project to *more* people capable of comitting. The temptation is to batton-down-the-hatches and keep new people out. But actually I think by involving more people in the review process we have the opportunity to improve quality through having many-eyes reviewing changes - instead of just a very small number who don't have enough time to spend on it. Matt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: valgrind and ubsan tests
- Original Message - From: Reini Urban re...@cpanel.net To: openssl-dev@openssl.org Sent: Thursday, 10 April, 2014 11:57:32 PM Subject: valgrind and ubsan tests Hi In light of recent bad news I checked openssl master against some of the typical tools. (As many other I guess) Not symbolic sat solvers yet, stp with cryptominisat is used by some fine fuzzers, checkers and other tools already. * valgrind https://gist.github.com/rurban/10414413 valgrind complains about uninitialised value(s) but misses apparently the memset in the *_init() functions. Strange because I thought valgrind is that clever and knows about memset. I've seen this before, the problem was with the code, not valgrind. Unfortunately, I don't remember what exactly was the problem. Compile with -O0 and -ggdb, compare assembly from -O3 and -O0, that should show what's the problem. * ubsan https://gist.github.com/rurban/10424468 clang's recent -fsanitizer=undefined found better problems e.g. c_enc.c:80:5: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int' obj_dat.c:151:21: runtime error: left shift of 2 by 30 places cannot be represented in type 'int' cbc128.c:96:41: runtime error: load of misaligned address 0x6110824f for type 'size_t' (aka 'unsigned long'), which requires 8 byte alignment 0x6110824f: note: pointer points here ... Note that -O3 without -fwrapv and undefined wrap or overflow behavior is rather critical on some compilers, as it may do wrong optimizations. Sooner or later. So I'm a bit worried about missing explicit wrap semantics. Not so about missing alignments. On Intel misaligned access is actually faster mostly, but it points at missing optimizations via SSE intrinsics. Ahh, if you're compiling with -O3, then you will have false positives from valgrind. Note that some of those cases where valgrind does complain in -O3 and does not in -O0 may actually be caused by code itself being... let's say, suboptimal (e.g. duplicated). -- Regards, Hubert Kario Quality Engineer, QE BaseOS Security team Email: hka...@redhat.com Web: www.cz.redhat.com Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl-dev] Maximum length of passwords
Hello, In the apps `pkcs8', `passwd', `enc' and `pkcs12' there are hard-coded maximum lengths for passwords given: (all of the following examples will use the respective char * as buffer for `EVP_read_pw_string') pkcs8.c: 86 char pass[50], /* ... */; enc.c: 78 #define SIZE(512) ... 106 char *strbuf=NULL; ... 374 strbuf=OPENSSL_malloc(SIZE); pkcs12.c: 106 char /* ... */, macpass[50]; passwd.c: 66 char /* ... */, *passwd = NULL, /* ... */; 67 char /* ... */, *passwd_malloc = NULL; 68 size_t passwd_malloc_size = 0; ... 74 size_t pw_maxlen = 0; ... 209 if (usecrypt) 210 pw_maxlen = 8; 211 else if (use1 || useapr1) 212 pw_maxlen = 256; /* arbitrary limit, should be enough for most passwords */ ... 218 passwd_malloc_size = pw_maxlen + 2; 219 /* longer than necessary so that we can warn about truncation */ 220 passwd = passwd_malloc = OPENSSL_malloc(passwd_malloc_size); Only `passwd' warns if a password was truncated, the other programs do not even check if it was truncated. There should either be a function that automatically allocates enough memory to put the whole password in it (openssh does it this way, see read_passphrase from openssh/readpass.c), or a compile-time flag that sets the PASS_MAXLEN. Either way every `app' should check whether the whole password was read and not silently truncate the password, and all apps should behave consistently. I would like to hear which approach you would choose, malloc or PASS_MAXLEN. Regards, Jakob Kramer __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl-dev] Maximum length of passwords
- Original Message - From: Jakob Kramer jakob.kra...@gmx.de To: openssl-dev@openssl.org Sent: Friday, 11 April, 2014 3:01:42 PM Subject: [openssl-dev] Maximum length of passwords There should either be a function that automatically allocates enough memory to put the whole password in it (openssh does it this way, see read_passphrase from openssh/readpass.c), or a compile-time flag that sets the PASS_MAXLEN. Either way every `app' should check whether the whole password was read and not silently truncate the password, and all apps should behave consistently. I would like to hear which approach you would choose, malloc or PASS_MAXLEN. 128 characters allows you to hex encode 512 bits of data (e.g. from sha512sum) assuming 8 character words from 2048 word dictionary gives you 176 bit entropy for the same 128 characters. So, PASS_MAXLEN of 256 characters should be enough for anybody and of 128 for most environments. It still should refuse to accept longer passwords and not silently truncate As such, I think both solutions are acceptable. The static buffer implementation should be simpler, so should have lower probability of bugs in it. -- Regards, Hubert Kario Quality Engineer, QE BaseOS Security team Email: hka...@redhat.com Web: www.cz.redhat.com Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl.org #3301] [PATCH] Silently discard too long heartbeat messages per RFC 6520
RFC 6520, section 4 states that The total length of a HeartbeatMessage MUST NOT exceed 2^14 or max_fragment_length when negotiated as defined in [RFC6066]. and If the payload_length of a received HeartbeatMessage is too large, the received HeartbeatMessage MUST be discarded silently. The attached patch against git adds a check to silently discard heartbeat messages longer than 2^14 bytes. The max_fragment_length negotiation is not allowed to increase this value. RFC 6066 allows 2^9, 2^10, 2^11, or 2^12 as negotiated max_fragment_length values. Thanks, Erik diff --git a/ssl/d1_both.c b/ssl/d1_both.c index d8bcd58..cf74fc2 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -1338,6 +1338,8 @@ dtls1_process_heartbeat(SSL *s) /* Read type and payload length first */ if (1 + 2 + 16 s-s3-rrec.length) return 0; /* silently discard */ + if (s-s3-rrec.length SSL3_RT_MAX_PLAIN_LENGTH) + return 0; /* silently discard per RFC 6520 sec. 4 */ hbtype = *p++; n2s(p, payload); if (1 + 2 + payload + 16 s-s3-rrec.length)
Re: Getting patches applied
On Fri, Apr 11, 2014 at 10:26:08AM +0100, Matt Caswell wrote: On 11 April 2014 00:00, Steve Marquess marqu...@opensslfoundation.com wrote: With the very, very important caveat that I'm not one of the people who directly carry this burden: There is certainly room for improvement in the process by which patches are reviewed and merged into OpenSSL. For the more straightforward bug fixes and minor changes it might be useful to have a mechanism where a patch could be approved by multiple people and then committed to OpenSSL almost automatically. Obviously this wouldn't work for significant changes like whole new APIs and infrastructure mods. I have long been of the view that the current process for reviewing patches is broken. Through no individual's fault there just aren't enough people with commit rights to review the number of patches that get submitted. Many of these patches are really quite straight forward and I think we miss out on a lot. I see lots of patches go by which would have added value (e.g. documentation fixes, minor code fixes etc). If someone has gone to the effort of providing a patch, then they really deserve some kind of response (even if that response is thanks but no thanks). Many patches go by without anyone ever looking at them. Yes, and this is my main motivation for starting this thread. It's unrelated to heartbleed. Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: tls1_process_heartbeat/dtls1_process_heartbeat don't check RAND_pseudo_bytes return value
On Thu, Apr 10, 2014 at 12:04 PM, Joseph Birr-Pixton jpix...@gmail.comwrote: On 10 April 2014 18:54, Kylo Ginsberg k...@kylo.net wrote: Looking at the heartbeat code, I notice that neither of the process heartbeat functions check whether RAND_pseudo_bytes returned success when it is generating the heartbeat padding. I don't know if there are real-world scenarios where this could happen Failed memory allocation, typically. Alterntely, in rand_lib.c, if RAND_get_rand_method returns null or its pseudorand method is null, it will return -1. So there may be multiple failure modes that expose this hole. A patch might look like this: diff --git a/ssl/d1_both.c b/ssl/d1_both.c + if (RAND_pseudo_bytes(bp, padding) 0) RAND_pseudo_bytes returns -1 or 0 if it fails[1]. This expression should be RAND_pseudo_bytes(...) != 1, which basically equivalent to RAND_bytes(...) != 1. This isn't your fault; the documentation doesn't have any relationship to the actual behaviour, and the many other callers in the library are sloppy like this. I did find the docs here: https://www.openssl.org/docs/crypto/RAND_bytes.html I chose the ' 0' comparison because: a) I thought either 0 or 1 would be okay in this case (I don't think the random bytes have to be cryptographically sound for the heartbeat padding). b) I was trying to follow the style of the code base, although it's not consistent. Most of the sites in the code that check the return from RAND_pseudo_bytes use a = 0 or a 0 (though some do direct comparisons as you suggest). So I went with what seemed to be most typical practice for this code base. All that said, I mostly wanted confirmation that this is a bug. I'd be fine with tweaking the comparison to be != 1. Thanks for the comments - much appreciated! Kylo
Re: [openssl-dev] Maximum length of passwords
On 4/11/2014 8:51 AM, Hubert Kario wrote: - Original Message - From: Jakob Kramer jakob.kra...@gmx.de To: openssl-dev@openssl.org Sent: Friday, 11 April, 2014 3:01:42 PM Subject: [openssl-dev] Maximum length of passwords There should either be a function that automatically allocates enough memory to put the whole password in it (openssh does it this way, see read_passphrase from openssh/readpass.c), or a compile-time flag that sets the PASS_MAXLEN. Either way every `app' should check whether the whole password was read and not silently truncate the password, and all apps should behave consistently. I would like to hear which approach you would choose, malloc or PASS_MAXLEN. 128 characters allows you to hex encode 512 bits of data (e.g. from sha512sum) assuming 8 character words from 2048 word dictionary gives you 176 bit entropy for the same 128 characters. So, PASS_MAXLEN of 256 characters should be enough for anybody Maybe not. Some smart cards are looking at accepting a fingerprint scan as a PIN. The card then does the match. In some apps this might be passed in as a password then passed to an engine to be passed in as a PIN. (How the card detects a replay is up to the card.) and of 128 for most environments. It still should refuse to accept longer passwords and not silently truncate I agree. no silent truncation. As such, I think both solutions are acceptable. The static buffer implementation should be simpler, so should have lower probability of bugs in it. -- Douglas E. Engert deeng...@gmail.com __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: OpenSSL has exploit mitigation countermeasures to make sure its exploitable
On Fri, Apr 11, 2014 at 5:17 AM, Salz, Rich rs...@akamai.com wrote: Karma has a sense of humor. http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/ssl/Makefile?f=h;rev=1.29 http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/ssl/Makefile.diff?r1=1.29;r2=1.30;f=h Two people reviewed the change (the ok line) and they got the -D flag wrong for nearly an hour. However, to their credit, they fixed it, and it appears that it was done before the next build would take place. In any case, the next build would have pick that up. People who live in glass houses ... (Or let he who is without sin, if you're inclined that way) I would be more interested to see more work towards improving OpenSSL, rather than pointing fingers at people who gave us a high-quality SSH implementation ;-) /r$ -- Principal Security Engineer Akamai Technology Cambridge, MA __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org -- This message is strictly personal and the opinions expressed do not represent those of my employers, either past or present. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: RSA [FIPS 186-4] issue
Steve Marquess-3 wrote I think you will find that a number of other code modifications will also be required. Are you saying that you think more than just what Leon mentioned will have to be changed in order to validate RSA Key Generation? Is there any chance that OpenSSL would be willing to point to the sections of code that they (you) believe would need to be changed? -- View this message in context: http://openssl.6102.n7.nabble.com/RSA-FIPS-186-4-issue-tp48944p49310.html Sent from the OpenSSL - Dev mailing list archive at Nabble.com. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: RSA [FIPS 186-4] issue
Leon Brits wrote I am in no way capable of writing such a patch and was hoping that someone is willing to share. To be more specific I need a patch that will change the key generation from: d = e-1 mod((p-1)(q-1)) to this: d = e-1 mod(LCM(p-1, q-1)) We’re also pursuing a patch to RSA Key Generation. Leon, are you saying that you believe this is the change that is necessary in order for it to be validated? What makes you think that? I think you’re further along in the process than we are and I’d like to learn from what you’ve found. -- View this message in context: http://openssl.6102.n7.nabble.com/RSA-FIPS-186-4-issue-tp48944p49309.html Sent from the OpenSSL - Dev mailing list archive at Nabble.com. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: OpenSSL has exploit mitigation countermeasures to make sure its exploitable
On 10/04/14 18:46, Salz, Rich wrote: We've been compiling -DOPENSSL_NO_BUF_FREELISTS forever. Our only complaint is that the BUF is misspelled :) Theo can be obnoxious. This should not be news to most folks. /r$ -- Principal Security Engineer Akamai Technology Cambridge, MA Probably this blog post provides more information about what Akamai has been doing related to this issue: https://blogs.akamai.com/2014/04/heartbleed-update.html It would be appreciated if you cared to contribute back your own custom secure_malloc allocator. signature.asc Description: OpenPGP digital signature
Re: [openssl.org #3301] [PATCH] Silently discard too long heartbeat messages per RFC 6520
Hi Erik, Presumably this restriction is already enforced at the record level for all message types? Regards, Pete Dettman On 11/04/2014 9:43 PM, Erik Auerswald via RT wrote: RFC 6520, section 4 states that The total length of a HeartbeatMessage MUST NOT exceed 2^14 or max_fragment_length when negotiated as defined in [RFC6066]. and If the payload_length of a received HeartbeatMessage is too large, the received HeartbeatMessage MUST be discarded silently. The attached patch against git adds a check to silently discard heartbeat messages longer than 2^14 bytes. The max_fragment_length negotiation is not allowed to increase this value. RFC 6066 allows 2^9, 2^10, 2^11, or 2^12 as negotiated max_fragment_length values. Thanks, Erik __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
