RE: Query regarding extracting ssl hello sni.

2014-04-11 Thread Lukas Tribus
Hi,


 I would suggest that it will not harm even if you relax the check for
 client hello too as the old client can using SSL 3.0 is still supported
 and its according to RFC

I disagree. SNI is documented as a TLS extension and unsupported in SSLv3.
RFC3546 and RFC6066 are the relevant RFCs, not RFC5246.

Try SSLv3 with SNI:
 openssl s_client -connect localhost:443 -servername snitest -ssl3

The client_hello doesn't contain SNI, because TLS extensions are not
supported in SSLv3.



 and also note that the max supported TLS version is 3.3. I would suggest
 the below mentioned changes.

I disagree here also. Why should we limit the max supported TLS version and
introduce forward compatibility issues? We will just be a like those stupid
obsolete SSL middleboxes the browsers need to workaround everytime they
enable a new SSL feature. I would rather avoid that.



Regards,

Lukas

  


Re: Query regarding extracting ssl hello sni.

2014-04-11 Thread Pravin Tatti
I too agree with your first comment relax only the check for record layer
version as SNI is extensions for TLS protocols.
I think the next version may or may not contain the same client hello
format if it allows i don't have any issues if it doesn't allows then the
code may crash or it may return bad value for SNI. I just suggested it for
safety reasons its just my input.


On Fri, Apr 11, 2014 at 12:09 PM, Lukas Tribus luky...@hotmail.com wrote:

 Hi,


  I would suggest that it will not harm even if you relax the check for
  client hello too as the old client can using SSL 3.0 is still supported
  and its according to RFC

 I disagree. SNI is documented as a TLS extension and unsupported in SSLv3.
 RFC3546 and RFC6066 are the relevant RFCs, not RFC5246.

 Try SSLv3 with SNI:
  openssl s_client -connect localhost:443 -servername snitest -ssl3

 The client_hello doesn't contain SNI, because TLS extensions are not
 supported in SSLv3.



  and also note that the max supported TLS version is 3.3. I would suggest
  the below mentioned changes.

 I disagree here also. Why should we limit the max supported TLS version and
 introduce forward compatibility issues? We will just be a like those stupid
 obsolete SSL middleboxes the browsers need to workaround everytime they
 enable a new SSL feature. I would rather avoid that.



 Regards,

 Lukas




RE: Query regarding extracting ssl hello sni.

2014-04-11 Thread Lukas Tribus
Hi,


 I think the next version may or may not contain the same client hello 
 format if it allows i don't have any issues if it doesn't allows then 
 the code may crash or it may return bad value for SNI. I just suggested 
 it for safety reasons its just my input.

If HAproxy would crash, we would need to fix the actual reason of the
crash, not ignore SNI when TLS version is higher than 1.2, because an
attacker can always send packets with TLSv1.2 and the offending payload,
even if its not valid packet as per RFC.


As for bad values: SNI is a client provided value and thus must never
be trusted. We can use it for routing the request to different backends,
but we always need to validate it before doing something with it.




Regards,

Lukas

  


Re: Query regarding extracting ssl hello sni.

2014-04-11 Thread Pravin Tatti
Ok fine you can be forward compatible but i still don't agree its my
personal opinion if I don't know what the packet format for next version
why should I support it. But this was not the major issue for what i
started the discussion. I think the major is relaxing the record layer
check to SSLv3 and we should fix it.




On Fri, Apr 11, 2014 at 4:32 PM, Lukas Tribus luky...@hotmail.com wrote:

 Hi,


  I think the next version may or may not contain the same client hello
  format if it allows i don't have any issues if it doesn't allows then
  the code may crash or it may return bad value for SNI. I just suggested
  it for safety reasons its just my input.

 If HAproxy would crash, we would need to fix the actual reason of the
 crash, not ignore SNI when TLS version is higher than 1.2, because an
 attacker can always send packets with TLSv1.2 and the offending payload,
 even if its not valid packet as per RFC.


 As for bad values: SNI is a client provided value and thus must never
 be trusted. We can use it for routing the request to different backends,
 but we always need to validate it before doing something with it.




 Regards,

 Lukas




