5 ms·

Let me say, I really appreciate OpenSSL, and it's made amazing progress in terms of security over the last few years. I make monthly donations to the developers

Let me say, I really appreciate OpenSSL, and it's made amazing progress in terms of security over the last few years. I make monthly donations to the developers as I believe it's critical infrastructure. Buutt... From [0], the issue arises when they send "user:pass@host" to SSL_set_tlsext_host_name, which happily sets the SNI to whatever it's given [1]. As a point of comparison, when you create a Rustls client via ClientSession::new [2], you have to pass it a DNSNameRef, which will validate that there's no auth component in the string it wraps, and return an error if you try to set the server name to something involving auth details. I'm sure there's reasons why OpenSSL is set up to work like this, but I can't see why anyone would ever want to send those auth details in the clear in the SNI, and I wish it provided an API that would anticipate this misuse, like in Rust. The OpenSSL docs don't indicate it's an issue you should think about when invoking this function [3]. I realise I'm picking on OpenSSL here, and GnuTLS appears to do the exact same thing. I'm just not certain anyone not wearing a hazmat suit and being watched by multiple other trained professionals should be handling OpenSSL code. 0: https://www.openwall.com/lists/oss-security/2021/08/07/7 https://www.openwall.com/lists/oss-security/2021/08/07/7 1: https://github.com/openssl/openssl/blob/5cbd2ea3f94aa8adec9b4486ac757d4d688e3f8c/ssl/s3_lib.c#L3475 https://github.com/openssl/openssl/blob/5cbd2ea3f94aa8adec9b... 2: https://docs.rs/rustls/0.19.1/rustls/struct.ClientSession.html#method.new https://docs.rs/rustls/0.19.1/rustls/struct.ClientSession.ht... 3: https://www.openssl.org/docs/man1.1.1/man3/SSL_set_tlsext_host_name.html https://www.openssl.org/docs/man1.1.1/man3/SSL_set_tlsext_ho...

ziml77 5y ago
Does this mean that they've never been parsing URLs correctly? Kind of crazy if that's the case since a web browser would be the last place I'd expect to find such a basic URL processing issue.
- opheliate 5y ago
  I would think that this was introduced at the same time as SNI was enabled for Lynx (2009-04-26 from the changelog, but I haven’t checked if it was actually vulnerable at that time). I don’t know if I’d call any URL processing issue basic though, I can safely say that I’m never even going to attempt to write a URL parser, just because of the number of potential edge cases. You can imagine when you’re enabling a (fairly simple, compared to the rest of the TLS spec) extension, you might not be thinking about all the different quirks of the URL spec which could occur and mess up what seems like a basic change.
  - nly 5y ago
    URIs have a generic grammar defined in RFC 3986. However, this doesn't cover DNS name validation for the hostname: "This specification does not mandate a particular registered name lookup technology and therefore does not restrict the syntax of reg-name beyond what is necessary for interoperability" It also doesn't constrain the userinfo ("username:password") part described in this vulnerability. So yeah, it's non-trivial.
    - tialaramex 5y ago
      I don't see what's "non-trivial" about the decomposition: authority = [ userinfo "@" ] host [ ":" port ] described in section 3.2 of that specification. Looks pretty trivial to me.
      - nly 5y ago
        It's non-trivial because the userinfo part is not structured in the grammar, but is instead just constrained to a particular character set. The RFC reads: > Use of the format "user:password" in the userinfo field is deprecated. In the real world, for HTTP use cases, you can't just ignore this because it's deprecated.
        tialaramex 5y ago
        > the userinfo part is not structured in the grammar, but is instead just constrained to a particular character set. Yes. So, a correct implementation can't use this document to destructure it, but that's OK because it doesn't need to do so. It can however easily distinguish the userinfo from the rest of the authority. Unless, like this code, it simply doesn't bother to try. > In the real world, for HTTP use cases, you can't just ignore this because it's deprecated. Then I guess you'll need to write code to further parse the userinfo. Seems pretty easy, but this document doesn't explain how because it's deprecated. No impact on whether you can find the host name in the authority since that's a separate field from userinfo. Assuming, of course, that you bother to separate userinfo from the domain name in the authority, which Lynx did not.
kevin_thibedeau 5y ago
OpenSSL isn't an HTTPS library. It's TLS and agnostic to any higher level protocol. It's not their job to filter strings with rules that may not apply for every use case.
- ctz 5y ago
  SNI is specified to only take DNS hostnames. Not bits of URL. DNS hostnames only. OpenSSL and gnutls are faulty in this respect.
  - kevin_thibedeau 5y ago
    RFC 6066: Currently, the only server names supported are DNS hostnames; however, this does not imply any dependency of TLS on DNS, and other name types may be added in the future (by an RFC that updates this document).
    - tialaramex 5y ago
      In practice this functionality is rusted in place. If you invented a new name type, that doesn't mean you can call your existing TLS stack's SNI methods with "@Twitter_handle" or "my@email.address" or "#TopicGoesHere" because those methods always write DNS names. There is a type parameter, there has only ever been one type (DNS names) and so that's what your implementation chooses and I haven't ever seen one that lets you change it, because what would you change it to?
      - kevin_thibedeau 5y ago
        You don't have to invent a new name type. RFC 5280: The subject alternative name extension allows identities to be bound to the subject of the certificate. These identities may be included in addition to or in place of the identity in the subject field of the certificate. Defined options include an Internet electronic mail address, a DNS name, an IP address, and a Uniform Resource Identifier (URI). Other options exist, including completely local definitions.
