5 ms·

OpenSSL bug exposed up to 255 bytes of client heap and existed since 2011

149 points by belter 2y ago

mrngm 2y ago
A few excerpts from the blog post (CVE-2024-5535 was assigned): > Meeting those constraints is quite unlikely nowadays: > NPN is a precursor to ALPN and was abandoned in 2012. It is very uncommon on internet servers now. > Node.js 10 and later removed NPN support, and is well past end-of-life. > Python 3.10 and later removed NPN support. OpenSSL advisory: https://openssl-library.org/news/secadv/20240627.txt https://openssl-library.org/news/secadv/20240627.txt > Severity: Low > Issue summary: Calling the OpenSSL API function SSL_select_next_proto with an empty supported client protocols buffer may cause a crash or memory contents to be sent to the peer. BoringSSL fix: https://boringssl.googlesource.com/boringssl/+/c1d9ac02514a138129872a036e3f8a1074dcb8bd https://boringssl.googlesource.com/boringssl/+/c1d9ac02514a1... The heap leak was independently observed in 2014 in the Android okhttp library: https://github.com/square/okhttp/issues/437#issuecomment-35860090 https://github.com/square/okhttp/issues/437#issuecomment-358...
deleted 2y ago
[deleted]
megadal 2y ago
The title: "bug exposed up to 255 bytes of server heap and existed since 2011" The post: "Silently sends up to 255 bytes of the client’s heap to the server."
- seba_dos1 2y ago
  Not to mention that the HN submission title never appears in the linked article.
- tux1968 2y ago
  You're right. The HN title really needs to be changed, to say it's the client's heap that is exposed.
  - belter 2y ago
    Agree. Only the mods can do it.
- dang 2y ago
  OK, we'll s/server/client/ the title above. Thanks!
hannob 2y ago
An interesting aspect of this is that it's a bug in NPN, which is more or less a historic artifact. When SPDY, the precursor of HTTP/2, got introduced, it needed a mechanism to signal that within TLS, a different protocol (SPDY instead of HTTP/1.1) was spoken. That mechanism was originally NPN. I don't know the exact details and motivation, but eventually people seem to have decided that NPN wasn't exactly what they wanted, and they invented a new mechanism called ALPN. Now, that was a decade ago (the ALPN RFC is from 2014), so the question is: why do we still have NPN code in OpenSSL? I don't think anyone uses it any more. Shouldn't it have been removed long ago? To put this in a larger context: it appears to me that OpenSSL still has a strong tendency to bloat. Heartbleed was essentially a "we added this feature, although noone knows why we need it" kind of bug, but it doesn't look to me they've changed. I still get the feeling that OpenSSL adds many features that they probably just should ignore (e.g. obscure "not invented here"-type algorithms), and they don't remove features that are obsolete.
- lazide 2y ago
  Removing old code, especially in open source, has a problem. Unless it is obviously breaking something someone wants to do, it has roughly a zero cost (in maintenance) being left alone, especially since bugs can often be ignored, but a non-zero cost to remove it - as it generally will only garner complaints and whining from any remaining users. And in open source in particular, it’s damn near impossible finding remaining users in advance, or even telling if there are no remaining users.
  - RetpolineDrama 2y ago
    Is it really "zero cost in maintenance" if that old code presents attack surface?
    - c0balt 2y ago
      No, it's zero upfront cost in maintenance. Owning code always generates some maintenance/ tech debt but it often is opaque nor easily quantifiable.
rurban 2y ago
> (though likely with a huge false-positive rate in other code) That's what I doubt at all. I rather trust the compilers than the openssl devs.
- guipsp 2y ago
  Bounds analysis are notoriously hard to get right on C code.