Re: Query regarding extracting ssl hello sni.

2014-04-11 Thread Willy Tarreau
On Fri, Apr 11, 2014 at 04:57:17PM +0530, Pravin Tatti wrote:
 Ok fine you can be forward compatible but i still don't agree its my
 personal opinion if I don't know what the packet format for next version
 why should I support it. But this was not the major issue for what i
 started the discussion. I think the major is relaxing the record layer
 check to SSLv3 and we should fix it.

It's not a matter of opinion but specification. If the packet format is
specified as being exclusively for 3.0..3.3, then we should only match
this. If it's specified as part of TLS for which only versions 3.0 to
3.3 are currently defined, then we must apply the default rule specified
for the whole protocol according to how to handle newer versions. All
protocols generally indicate how newer versions must be handled, and
it's important to stick on the rule they dictate in order not to break
interoperability with future clients.

Willy




RE: Query regarding extracting ssl hello sni.

2014-04-11 Thread Lukas Tribus
Hi,


 Ok fine you can be forward compatible but i still don't agree its my 
 personal opinion if I don't know what the packet format for next 
 version why should I support it.

Because we are talking about a industry standard with a huge user base
and it is very likely that next version will be backwards compatible,
like it was the case with TLSv1.2-TLSv1.1-TLSv1.0-SSLv3. Its unlikely
that the IETF WG will push a change that will break this format.

If the next TLS version doesn't break it, SNI will continue to work.
If the next TLS version breaks SNI, we will need to fix it.

If we restrict the SNI fetching to TLSv1.[0-2], we need to fix SNI
in every case, without any obvious advantage.



 I think the major is relaxing the record layer check to SSLv3 and we
 should fix it. 

Its already fixed, Willy committed it yesterday. Tonight's snapshot already
contains the fix.




Regards,

Lukas

  


RE: Query regarding extracting ssl hello sni.

2014-04-11 Thread Lukas Tribus
Hi,


 It's not a matter of opinion but specification. If the packet format is
 specified as being exclusively for 3.0..3.3, then we should only match
 this. If it's specified as part of TLS for which only versions 3.0 to
 3.3 are currently defined, then we must apply the default rule specified
 for the whole protocol according to how to handle newer versions. All
 protocols generally indicate how newer versions must be handled, and
 it's important to stick on the rule they dictate in order not to break
 interoperability with future clients.

Mmmmh, good point.


TLS extensions are documented pretty strictly against certain TLS version:
RFC3546 -- TLSv1.0
RFC4366 -- TLSv1.0 and TLSv1.1
RFC6066 -- TLSv1.2


I'm not sure that those RFCs really address forward compatibility. I took
a very quick look at RFC6066, but didn't found any real indication of what
will happen in future version, other than:

   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).  The data structure associated with the host_name NameType
   is a variable-length vector that begins with a 16-bit length.  For
   backward compatibility, all future data structures associated with
   new NameTypes MUST begin with a 16-bit length field.  TLS MAY treat
   provided server names as opaque data and pass the names and types to
   the application.


So, according to what you said and how the RFCs are written Pravin would
be right suggesting to limit the SNI fetch to TLSv1.2.

Anyway, even if we do this, its a good thing to separate the bugfix (relax
the record version check) and a change (only fetch SNI up to TLSv1.2) instead
of having both in a single commit.


So, should we do this? Should both record version and client_hello be checked
to be = TLSv1.2?




Regards,

Lukas

  


Re: Query regarding extracting ssl hello sni.

2014-04-11 Thread Willy Tarreau
Hi Lukas,

