628312 - Need IDL to be able to set expected hostname against which to check SSL cert

Status: RESOLVED WONTFIX

(Core :: Security: PSM, enhancement)

Description

[Ben Bucksch (:BenB)]

If your socket goes to a different hostname than hostname/domain the user requested, and the SSL cert is for the latter, you need the ability to set the hostname to verify the cert against.

Use-case: I am implementing XMPP in chrome-JS. XMPP uses DNS SRV lookups to find the hostname of a server. For the user, the connection just goes to "foo.com". We make a DNS SRV lookup of _xmpp-client._tcp.foo.com, and find "5 0 5222 jabber.bar.com", so we connect to jabber.bar.com:5222. Yet, the SSL cert of the server is for "foo.bar", because that's what the user originally wanted to connect to. This is how SSL is intended to be used, and anyways how XMPP works.

Unfortunately, PSM doesn't let me do that. If I open a socket to jabber.bar.com:5222, it sets SSL_SetURL(fd, "jabber.bar.com");. PSM doesn't expose any API to change that, much less from JS. NSS does expose said SSL_SetURL().

Comment 1

[Ben Bucksch (:BenB)]

Attachment: First pass (v1)

Comment 2

[Ben Bucksch (:BenB)]

This compiles just fine, and seems to be the right fix.
I can also see in JS that Ci.nsISSLExpectedHost is defined.
But nsISocketTransport.securityInfo doesn't want to QI to it.
- In the begining, it's null (great!)
- Right before and after
  socket.securityInfo.QueryInterface(Ci.nsISSLSocketControl).StartTLS();
  it does QI to nsISSLSocketControl, but not to nsISSLExpectedHost.
No idea what's going on.

This is all too opaque and totally undocumented to be able to solve it.

Comment 3

[Ben Bucksch (:BenB)]

Kai?

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 506443 [details] [diff] [review]
First pass (v1)

Sorry, I don't have time to look at feature enhancements at the PSM level. 

And I was told that any such changes are nowadays required to come with automated tests.

Comment 5

[Florian Quèze [:florian]]