sebstefan 2y ago
Funny that this bug was discovered by rewriting it in Rust!
- Pet_Ant 2y ago
  I had to check if that was a meme joke or that is actually what happened, and it seems that it actually was found by rewriting it in Rust: > 2024-04-23 - Discovery of SSL_select_next_proto memory unsafety while rewriting it in rust. [ https://github.com/rustls/rustls-openssl-compat/tree/main/rustls-libssl https://github.com/rustls/rustls-openssl-compat/tree/main/ru... ]
- MaxBarraclough 2y ago
  Not the first time this kind of thing has happened. A rewrite in SPARK Ada uncovered a flaw in the reference C implementation of the Skein cryptographic algorithm. • https://www.adacore.com/press/spark-skein https://www.adacore.com/press/spark-skein (ctrl-f for flaw) • [PDF] https://www.cl.cam.ac.uk/archive/mjcg/Meeting.SecurityTools.2011/papers/sparkskein_fmats2011.pdf https://www.cl.cam.ac.uk/archive/mjcg/Meeting.SecurityTools.... (page 12) • [PDF] https://www.adacore.com/uploads/techPapers/SPARKSkein_SBMF.pdf https://www.adacore.com/uploads/techPapers/SPARKSkein_SBMF.p... (page 5)
- tialaramex 2y ago
  Specifically this is an attempted ABI compatibility layer. That is, given rustls, which is a perfectly nice Rust TLS implementation, what if we built the OpenSSL API and then shipped the C ABI compatible library ? In principle the result is definitely safe if your C code (which previously called OpenSSL) is safe, regardless of whether OpenSSL itself is riddled with bugs (which it likely is) A safety focused person rewriting tricky C pointer banging code in Rust is likely to inadvertently fix bugs - what's interesting is whether you notice that as you're doing it or it's just silently not buggy any more in Rust. A few hours ago I had some code that was doing bignum arithmetic and one call returned None where I hadn't realised it could and in a haze I thought "Oh, if it's None we can treat it as Zero" so I changed unwrap() to unwrap_or(Zero::zero()) † but nope, it was None because that function assumes we're getting integers and it's just figured out it has a proper fraction instead, so hence there is no such integer, None. Treating None as Zero meant all the related tests blew up and I realised my mistake after a couple of minutes. No, e to the power 0.5 is not 1. † Yes that's suboptimal because it will make the zero bignum even when it doesn't need it, I should have called unwrap_or_else instead but also no I shouldn't because it's wrong.
colmmacc 2y ago
I started writing s2n the day after Heartbleed and the first lines of code were for the stuffer interface. A stuffer is a buffer for stuff, and it's like Java buffered I/O for C. You can get a flavor from reading the header: https://github.com/aws/s2n-tls/blob/main/stuffer/s2n_stuffer.h https://github.com/aws/s2n-tls/blob/main/stuffer/s2n_stuffer... The implementation is incredibly simple. Treat all blocks of memory as blob with a known size and then read/write into those blobs with a cursor to track progress and bounds checks on every access. Fence all serialization/deserialization through a safe low level interface. Not only do you get memory safety (which we later proved using formal reasoning) ... but when you're parsing message formats it lends itself to a declarative coding style that makes it very clear what the structure is. You can also do lifecycle things, like erasing sensitive memory with zeroes when you're done with it, making sure things don't show up in core dumps, etc. BoringSSL introduced a Crypto_bytes API that also did some of this plus bounds checking, and retrofit it into OpenSSL. OpenSSL on the other hand is a horrific mash up of raw pointer arithmetic, ad-hoc parsers interleaved with business logic and control flow. I could never keep it straight, and it always scared me to review.

149 points by belter 2y ago

A few excerpts from the blog post (CVE-2024-5535 was assigned): > Meeting those constraints is quite unlikely nowadays: > NPN is a precursor to ALPN and was abandoned in 2012. It is very uncommon on internet servers now. > Node.js 10 and later removed NPN support, and is well past end-of-life. > Python 3.10 and later removed NPN support. OpenSSL advisory: https://openssl-library.org/news/secadv/20240627.txt https://openssl-library.org/news/secadv/20240627.txt > Severity: Low > Issue summary: Calling the OpenSSL API function SSL_select_next_proto with an empty supported client protocols buffer may cause a crash or memory contents to be sent to the peer. BoringSSL fix: https://boringssl.googlesource.com/boringssl/+/c1d9ac02514a138129872a036e3f8a1074dcb8bd https://boringssl.googlesource.com/boringssl/+/c1d9ac02514a1... The heap leak was independently observed in 2014 in the Android okhttp library: https://github.com/square/okhttp/issues/437#issuecomment-35860090 https://github.com/square/okhttp/issues/437#issuecomment-358...

deleted 2y ago

[deleted]

megadal 2y ago

The title: "bug exposed up to 255 bytes of server heap and existed since 2011" The post: "Silently sends up to 255 bytes of the client’s heap to the server."

seba_dos1 2y ago
Not to mention that the HN submission title never appears in the linked article.
tux1968 2y ago
You're right. The HN title really needs to be changed, to say it's the client's heap that is exposed.
- belter 2y ago
  Agree. Only the mods can do it.
dang 2y ago
OK, we'll s/server/client/ the title above. Thanks!

hannob 2y ago

An interesting aspect of this is that it's a bug in NPN, which is more or less a historic artifact. When SPDY, the precursor of HTTP/2, got introduced, it needed a mechanism to signal that within TLS, a different protocol (SPDY instead of HTTP/1.1) was spoken. That mechanism was originally NPN. I don't know the exact details and motivation, but eventually people seem to have decided that NPN wasn't exactly what they wanted, and they invented a new mechanism called ALPN. Now, that was a decade ago (the ALPN RFC is from 2014), so the question is: why do we still have NPN code in OpenSSL? I don't think anyone uses it any more. Shouldn't it have been removed long ago? To put this in a larger context: it appears to me that OpenSSL still has a strong tendency to bloat. Heartbleed was essentially a "we added this feature, although noone knows why we need it" kind of bug, but it doesn't look to me they've changed. I still get the feeling that OpenSSL adds many features that they probably just should ignore (e.g. obscure "not invented here"-type algorithms), and they don't remove features that are obsolete.

lazide 2y ago
Removing old code, especially in open source, has a problem. Unless it is obviously breaking something someone wants to do, it has roughly a zero cost (in maintenance) being left alone, especially since bugs can often be ignored, but a non-zero cost to remove it - as it generally will only garner complaints and whining from any remaining users. And in open source in particular, it’s damn near impossible finding remaining users in advance, or even telling if there are no remaining users.
- RetpolineDrama 2y ago
  Is it really "zero cost in maintenance" if that old code presents attack surface?
  - c0balt 2y ago
    No, it's zero upfront cost in maintenance. Owning code always generates some maintenance/ tech debt but it often is opaque nor easily quantifiable.

rurban 2y ago

> (though likely with a huge false-positive rate in other code) That's what I doubt at all. I rather trust the compilers than the openssl devs.

guipsp 2y ago
Bounds analysis are notoriously hard to get right on C code.

sebstefan 2y ago

Funny that this bug was discovered by rewriting it in Rust!

Pet_Ant 2y ago
I had to check if that was a meme joke or that is actually what happened, and it seems that it actually was found by rewriting it in Rust: > 2024-04-23 - Discovery of SSL_select_next_proto memory unsafety while rewriting it in rust. [ https://github.com/rustls/rustls-openssl-compat/tree/main/rustls-libssl https://github.com/rustls/rustls-openssl-compat/tree/main/ru... ]
MaxBarraclough 2y ago
Not the first time this kind of thing has happened. A rewrite in SPARK Ada uncovered a flaw in the reference C implementation of the Skein cryptographic algorithm. • https://www.adacore.com/press/spark-skein https://www.adacore.com/press/spark-skein (ctrl-f for flaw) • [PDF] https://www.cl.cam.ac.uk/archive/mjcg/Meeting.SecurityTools.2011/papers/sparkskein_fmats2011.pdf https://www.cl.cam.ac.uk/archive/mjcg/Meeting.SecurityTools.... (page 12) • [PDF] https://www.adacore.com/uploads/techPapers/SPARKSkein_SBMF.pdf https://www.adacore.com/uploads/techPapers/SPARKSkein_SBMF.p... (page 5)
tialaramex 2y ago
Specifically this is an attempted ABI compatibility layer. That is, given rustls, which is a perfectly nice Rust TLS implementation, what if we built the OpenSSL API and then shipped the C ABI compatible library ? In principle the result is definitely safe if your C code (which previously called OpenSSL) is safe, regardless of whether OpenSSL itself is riddled with bugs (which it likely is) A safety focused person rewriting tricky C pointer banging code in Rust is likely to inadvertently fix bugs - what's interesting is whether you notice that as you're doing it or it's just silently not buggy any more in Rust. A few hours ago I had some code that was doing bignum arithmetic and one call returned None where I hadn't realised it could and in a haze I thought "Oh, if it's None we can treat it as Zero" so I changed unwrap() to unwrap_or(Zero::zero()) † but nope, it was None because that function assumes we're getting integers and it's just figured out it has a proper fraction instead, so hence there is no such integer, None. Treating None as Zero meant all the related tests blew up and I realised my mistake after a couple of minutes. No, e to the power 0.5 is not 1. † Yes that's suboptimal because it will make the zero bignum even when it doesn't need it, I should have called unwrap_or_else instead but also no I shouldn't because it's wrong.

colmmacc 2y ago

I started writing s2n the day after Heartbleed and the first lines of code were for the stuffer interface. A stuffer is a buffer for stuff, and it's like Java buffered I/O for C. You can get a flavor from reading the header: https://github.com/aws/s2n-tls/blob/main/stuffer/s2n_stuffer.h https://github.com/aws/s2n-tls/blob/main/stuffer/s2n_stuffer... The implementation is incredibly simple. Treat all blocks of memory as blob with a known size and then read/write into those blobs with a cursor to track progress and bounds checks on every access. Fence all serialization/deserialization through a safe low level interface. Not only do you get memory safety (which we later proved using formal reasoning) ... but when you're parsing message formats it lends itself to a declarative coding style that makes it very clear what the structure is. You can also do lifecycle things, like erasing sensitive memory with zeroes when you're done with it, making sure things don't show up in core dumps, etc. BoringSSL introduced a Crypto_bytes API that also did some of this plus bounds checking, and retrofit it into OpenSSL. OpenSSL on the other hand is a horrific mash up of raw pointer arithmetic, ad-hoc parsers interleaved with business logic and control flow. I could never keep it straight, and it always scared me to review.