On Fri, Apr 11, 2014 at 03:17:32PM +0200, Lukas Tribus wrote:
 Hi,
 
  It's not a matter of opinion but specification. If the packet format is
  specified as being exclusively for 3.0..3.3, then we should only match
  this. If it's specified as part of TLS for which only versions 3.0 to
  3.3 are currently defined, then we must apply the default rule specified
  for the whole protocol according to how to handle newer versions. All
  protocols generally indicate how newer versions must be handled, and
  it's important to stick on the rule they dictate in order not to break
  interoperability with future clients.
 
 Mmmmh, good point.
 
 
 TLS extensions are documented pretty strictly against certain TLS version:
 RFC3546 -- TLSv1.0
 RFC4366 -- TLSv1.0 and TLSv1.1
 RFC6066 -- TLSv1.2

 I'm not sure that those RFCs really address forward compatibility.

Well, I have a different interpretation of this. First, 2246 clearly says
that TLSv1.0 has major 3 and minor 1 because it's a minor improvement over
SSLv3.0. So that clearly states that a change of the minor here only means
a minor improvement.

Second, 3546 discusses extensions and says :

  The extensions may be used by TLS clients and servers.  The
   extensions are backwards compatible - communication is possible
   between TLS 1.0 clients that support the extensions and TLS 1.0
   servers that do not support the extensions, and vice versa.

This implies forward compatibility since both sides may use extensions
in a backwards compatible way with the other one. Otherwise, lack of
forward compatibility would break this type of backwards compatibility.

5246 says this :

  Appendix E.  Backward Compatibility

   E.1.  Compatibility with TLS 1.0/1.1 and SSL 3.0

   Since there are various versions of TLS (1.0, 1.1, 1.2, and any
   future versions) and SSL (2.0 and 3.0), means are needed to negotiate
   the specific protocol version to use.  The TLS protocol provides a
   built-in mechanism for version negotiation so as not to bother other
   protocol components with the complexities of version selection.

   TLS versions 1.0, 1.1, and 1.2, and SSL 3.0 are very similar, and use
   compatible ClientHello messages; thus, supporting all of them is
   relatively easy.  Similarly, servers can easily handle clients trying
   to use future versions of TLS as long as the ClientHello format
   remains compatible, and the client supports the highest protocol
   version available in the server.

   A TLS 1.2 client who wishes to negotiate with such older servers will
   send a normal TLS 1.2 ClientHello, containing { 3, 3 } (TLS 1.2) in
   ClientHello.client_version.  If the server does not support this
   version, it will respond with a ServerHello containing an older
   version number.  If the client agrees to use this version, the
   negotiation will proceed as appropriate for the negotiated protocol.

Again, to me it clearly states that the focus is on supporting different
versions on both sides, which implies that a client talking 1.2 can be
understood by a server talking 1.1 etc...

I see nothing in these specs mandating a limit to a certain protocol
level for certain features. All I'm seeing is protocol version is
{ 3, 3 } for this version of the spec in 1.2, or { 3, 2 } for 1.1,
etc... It just reminds what to *emit* to indicate what protocol you're
speaking, not anything related to a sudden drop of forwards compatibility.

So in my opinion, since the protocol is designed to be backwards and
forwards compatible, and uses minor version for newer extensions, there
is no reason for limiting ourselves to TLSv1.2 for extensions that
exist since 1.0 and will certainly continue to be supported by later
version, at least for backwards compatibility.

Regards,
Willy




RE: Query regarding extracting ssl hello sni.

2014-04-11 Thread Lukas Tribus
Hi Willy,


 So in my opinion, since the protocol is designed to be backwards and
 forwards compatible, and uses minor version for newer extensions, there
 is no reason for limiting ourselves to TLSv1.2 for extensions that
 exist since 1.0 and will certainly continue to be supported by later
 version, at least for backwards compatibility.

Ok. I do in fact agree with this and it matches with what I was saying
earlier in this thread, I just didn't take the time to read the RFC
carefully enough to draw the same conclusion.



Thanks,

Lukas

  


Re: Query regarding extracting ssl hello sni.

2014-04-10 Thread Willy Tarreau
Hi,