(In reply to Kai Engert (:kaie) from comment #4)
> Comment on attachment 506443 [details] [diff] [review]
> First pass (v1)
> 
> Sorry, I don't have time to look at feature enhancements at the PSM level. 
> 
> And I was told that any such changes are nowadays required to come with
> automated tests.

Can you suggest someone to review this once tests are added? Thanks!

Comment 6

[Kai Engert (:KaiE:)]

bsmith or honzab.moz (mozilla networking)

Comment 7

[Kai Engert (:KaiE:)]

May be related to, see 
Bug 644640 comment 127

Comment 8

[Ben Bucksch (:BenB)]

FWIW, I won't add tests myself, so we need some volunteer for it.

Comment 9

[JP Rosevear [:jpr]]

Ben, is the patch still relevant?  If so, please point it for review as per #6

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 506443 [details] [diff] [review]
First pass (v1)

Review of attachment 506443 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is very old and has to be updated first.  We have split some code out from nsNSSSocketInfo to TransportSecurityInfo (that nsNSSSocketInfo inherits from).  The TransportSecurityInfo class has just informative character.

If you want to be able to override the host name, then add a new method (attribute?) to nsISSLSocketControl.  That will call SSL_SetURL and when it succeeds modify the name in TransportSecurityInfo as well.

However, before I would review such patch it would need a bit more thinking.  Having a JS exposed API for this might have some consequences.

Comment 11

[Ben Bucksch (:BenB)]

Which consequences? Chrome JS code is just as trusted as C code, as the former can call nsIProcess.run(). There are no security implications.

That this API is desperately needed is explained in comment 0. We need this for proper XMPP support.

Thanks for the tip on nsISSLSocketControl and SSL_SetURL().

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Ben Bucksch (:BenB) from comment #0)
> Use-case: I am implementing XMPP in chrome-JS. XMPP uses DNS SRV lookups to
> find the hostname of a server. For the user, the connection just goes to
> "foo.com". We make a DNS SRV lookup of _xmpp-client._tcp.foo.com, and find
> "5 0 5222 jabber.bar.com", so we connect to jabber.bar.com:5222. Yet, the
> SSL cert of the server is for "foo.bar", because that's what the user
> originally wanted to connect to. This is how SSL is intended to be used, and
> anyways how XMPP works.

Please link to the spec that says that XMPP must work this way. AFAICT, SSL is definitely not supposed to be used this way. If you connect to https://hostname, then the certificate must be valid for https://hostname.

In particular, what if the DNS SRV lookup lookup resulted in "5 0 5222 paypal.com". Then, are we going to accept a foo.com certificate for that connection? No way.

Comment 13

[Devdatta Akhawe [:devd]]

The opposite attack is also possible. DNS is an insecure mechanism. I don't see how you can trust it to say which hostnames are correct.

Comment 14

[Patrick McManus [:mcmanus]]

I think ben and brian are closer than you think - but the interface ben wants isn't the right thing to do :) And I say that without reading XMPP - but I've certainly been thinking about srv wrt http/2.

The srv is basically just routing information, like dns or even a proxy tunnel for tls.

so, using ben's example config, if the user clicks on an xmpp url for foo.com the server cert better darn well cover foo.com as brian said and ben indicates is the case of what happens.

The detail here is just getting the socket transport code which establishes the TCP connection to be given the service name and honor that and srv as part of its normal dns lookup process. The SSL stack just inherits that socket (peered at the TCP level to jabber.bar.com:5222) and the SSL level hostname (foo.com) in the url bar as usual. That's the patch we want imo.

opening a https connection to jabber.bar.com:5222 and trying to tell PSM the hostname is really foo.com (which is how I understand the proposed patch.. maybe I'm wrong) isn't the right way to go.

Comment 15

[Florian Quèze [:florian]]

(In reply to Brian Smith (:bsmith) from comment #12)

> Please link to the spec that says that XMPP must work this way.

http://xmpp.org/rfcs/rfc3920.html#tls-overview see point 8 (the sentence starting with "Certificates MUST be checked against the hostname as provided by the initiating entity (e.g., a user), not the hostname as resolved via the Domain Name System").

> In particular, what if the DNS SRV lookup lookup resulted in "5 0 5222
> paypal.com". Then, are we going to accept a foo.com certificate for that
> connection? No way.

I'll give you an example of the similar situation on HTTPS, as this may clarify things for you: If the user types https://www.example.com in the address bar, and www.example.com is a CNAME record pointing to staging.example.com, you want to require a valid certificate for "www.example.com", not for "staging.example.com".

SRV records are the equivalent of CNAME records, but for specific services.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 506443 [details] [diff] [review]
First pass (v1)

Review of attachment 506443 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> http://xmpp.org/rfcs/rfc3920.html#tls-overview see point 8 (the sentence
> starting with "Certificates MUST be checked against the hostname as provided
> by the initiating entity (e.g., a user), not the hostname as resolved via
> the Domain Name System").
> 
> > In particular, what if the DNS SRV lookup lookup resulted in "5 0 5222
> > paypal.com". Then, are we going to accept a foo.com certificate for that
> > connection? No way.
> 
> I'll give you an example of the similar situation on HTTPS, as this may
> clarify things for you: If the user types https://www.example.com in the
> address bar, and www.example.com is a CNAME record pointing to
> staging.example.com, you want to require a valid certificate for
> "www.example.com", not for "staging.example.com".
> 
> SRV records are the equivalent of CNAME records, but for specific services.

Right. This is a good clarification. The important question is whether the hostname in question is "the hostname as provided by the initiating entity" or the "hostname as resolved by the DNS system."

(In reply to Patrick McManus [:mcmanus] from comment #14)
> The detail here is just getting the socket transport code which establishes
> the TCP connection to be given the service name and honor that and srv as
> part of its normal dns lookup process. The SSL stack just inherits that
> socket (peered at the TCP level to jabber.bar.com:5222) and the SSL level
> hostname (foo.com) in the url bar as usual. That's the patch we want imo.

Right. Basically, nsNSSSocketInfo (nsISSLSocketControl/nsITransportSecurityInfo) expects to be given the "effective" name to use. It has no use for any other hostname. It seems like we need a patch to handle SRV records correctly instead of this patch.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am resolving this as invalid based on the comments above. It seems like Patrick and I (at least) agree on what the correct form of a solution should look like. I think part of the motivation of this patch may be that we've been blocked on getting various SRV-related patches landed. Let's try to resolve that issue in the SRV-related bugs.

Please re-open if you feel like there's something that's being missed here.

Comment 18

[Florian Quèze [:florian]]

(In reply to Brian Smith (:bsmith) from comment #17)
> I think part of the motivation of this patch may be that we've
> been blocked on getting various SRV-related patches landed.

I think this is right. This patch seems the smallest change that's absolutely required in the core to let Mozilla-based XMPP implementations use DNS SRV code shipped as a separate binary XPCOM component.

> Let's try to resolve that issue in the SRV-related bugs.

Unless there's a clear commitment from someone to move forward the DNS SRV bug(s) soon (are you committing to this?), I think the patch here still has some value.

Comment 19

[Honza Bambas (:mayhemer)]

Brian, Patrick, thanks for the good formulation of the state here.

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> I think this is right. This patch seems the smallest change that's
> absolutely required in the core to let Mozilla-based XMPP implementations
> use DNS SRV code shipped as a separate binary XPCOM component.

I don't understand the issues holding up SRV support as well as mcmanus does, but I do know that mcmanus has tried to drive home some of the decision-making here.

> > Let's try to resolve that issue in the SRV-related bugs.
> 
> Unless there's a clear commitment from someone to move forward the DNS SRV
> bug(s) soon (are you committing to this?), I think the patch here still has
> some value.

I would say that, if we can't have full SRV support in Gecko soon for whatever reason, it may be reasonable for us to consider providing hooks to our DNS resolution that lets one plug in SRV support. But, this is a question that is best answered by those that work on the DNS-related stuff in Necko, not me. I suggest we take the topic to dev-tech-network.

By the way, one of the things I *don't* like about the patch is that it makes nsNSSSocketInfo's hostname a mutable variable, when it is currently immutable. It is *very* unclear that it is safe for us to change the hostname at any point during a handshake, and a handshake can occur at any time. This is particularly complicated by the SPDY connection coalescing and our asynchronous certificate validation (certificates are validated in parallel to the SSL handshake).

Further, this patch makes the SetHostName method scriptable, meaning that it is intended to be called on the main thread. But, SSL_SetURL should only be called before we start the initial handshake, or at least only during the execution of a libssl callback like AuthCertificateHook or HandshakeCallback.

So, the r- isn't just a design philosophy issue, it is also a code correctness issue.

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> (In reply to Brian Smith (:bsmith) from comment #17)
> > Let's try to resolve that issue in the SRV-related bugs.
> 
> Unless there's a clear commitment from someone to move forward the DNS SRV
> bug(s) soon (are you committing to this?), I think the patch here still has
> some value.

I think for B2G email v2 we may be able to get Mozilla Corp resources explicitly assigned to finish implementing DNS SRV support.  Obviously if someone wants to move things forward before whenever that turns out to be, that would be great.

Comment 22

[Ben Bucksch (:BenB)]

Sorry, but the request is absolutely valid as formulated.

1. We don't have DNS SRV support in Mozilla yet. At least 3 different people, including myself, have created patches for it, over the course of 5-10 years, none of them has even gotten as much as a proper review.
2. I think that DNS SRV support is none of NSS's business. But even with that implemented in whatever way, I think the application should still be in control of which entity the certificate is checked against, not NSS alone and automatically. It's fine and good that NSS does things automatically correctly in 99% of the cases, but there are cases where the application initiating the connection wants explicit control over the check process, and that must be possible. When I ran into this bug, I could not believe that this is impossible.

Comment 23

[Ben Bucksch (:BenB)]

> nsNSSSocketInfo (nsISSLSocketControl/nsITransportSecurityInfo) expects to be given the
> "effective" name to use.

The problem is that I have no way, from JS, to set this. All I wanted was to create a socket, then start SSL on it and tell NSS which hostname to check against. If there is a way, please let me know.

There are other situations conceivable where we would need this. *Any* protocol that makes hostnames transparent for the end user will need this, and not all of them will use DNS SRV.

Comment 24

[Ben Bucksch (:BenB)]

> So, the r- isn't just a design philosophy issue, it is also a code correctness issue.

Thanks for the review.

Comment 25

[Peter Eckersley]

(In reply to Brian Smith (:bsmith) from comment #12)

> 
> Please link to the spec that says that XMPP must work this way. AFAICT, SSL
> is definitely not supposed to be used this way. If you connect to
> https://hostname, then the certificate must be valid for https://hostname.

This is something that we want the ability to alter from within extensions.  In HTTPS Everywhere for instance we want to be able to tell the browser to accept a certificate for Akamai or Amazon AWS if the domain is hosted there, and cert isn't configured correctly (at the moment we know about thousands of sites that could be fetched over SSL with this tweak; they're the ones labelled "mismatches" in this list: https://gitweb.torproject.org/https-everywhere.git/tree/HEAD:/src/chrome/content/rules ).

I'm not sure if this has any bearing on this particular patch, since we were hoping to get this extension API via #644640.

Comment 26

[Ben Bucksch (:BenB)]

(In reply to Brian Smith (:bsmith) from comment #12)

> Please link to the spec that says that XMPP must work this way. AFAICT, SSL
> is definitely not supposed to be used this way. If you connect to
> https://hostname, then the certificate must be valid for https://hostname.

RFC 3920, Section 5.1, point 8 <http://xmpp.org/rfcs/rfc3920.html>:
Certificates MUST be checked against the hostname as provided by the initiating entity (e.g., a user), not the hostname as resolved via the Domain Name System; e.g., if the user specifies a hostname of "example.com" but a DNS SRV [SRV] lookup returned "im.example.com", the certificate MUST be checked as "example.com".


In XMPP, the user specifies his own account to be [e-mail] . The XMPP client then makes a "DNS SRV _xmpp.web.de" or similar DNS lookup and finds that the XMPP server for web.de is xmpp-webde.web.de. The client connects to xmpp-webde.web.de. But the certificate must be for web.de, not for xmpp-webde.web.de. Otherwise we'd have an insecure DNS lookup step in the process and the SSL validation would be useless.

I don't think DNS lookups are the domain of NSS, but if you want to implement all of DNS SRV for all protocols in NSS, be my guest. It certainly would make my life easier (bug 14328).

Comment 27

[Patrick McManus [:mcmanus]]

I think this ought to be WONTFIXED. David?

The hostRoute concept of nsSocketTransport::Init provides an example of how a connection can be made to one host (the routed host - presumably for the purposes of this bug that would be the result of a SRV lookup) without changing the origin host (which is what is given to PSM and used for authentication). PSM does not need to be aware of how the connection is routed (because PSM does not need to create the connection), just what the origin is that needs to be authenticated.

I think (hope?) that any use of SRV can live outside of PSM.

Comment 28

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Yes, I think that's the right abstraction.