ctz 5y ago
During the development of rustls, we also found a similar bug in Apple's TLS stack. We reported that to Apple and they fixed it. So it is feasible -- at scale -- for TLS clients to validate that DNS names in the SNI extension really are DNS names, and are not IP addresses or bits of URL.
- opheliate 5y ago
  Thank you so much for your work on rustls by the way, such a great library, and the results of the last audit were very impressive :)
- zinekeller 5y ago
  > During the development of rustls, we also found a similar bug in Apple's TLS stack. Was Windows' Schannel also tested? Or this is simply due to the dev team having access to Linux and macOS?

ziml77 5y ago

Does this mean that they've never been parsing URLs correctly? Kind of crazy if that's the case since a web browser would be the last place I'd expect to find such a basic URL processing issue.

opheliate 5y ago
I would think that this was introduced at the same time as SNI was enabled for Lynx (2009-04-26 from the changelog, but I haven’t checked if it was actually vulnerable at that time). I don’t know if I’d call any URL processing issue basic though, I can safely say that I’m never even going to attempt to write a URL parser, just because of the number of potential edge cases. You can imagine when you’re enabling a (fairly simple, compared to the rest of the TLS spec) extension, you might not be thinking about all the different quirks of the URL spec which could occur and mess up what seems like a basic change.
- nly 5y ago
  URIs have a generic grammar defined in RFC 3986. However, this doesn't cover DNS name validation for the hostname: "This specification does not mandate a particular registered name lookup technology and therefore does not restrict the syntax of reg-name beyond what is necessary for interoperability" It also doesn't constrain the userinfo ("username:password") part described in this vulnerability. So yeah, it's non-trivial.
  - tialaramex 5y ago
    I don't see what's "non-trivial" about the decomposition: authority = [ userinfo "@" ] host [ ":" port ] described in section 3.2 of that specification. Looks pretty trivial to me.
    - nly 5y ago
      It's non-trivial because the userinfo part is not structured in the grammar, but is instead just constrained to a particular character set. The RFC reads: > Use of the format "user:password" in the userinfo field is deprecated. In the real world, for HTTP use cases, you can't just ignore this because it's deprecated.
      - tialaramex 5y ago
        > the userinfo part is not structured in the grammar, but is instead just constrained to a particular character set. Yes. So, a correct implementation can't use this document to destructure it, but that's OK because it doesn't need to do so. It can however easily distinguish the userinfo from the rest of the authority. Unless, like this code, it simply doesn't bother to try. > In the real world, for HTTP use cases, you can't just ignore this because it's deprecated. Then I guess you'll need to write code to further parse the userinfo. Seems pretty easy, but this document doesn't explain how because it's deprecated. No impact on whether you can find the host name in the authority since that's a separate field from userinfo. Assuming, of course, that you bother to separate userinfo from the domain name in the authority, which Lynx did not.

kevin_thibedeau 5y ago

OpenSSL isn't an HTTPS library. It's TLS and agnostic to any higher level protocol. It's not their job to filter strings with rules that may not apply for every use case.

ctz 5y ago
SNI is specified to only take DNS hostnames. Not bits of URL. DNS hostnames only. OpenSSL and gnutls are faulty in this respect.
- kevin_thibedeau 5y ago
  RFC 6066: Currently, the only server names supported are DNS hostnames; however, this does not imply any dependency of TLS on DNS, and other name types may be added in the future (by an RFC that updates this document).
  - tialaramex 5y ago
    In practice this functionality is rusted in place. If you invented a new name type, that doesn't mean you can call your existing TLS stack's SNI methods with "@Twitter_handle" or "my@email.address" or "#TopicGoesHere" because those methods always write DNS names. There is a type parameter, there has only ever been one type (DNS names) and so that's what your implementation chooses and I haven't ever seen one that lets you change it, because what would you change it to?
    - kevin_thibedeau 5y ago
      You don't have to invent a new name type. RFC 5280: The subject alternative name extension allows identities to be bound to the subject of the certificate. These identities may be included in addition to or in place of the identity in the subject field of the certificate. Defined options include an Internet electronic mail address, a DNS name, an IP address, and a Uniform Resource Identifier (URI). Other options exist, including completely local definitions.

ctz 5y ago

During the development of rustls, we also found a similar bug in Apple's TLS stack. We reported that to Apple and they fixed it. So it is feasible -- at scale -- for TLS clients to validate that DNS names in the SNI extension really are DNS names, and are not IP addresses or bits of URL.

opheliate 5y ago
Thank you so much for your work on rustls by the way, such a great library, and the results of the last audit were very impressive :)
zinekeller 5y ago
> During the development of rustls, we also found a similar bug in Apple's TLS stack. Was Windows' Schannel also tested? Or this is simply due to the dev team having access to Linux and macOS?