On Thu, Apr 10, 2014 at 10:33:38AM +0530, Pravin Tatti wrote:
 Hi,
 
 The function smp_fetch_ssl_hello_sni() only supports record layer version
 and client hello version greater than or equal to 3.1. But as in the
 RFC5246 in appendix E says that TLS versions 1.0, 1.1, and 1.2, and SSL
 3.0 are very similar and also if we check the last 2 paras as mentioned
 below
 
  Earlier versions of the TLS specification were not fully clear on what
 the record layer version number (TLSPlaintext.version) should contain when
 sending ClientHello (i.e., before it is known which version of the protocol
 will be employed). Thus, TLS servers compliant with this specification MUST
 accept any value {03,XX} as the record layer version number for
 ClientHello.
 
 TLS clients that wish to negotiate with older servers MAY send any value
 {03,XX} as the record layer version number. Typical values would be
 {03,00}, the lowest version number supported by the client, and the value
 of ClientHello.client_version. No single value will guarantee
 interoperability with all old servers, but this is a complex topic beyond
 the scope of this document. 
 
 This indicates that the tls record version might be SSL 3.0 and the client
 hello might be TLS version 1.0, 1.1 or 1.2.
 
 Do we need to consider this case while parsing in smp_fetch_ssl_hello_sni()
 or is there any other specific reason for it.
 
 I noticed such kind of issue when application is using gnutls instead of
 openssl.
 ie., the record layer version is SSL 3.0 and client hello is 3.3.

Well, there is no particular reason for being limited to a certain set of
versions, however I think we did it this way because SNI started to be used
long after any client totally stopped supporting SSLv3, so it did not appear
that there was any intersection between SSLv3 and SNI. But if there is, sure,
maybe propose a patch!

Thanks,
Willy




Re: Query regarding extracting ssl hello sni.

2014-04-10 Thread Pravin Tatti
I think you still didn't understood the problem. There are two versions in
SSL one is record layer hello version and the client hello version. Any
application that support TLS versions 1.0, 1.1, 1.3 or SSLv3 (client hello
version) may contain SSL 3.0 as the record layer version number and the
once the negotiation is done the record layer version is updated.
The problem is not with SSLv3 alone the problem is with all the TLS
versions 1.0, 1.1, 1.3 or SSLv3 who has the record layer version as SSLv3
for client hello packet.

The problem is the application using gnutls instead of openssl has record
layer hello version set to SSL 3.0 for client hello handshake and the
client hello version to TLSv2 (max TLS version supported by client).

What i suggest is fetching of SNI is still valid even if the record layer
version is 3.0 and the actual client hello version is any of the TLS
versions including SSLv3.




