Comment by opheliate | Hacker News Reader

Comment by opheliate | Hacker News Reader

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

1: https://github.com/openssl/openssl/blob/5cbd2ea3f94aa8adec9b...

2: https://docs.rs/rustls/0.19.1/rustls/struct.ClientSession.ht...

3: https://www.openssl.org/docs/man1.1.1/man3/SSL_set_tlsext_ho...

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.

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 :)

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

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.

SNI is specified to only take DNS hostnames. Not bits of URL. DNS hostnames only. OpenSSL and gnutls are faulty in this respect.

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

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?

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.

X509's subject alternative name extension has nothing to do with TLS' SNI extension. SNI does not have a mechanism for locally-defined name types akin to X509's OtherName.

SNI matches certificates by looking at the SAN list on a list of certificates and finding the best match.

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.

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.

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.

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.

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.

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

You don't need to validate the hostnamez you can safely leave that for name resolution libraries. You just need to properly separate the user info from the host name and port. Which is pretty simply defined in the grammar.

That depends on your use case. If you're trying to check the hostname against some sort of whitelist or blacklist you're going to want to normalize it. Then you have all kinds of fun like IDN to deal with.

Reply on news.ycombinator.com