14 ms·
Heap overflow bug in OpenSSL
- tptacek 15y agoThis looks pretty bad. The canonical bit of untrusted ASN.1/DER information most systems deal with: SSL certificates. In particular, all OpenSSL-based SSL client software (Ruby and Python HTTP libraries), and web applications that use client certificates for authentication. When Tavis Ormandy says "textbook heap overflow", patch fast.
- dchest 15y ago9th vulnerability in 2012.
- adobriyan 15y agoLengths should always be unsigned. Always. I'd even say they should always be uintmax_t in C.
- djmdjm 15y agoPlease explain how a string length would need more than size_t to represent it.
- adobriyan 15y agoThis is largely theoretical but still. Smart programming languages have bignum integers and seamless promotion between fixnum and bignum versions. Thus they avoid the issue completely, no stupid casts (especially casts which change both signedness and size), no overflows, no nothing, just an integer length. It's a perfect world and everyone should try to achieve it. C is not such programming language, with the closest approximation being uintmax_t. size_t is of course enough in practice.
- wladimir 15y agoEven for fixnums, integer overflows and underflows of any kind should ideally result in an exception by default. I think it's a pity that C(++) doesn't have support for this. A lot of bugs and weaknesses could have been prevented (for example, CWE 680 http://cwe.mitre.org/data/definitions/680.html http://cwe.mitre.org/data/definitions/680.html). I know this decision (to simply wrap around in the case of an over/underflow) was probably performance-driven, but on the other hand, if the common languages had required it, CPUs would have better support for it... Edit: Some googling shows that Microsoft has a SafeInt class in common use that reports under/overflow: http://safeint.codeplex.com/ http://safeint.codeplex.com/ . Still it feels like a kludge for this to be not part of the main language.
- JoachimSchipper 15y ago(Hint: gcc has a -ftrapv option, which traps on signed integer overflow. Unsigned overflow is defined, so trapping on that would break working code.)
- wladimir 15y agoI don't think I'd necessarily want to trap on all (signed) integer overflows. As you say, it might break working code. There's just too much C/C++ code around to change that retroactively. And for modular arithmetic and such it is desirable for integers to wrap around. But a "trap on overflow" signed and unsigned int type would be nice.
- JoachimSchipper 15y agoIMHO, size_t (for in-memory objects) or off_t (for files) makes more sense. That is not the issue here, though: casting longs to ints would cause trouble even if both are unsigned.
- adobriyan 15y agoYou didn't understand. Consistent use of one (unsigned) type for lengths would avoid issues because there'd be no casts. off_t is signed and doesn't make sense because file length can't be negative.
- adobriyan 15y agoIn fact existence of off64_t proves off_t is not OK.
- tedunangst 15y agoThe existence of off64_t proves Linux fucked up. It's not a standard type and other operating systems don't have it.
- acqq 15y ago> off_t is signed and doesn't make sense Wrong, it's used to seek in both directions in calls where direction is signed by... a sign! http://www.unix.com/man-page/POSIX/3posix/lseek/ http://www.unix.com/man-page/POSIX/3posix/lseek/
- adobriyan 15y ago[slowly] off_t is wrong for maintaining length because off_t is signed and lengths are fundamentally unsigned. Signed offset is OK for seeking.
- JoachimSchipper 15y agoIndeed, consistent use of one type for lengths avoids issues because you need no casts. For in-memory data, the type to use is size_t since that is what e.g. memcpy(), strncmp() and read() accept; for a position in a file or a file length, off_t is a better choice, since that is what pread(), lseek() and stat() use. I'm a fan of unsigned data types in general, but using off_t for file lengths is the way to go. (It's not like you need defined overflow semantics for file positions, anyway.) (You don't need off64_t if you compile with the proper #defines, which you should do on Linux.)
- SamReidHughes 15y agoNo. Lengths should always be signed.
- tlb 15y agoThat only avoids some problems. For example, this is vulnerable to s = MAX_SIZE_T/4 + 1 size_t s = get_size(); int *a = (int *)malloc(s * sizeof(int)); for (size_t i=0; i<s; i++) { a[i] = get_int(); } I've gotten in the habit of, when reading array sizes over the wire, explicitly limiting to a reasonable value like 1 million. Occasionally things should be allowed to use all available memory, but it's rare.
- astrange 15y agoGoogle recommends the opposite: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types http://google-styleguide.googlecode.com/svn/trunk/cppguide.x... I prefer this since unsigned underflow (which is an easy bug) produces a value which is still a valid size and is not detected by IOC or -ftrapv. Also, it requires you to use unsigned loop indexes, which will simply lead to more bugs. The fact that your program contains no individual objects whose size is > INT_MAX should be a sign that you should use int for their size.
- djmdjm 15y agoFYI OpenSSH's sshd is not vulnerable, despite using OpenSSL. Back in 2002 and after a different ASN.1 bug, Markus Friedl observed that RSA verification used the OpenSSL's full ASN.1 parser to parse PKCS#1 RSA signatures despite them having an almost entirely fixed format under the parameters used in the SSH protocol. He wrote a minimal RSA signature verification implementation that we've used ever since. This is the eighth bug that it has saved us from in the last ten years. Attack surface reduction works. Note, other parts of OpenSSH are vulnerable - specifically private key loading. So, if you allow untrusted users to supply private keys to ssh, ssh-add or ssh-keygen running in a privileged context, then you should patch this bug ASAP.
- agl 15y agoWhile having a minimal signature verifier is good, you wouldn't have been hit by this bug in any case. This only affects parsing from a BIO or a FILE, and the OpenSSL RSA code parses the signature from memory.
- linker3000 15y agoIs this likely to have any implications for users of OpenVPN?
- dhx 15y agoYes. wget -q -O - http://swupdate.openvpn.org/community/releases/openvpn-2.2.2.tar.gz http://swupdate.openvpn.org/community/releases/openvpn-2.2.2... | tar -xzO | grep d2i if ((eku = (EXTENDED_KEY_USAGE *)X509_get_ext_d2i (x509, NID_ext_key_usage, NULL, NULL)) == NULL) { if ((ku = (ASN1_BIT_STRING *)X509_get_ext_d2i (x509, NID_key_usage, NULL, NULL)) == NULL) { p12 = d2i_PKCS12_bio(b64, NULL); p12 = d2i_PKCS12_fp(fp, NULL); cert = d2i_X509(NULL, (const unsigned char **) &cd->cert_context->pbCertEncoded,
- js2 15y agoStrictly speaking, yes it is likely to have some implication(s). I think more importantly is whether it is likely to allow a server to be remotely exploited. The answer to that is "probably not", and "very likely not" if using OpenVPN's tls-auth option. At least as far as I understand the issue. Also, http://article.gmane.org/gmane.network.openvpn.devel/6309 http://article.gmane.org/gmane.network.openvpn.devel/6309
- deleted 15y ago[deleted]
- dhx 15y agoSample emergency bump for Gentoo: Firstly and most importantly: check http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/dev-libs/openssl/ChangeLog?view=markup http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/dev-... to see whether the Gentoo developers have already bumped OpenSSL in the official repository. If so, ignore everything below! wget -O /usr/portage/distfiles/openssl-1.0.0i.tar.gz http://www.openssl.org/source/openssl-1.0.0i.tar.gz chown portage:portage /usr/portage/distfiles/openssl-1.0.0i.tar.gz chmod g+w /usr/portage/distfiles/openssl-1.0.0i.tar.gz mkdir -p /usr/local/portage/dev-libs/openssl cp /usr/portage/dev-libs/openssl/openssl-1.0.0h.ebuild /usr/local/portage/dev-libs/openssl/openssl-1.0.0i.ebuild cp -R /usr/portage/dev-libs/openssl/files /usr/local/portage/dev-libs/openssl/ ebuild /usr/local/portage/dev-libs/openssl/openssl-1.0.0i.ebuild digest emerge -1q =dev-libs/openssl-1.0.0i shutdown -r -t 0 now Skip the first 3 commands when mirrors have the latest OpenSSL tarballs. Preferably skip the last command and manually restart daemons that rely on OpenSSL. I have used the drastic example of restarting the entire server in case someone blindly follows the above commands without thinking it through carefully. Note that openssl-1.0.1* is currently masked in Gentoo ~amd64. If you have it unmasked, it should be easy to adjust the above commands to use openssl-1.0.1a instead.
- agl 15y agoThe OpenSSL advisory contains details of the problematic functions, which are considerably smaller than the full set of ASN.1 functions: http://www.openssl.org/news/secadv_20120419.txt http://www.openssl.org/news/secadv_20120419.txt
- dhx 15y agoTo get an idea of which packages may be affected (try different functions for more results): https://github.com/search?q=d2i_X509_bio&type=Code https://github.com/search?q=d2i_X509_bio&type=Code http://www.koders.com/default.aspx?s=d2i_X509_bio http://www.koders.com/default.aspx?s=d2i_X509_bio Examples of impacted software include Android, Apache HTTPd (mod_ssl)[1] and Ruby. To reiterate what you've already stated elsewhere in this discussion, software shouldn't have a need to call these functions to validate certificates provided by remote clients. Users of email clients making heavy use of S/MIME and administrators managing PKI (signing, revoking, etc) may need to apply caution. [1] See line 99 at https://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c?view=markup https://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/... where Apache tries to load a PEM formatted certificate. If this fails, Apache tries loading the file as a DER+Base64 formatted certificate or as a last resort, just DER (both which use the vulnerable d2i_X509_bio function). Given that the PEM format is the standard that most Apache administrators are using and injection of vulnerable certificates and keys usually requires root permissions, Apache/mod_ssl users can probably treat this vulnerability as a non-issue.
- 0x0 15y agoWhat about HTTP SSL Client Certificates; i.e. what if a "browser" (attacker) simply includes a bad SSL Client Certificate in a request?
- jws 15y agoASN.1 is a ludicrously complicated format for what it does. Its designers added all sorts of ways to save a few bits here and there by creating optional special cases to be handled in the encoders and decoders. This makes complicated code with lots of edge cases for bugs to hide in, Cisco SNMP implementations were notoriously savaged by this. Ironically, it also requires lots of branches which slow down modern processors more than just reading the bits. Fortunately most uses of ASN.1 have died. SNMP and SSL are still using it. It's not 1984 anymore, let it die.
- ghshephard 15y agoOne of the emerging "Standard" SCADA protocols, IEC-61850 MMS uses ASN.1 as its encoding mechanism, so we'll continue to see format in broad use...
- tptacek 15y agoActually, all of the SNMP implementations were savaged by ASN.1/BER implementation bugs: the format is so poorly understood that virtually all of them derived from the same code. There is a trick to ASN.1/DER encoding that gets it down into the hundreds- of- lines- of- code range (you get the whole document in memory and serialize/unserialize it backwards). I once wrote a program that converted arbitrary ASN.1/DER/BER buffers into a shell script that regenerated the same. It's not as complicated as it looks, but it's idiosyncratic and you have to think about it a specific way. That said: I agree, it should be scorched off the planet with fire.
- samroberts 15y agoHow do you go backwards? You don't know the length of the last element/position of the last tag unless you work forward from the front, as far as I can see. BER is complicated, but no more complicated than rfc-822 style headers, which are also poorly parsed by ad-hoc parsers, and were a wide source of vulnerabilities.
- wladimir 15y agoI vaguely remember that parts of the GSM protocol were also described using ASN.1 (BER). I'm not sure how relevant this is for newer cellular network protocols, though. But if so, that makes it unlikely it will go away soon.
- EvilTerran 15y agoCould this be used maliciously to, say, gain access or control of an affected system, or to subvert certificate checking? Or is crashing the process that's using OpenSSL the worst it could do?
- tptacek 15y agoIf you are using the affected ASN.1 functions and you are feeding them attacker-controlled ASN.1 data (say, SSL certificates during user configuration), it is likely that attackers will be able to run their own code in your programs. You are probably not using the affected functions.
- EvilTerran 15y agoAm I right in thinking that would involve taking advantage of the memory corruption to inject code by using an appropriately constructed cert?
- tptacek 15y agoYes, but read 'agl's comments carefully: the systems impacted by this are going to tend to be ones that do special-case configuration of SSL certificates. We're not talking about browsers and (for the most part) web servers here. A hypothetical future Github feature that allowed users to upload SSL certs in lieu of SSH keys might have to review their code to make sure they weren't using OpenSSL BIOs to read certs from (or just patch). You should patch anyways. From now on, professional security assessments are going to doc this version of OpenSSL as a vulnerability.
- EvilTerran 15y agoOh, I'm not using OpenSSL professionally - I'm pretty much just a curious amateur when it comes to computer security. Good to know I've not got anything to worry about personally, though. You've explained it well.
- 15y ago
- newlog 15y agoI suppose it could not derive to arbitrary code execution, isn't it?
- deleted 15y ago[deleted]
- dhx 15y agoTests against some commonly used software that make use of libssl to load certificates from disk (or perhaps a socket in strange circumstances): php-5.4.1RC2 (and php-5.4.0): potentially affected (one match for d2i_PKCS12_bio) tar -xjOf /usr/portage/distfiles/php-5.4.1RC2.tar.bz2 | grep d2i nginx-1.1.19: probably safe (1 match for d2i_SSL_SESSION) tar -xzOf /usr/portage/distfiles/nginx-1.1.19.tar.gz | grep d2i postgresql-9.1.3: probably safe (no matches) tar -xjOf /usr/portage/distfiles/postgresql-9.1.3.tar.bz2 | grep d2i postfix-2.9.1: probably safe (one match for X509_get_ext_d2i and d2i_SSL_SESSION each) tar -xzOf /usr/portage/distfiles/postfix-2.9.1.tar.gz | grep d2i dovecot-2.1.4: probably safe (3 matches for d2i_DHparams, 1 match for X509_get_ext_d2i) tar -xzOf /usr/portage/distfiles/dovecot-2.1.4.tar.gz | grep d2i
- mhurron 15y agoIn most cases these packages should be loading libssl as a shared library. Simply updating libssl to a fixed version would solve the problem for all of those instances.
- munin 15y agoapparently, this was published in 2006: https://twitter.com/mdowd/status/192986878138523648 https://twitter.com/mdowd/status/192986878138523648
- __david__ 15y agoThe commit is here if you want to see what they did to fix it: http://cvs.openssl.org/chngview?cn=22447 http://cvs.openssl.org/chngview?cn=22447
- sohn 15y agoCool Debian will have it in 8 years
- comice 15y agothey already released a fix for it, yesterday.
- dguido 15y agoMark Dowd found this bug and described it in The Art of Software Security Assessment (TAOSSA) in 2006. lawl. https://twitter.com/#!/mdowd/status/192986878138523648 https://twitter.com/#!/mdowd/status/192986878138523648 Here's the actual page: http://i.imgur.com/vPjOR.jpg http://i.imgur.com/vPjOR.jpg