On Thu, Apr 10, 2014 at 2:34 PM, Willy Tarreau w...@1wt.eu wrote:

  [image: Boxbe] https://www.boxbe.com/overview Willy Tarreau (w...@1wt.eu)
 is not on your Guest 
 Listhttps://www.boxbe.com/approved-list?tc_serial=16879868868tc_rand=1178275786utm_source=stfutm_medium=emailutm_campaign=ANNO_MWTPutm_content=001token=%2Fwy11T7R1QVKnkycPjMukAJgXwKDuCjUkkpoPo3fnUbD27SXBkGi8peGgEmh%2FReckey=pl7vWF6XC3JfowFZA%2F0CL7u6ZiLZzX%2BJ6UJhky1jiZM%3D|
  Approve
 senderhttps://www.boxbe.com/anno?tc_serial=16879868868tc_rand=1178275786utm_source=stfutm_medium=emailutm_campaign=ANNO_MWTPutm_content=001token=%2Fwy11T7R1QVKnkycPjMukAJgXwKDuCjUkkpoPo3fnUbD27SXBkGi8peGgEmh%2FReckey=pl7vWF6XC3JfowFZA%2F0CL7u6ZiLZzX%2BJ6UJhky1jiZM%3D|
  Approve
 domainhttps://www.boxbe.com/anno?tc_serial=16879868868tc_rand=1178275786utm_source=stfutm_medium=emailutm_campaign=ANNO_MWTPutm_content=001domtoken=%2Fwy11T7R1QVKnkycPjMukAJgXwKDuCjUkkpoPo3fnUbD27SXBkGi8peGgEmh%2FReckey=pl7vWF6XC3JfowFZA%2F0CL7u6ZiLZzX%2BJ6UJhky1jiZM%3D

 Hi,

 On Thu, Apr 10, 2014 at 10:33:38AM +0530, Pravin Tatti wrote:
  Hi,
 
  The function smp_fetch_ssl_hello_sni() only supports record layer version
  and client hello version greater than or equal to 3.1. But as in the
  RFC5246 in appendix E says that TLS versions 1.0, 1.1, and 1.2, and SSL
  3.0 are very similar and also if we check the last 2 paras as mentioned
  below
 
   Earlier versions of the TLS specification were not fully clear on what
  the record layer version number (TLSPlaintext.version) should contain
 when
  sending ClientHello (i.e., before it is known which version of the
 protocol
  will be employed). Thus, TLS servers compliant with this specification
 MUST
  accept any value {03,XX} as the record layer version number for
  ClientHello.
 
  TLS clients that wish to negotiate with older servers MAY send any value
  {03,XX} as the record layer version number. Typical values would be
  {03,00}, the lowest version number supported by the client, and the value
  of ClientHello.client_version. No single value will guarantee
  interoperability with all old servers, but this is a complex topic beyond
  the scope of this document. 
 
  This indicates that the tls record version might be SSL 3.0 and the
 client
  hello might be TLS version 1.0, 1.1 or 1.2.
 
  Do we need to consider this case while parsing in
 smp_fetch_ssl_hello_sni()
  or is there any other specific reason for it.
 
  I noticed such kind of issue when application is using gnutls instead of
  openssl.
  ie., the record layer version is SSL 3.0 and client hello is 3.3.

 Well, there is no particular reason for being limited to a certain set of
 versions, however I think we did it this way because SNI started to be used
 long after any client totally stopped supporting SSLv3, so it did not
 appear
 that there was any intersection between SSLv3 and SNI. But if there is,
 sure,
 maybe propose a patch!

 Thanks,
 Willy





Re: Query regarding extracting ssl hello sni.

2014-04-10 Thread Willy Tarreau
On Thu, Apr 10, 2014 at 06:30:26PM +0530, Pravin Tatti wrote:
 I think you still didn't understood the problem. There are two versions in
 SSL one is record layer hello version and the client hello version. Any
 application that support TLS versions 1.0, 1.1, 1.3 or SSLv3 (client hello
 version) may contain SSL 3.0 as the record layer version number and the
 once the negotiation is done the record layer version is updated.
 The problem is not with SSLv3 alone the problem is with all the TLS
 versions 1.0, 1.1, 1.3 or SSLv3 who has the record layer version as SSLv3
 for client hello packet.

OK thanks for clarifying.

 The problem is the application using gnutls instead of openssl has record
 layer hello version set to SSL 3.0 for client hello handshake and the
 client hello version to TLSv2 (max TLS version supported by client).
 
 What i suggest is fetching of SNI is still valid even if the record layer
 version is 3.0 and the actual client hello version is any of the TLS
 versions including SSLv3.

Fine, could you send a patch to do that then ?

Willy




RE: Query regarding extracting ssl hello sni.

2014-04-10 Thread Lukas Tribus



 Date: Thu, 10 Apr 2014 15:22:43 +0200
 From: w...@1wt.eu
 To: tattiprav...@gmail.com
 CC: haproxy@formilux.org
 Subject: Re: Query regarding extracting ssl hello sni.

 On Thu, Apr 10, 2014 at 06:30:26PM +0530, Pravin Tatti wrote:
 I think you still didn't understood the problem. There are two versions in
 SSL one is record layer hello version and the client hello version. Any
 application that support TLS versions 1.0, 1.1, 1.3 or SSLv3 (client hello
 version) may contain SSL 3.0 as the record layer version number and the
 once the negotiation is done the record layer version is updated.
 The problem is not with SSLv3 alone the problem is with all the TLS
 versions 1.0, 1.1, 1.3 or SSLv3 who has the record layer version as SSLv3
 for client hello packet.

 OK thanks for clarifying.

