Heap overflow bug in OpenSSL | Hacker News Reader

Heap overflow bug in OpenSSL

lists.grok.org.uk

150 pointsanthonyb58 comments

FYI 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.

While 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.

ASN.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.

One 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...

Actually, 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.

How 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.

I 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.

Since it was standardized by ITU, it's still used in newer telecom protocols, and it's pervasively used throughout higher level cryptography-related protocols/standards, CAdES is one of the more recent ones and gaining acceptance in the EU. Seems like we have to live with it for a while...

It is being used in much more than just telecom and cryptography. Even the UPU use it for tracking packages that get shipped. Take a look at Space Link Extension (for space communications) that uses ASN.1 also.

Say hello to LTE and anything UTRAN-based - RRC is based on ASN.1.

The recent ANSI/NIST ITL-2011 biometric standard is XML Schema based, but includes a TYPE-98 security record that is nothing but ASN.1. ASN.1 was chosen to meet performance requirements, as XML-based cryptographic messaging was found to be too slow.

The just published DoD EBTS 3.0 standard uses ITL-2011 and is also defined using XSD. But it too looks to an ASN.1 binary representation of its XML markup messages to get the data compression and performance needed for high volume transaction systems and hand held mobile biometric collection devices.

Lots and lots of things still use it in telco land, and many of those things have crawled out to the internet (like h.323 voip).

But it did successfully mitigate, if not kill, the threat of having Sun's XDR (eXternal Data Representation) become the defacto way of describing complex data structures. The arguments that ASN used in the RPC standards wars were "no need to bundle both a description and the data", "new stuff could be added without changing clients", and "all systems (little endian and big endian) have similar data conversion costs."

It was a really annoying war (I suspect there isn't any other kind although some people apparently really enjoy the standards 'game').

My favorite quote from Bob Lyon at the time, "Try to solve the problem, not the future."

They'll be saying the same of XML in a few years. All attempts at a "universal" format for data interchange end up ludicrous.

The 1984 version of ASN.1 should die, but the 2008 version is widely being used in many industries. Digital Certificates still use it. Air-to-ground communication uses it. Financial services are using it. LTE (the next generation of mobile telephony) uses it. Electric Utilities are using it in monitoring power lines. Toll Roads are using it for those electronic payments so that you don't have to stop at toll booths. Incident Management - Fire, Ambulance, Police, Trafic Control, etc - use ASN.1. I could go on naming other areas as well. The 2008 Edition of ASN.1 is definitely NOT dying!!

Mark Dowd found this bug and described it in The Art of Software Security Assessment (TAOSSA) in 2006. lawl.

https://twitter.com/#!/mdowd/status/192986878138523648

Here's the actual page: http://i.imgur.com/vPjOR.jpg

Sample emergency bump for Gentoo:

Firstly and most importantly: check 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.

The 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

To 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

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/... 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.

What about HTTP SSL Client Certificates; i.e. what if a "browser" (attacker) simply includes a bad SSL Client Certificate in a request?

This 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.

Parsing from PEM isn't affected, nor is parsing of SSL certificates in SSL because libssl doesn't use the BIO functions.

It's mostly users of OpenSSL who are calling d2i_XXX_bio or d2i_XXX_fp.

Rats, you're right. I should stop commenting 15 minutes after I wake up.

Tests 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

In 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.

The commit is here if you want to see what they did to fix it: http://cvs.openssl.org/chngview?cn=22447

apparently, this was published in 2006: https://twitter.com/mdowd/status/192986878138523648

Could 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?

If 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.

Am I right in thinking that would involve taking advantage of the memory corruption to inject code by using an appropriately constructed cert?

Yes, 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.

Oh, 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.

Adam Langley's the one who did the real explaining on this thread; thank 'agl. :|

9th vulnerability in 2012.

Is this likely to have any implications for users of OpenVPN?

Yes.

wget -q -O - 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,

Strictly 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

I suppose it could not derive to arbitrary code execution, isn't it?

Lengths should always be unsigned.

Always.

I'd even say they should always be uintmax_t in C.

Please explain how a string length would need more than size_t to represent it.

This 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.

Even 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).

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/ . Still it feels like a kludge for this to be not part of the main language.

(Hint: gcc has a -ftrapv option, which traps on signed integer overflow. Unsigned overflow is defined, so trapping on that would break working code.)

I 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.

Recent gcc versions use the fact that signed overflow is undefined to do some unexpected optimizations (in particular, a + b < a will never be true if a, b are ints.) I don't think -ftrapv is going to cause many additional errors, but I haven't actually tried it. (Also, http://embed.cs.utah.edu/ioc/ looks interesting.)

> (in particular, a + b < a will never be true if a, b are ints.)

Code of exactly that form in the patch for this bug made me do a double take. Fortunately, they'd also changed the a & b from plain ints to size_t, so it was ok.

IMHO, 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.

You 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.

In fact existence of off64_t proves off_t is not OK.

The existence of off64_t proves Linux fucked up. It's not a standard type and other operating systems don't have it.

> 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/

[slowly]

off_t is wrong for maintaining length because off_t is signed and lengths are fundamentally unsigned.

Signed offset is OK for seeking.

Indeed, 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.)

No. Lengths should always be signed.

That 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.

Google recommends the opposite:

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.

Cool Debian will have it in 8 years

they already released a fix for it, yesterday.

Reply on news.ycombinator.com