Basically we just need to relax the record layer check to SSLv3 - and leave
the clienthello check as is, right?

Does the attached diff do the job for you correctly, Pravin?



Regards,

Lukas





  

sslv3-record-layer-sni.diff
Description: Binary data


RE: Query regarding extracting ssl hello sni.

2014-04-10 Thread Lukas Tribus
Hi,


 Basically we just need to relax the record layer check to SSLv3 - and
 leave the clienthello check as is, right?

 Does the attached diff do the job for you correctly, Pravin?

I have reproduced the issue with gnutls and can confirm that the patch
fixes the problem.

The function now requires only SSLv3 or later in the record layer, but
still requires at least TLSv1.0 in the client hello.

I don't think any SNI capable client announces SSLv2 in the record layer
or worse.


I will submit the patch formally.



Regards,

Lukas

  


Re: Query regarding extracting ssl hello sni.

2014-04-10 Thread Pravin Tatti
I would suggest that it will not harm even if you relax the check for
client hello too as the old client can using SSL 3.0 is still supported and
its according to RFC and also note that the max supported TLS version is
3.3. I would suggest the below mentioned changes.

288c288
 /* Check for TLSv1 or later (SSL version = 3.1) */
---
 /* Check for SSLv3 or later (SSL version = 3.0) */
291c291
 if (data[1]  0x03 || data[2]  0x01)
---
 if (data[1]  0x03 || data[2]  0x03)
322c322
 if (data[0]  0x03 || data[1]  0x01) /* TLSv1 minimum */
---
 if (data[0]  0x03 || data[1]  0x03) /* SSLv3 minimum TLSv1.2
maximum */


On Fri, Apr 11, 2014 at 12:43 AM, Lukas Tribus luky...@hotmail.com wrote:

 Hi,


  Basically we just need to relax the record layer check to SSLv3 - and
  leave the clienthello check as is, right?
 
  Does the attached diff do the job for you correctly, Pravin?

 I have reproduced the issue with gnutls and can confirm that the patch
 fixes the problem.

 The function now requires only SSLv3 or later in the record layer, but
 still requires at least TLSv1.0 in the client hello.

 I don't think any SNI capable client announces SSLv2 in the record layer
 or worse.


 I will submit the patch formally.



 Regards,

 Lukas




Query regarding extracting ssl hello sni.

2014-04-09 Thread Pravin Tatti
Hi,

The function smp_fetch_ssl_hello_sni() only supports record layer version
and client hello version greater than or equal to 3.1. But as in the
RFC5246 in appendix E says that TLS versions 1.0, 1.1, and 1.2, and SSL
3.0 are very similar and also if we check the last 2 paras as mentioned
below

 Earlier versions of the TLS specification were not fully clear on what
the record layer version number (TLSPlaintext.version) should contain when
sending ClientHello (i.e., before it is known which version of the protocol
will be employed). Thus, TLS servers compliant with this specification MUST
accept any value {03,XX} as the record layer version number for
ClientHello.

TLS clients that wish to negotiate with older servers MAY send any value
{03,XX} as the record layer version number. Typical values would be
{03,00}, the lowest version number supported by the client, and the value
of ClientHello.client_version. No single value will guarantee
interoperability with all old servers, but this is a complex topic beyond
the scope of this document. 

This indicates that the tls record version might be SSL 3.0 and the client
hello might be TLS version 1.0, 1.1 or 1.2.

Do we need to consider this case while parsing in smp_fetch_ssl_hello_sni()
or is there any other specific reason for it.

I noticed such kind of issue when application is using gnutls instead of
openssl.
ie., the record layer version is SSL 3.0 and client hello is 3.3.