Re: [squid-dev] [PATCH] splicing resumed sessions
Hi Amos,
I make a new patch for squid-3.5. Use this one it should be OK.
It include changes from r14013.
On 04/13/2015 03:49 PM, Amos Jeffries wrote:
On 11/04/2015 10:01 p.m., Tsantilas Christos wrote:
Patch applied as r14012.
I am attaching the t13 patch for squid-3.5 too.
I've backported the server_name ACL patch before this one and your 3.5
patch does not seem to apply well on top of it.
However the regular backport method bzr merge -r15011..14013 trunk
seems to have only one minor collision.
I'm unsure its doing the right things though. Comparing your patch to
the changeset bzr merge produces I see these two odd chunks (in relation
to the server_name ACL changes):
--- christos.patch 2015-04-13 04:54:19.678645218 -0700
+++ bzr_merge.patch 2015-04-13 04:33:23.514618772 -0700
@@ -599,19 +598,15 @@
+return helloMsgSize;
+}
+
- bool
--Ssl::Bio::sslFeatures::get(const unsigned char *hello)
++bool
+Ssl::Bio::sslFeatures::checkForCcsOrNst(const unsigned char *msg,
size_t size)
- {
--// The SSL handshake message should starts with a 0x16 byte
--if (hello[0] == 0x16) {
--return parseV3Hello(hello);
++{
+while (size 5) {
+const int msgType = msg[0];
+const int msgSslVersion = (msg[1] 8) | msg[2];
+debugs(83, 7, SSL Message Version : std::hex
std::setw(8) std::setfill('0') msgSslVersion);
+// Check for Change Cipher Spec message
-+// RFC5246 section 6.2.1
++// RFC5246 section 6.2.1
+if (msgType == 0x14) {// Change Cipher Spec message found
+debugs(83, 7, SSL Change Cipher Spec message found);
+return true;
@@ -636,9 +631,13 @@
+return false;
+}
+
-+bool
+ bool
+-Ssl::Bio::sslFeatures::get(const unsigned char *hello)
+Ssl::Bio::sslFeatures::get(const MemBuf buf, bool record)
-+{
+ {
+-// The SSL handshake message should starts with a 0x16 byte
+-if (hello[0] == 0x16) {
+-return parseV3Hello(hello);
+int msgSize;
+if ((msgSize = parseMsgHead(buf)) = 0) {
+debugs(83, 7, Not a known SSL handshake message);
If you can try the bzr merge and confirm the diff/changeset is correct I
will do that normal cherrypicking backport.
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Splicing resuming sessions
This patch adds code in squid to control SslBump behavior when dealing with
resuming SSL/TLS sessions. Without these changes, SslBump usually terminates
all resuming sessions with an error because such sessions do not include
server certificates, preventing Squid from successfully validating the server
identity.
After these changes, Squid splices resuming sessions. Splicing is the right
because Squid most likely has spliced the original connections that the client
and server are trying to resume now.
Without SslBump, session resumption would just work, and SslBump behaviour
should approach that ideal.
Future projects may add ACL checks for allowing resuming sessions and may
add more complex algorithms, including maintaining an SMP-shared
cache of sessions that may be resumed in the future and evaluating
client/server attempts to resume a session using that cache.
This patch also makes SSL client Hello message parsing more robust and
adds an SSL server Hello message parser.
Also add support for NPN (next protocol negotiation) and ALPN (Application-Layer Protocol Negotiation) tls extensions, required to correctly bump web clients
support these extensions
Technical details
-
In Peek mode, the old Squid code would forward the client Hello message to the
server. If the server tries to resume the previous (spliced) SSL session with
the client, then Squid SSL code gets an ssl/PeerConnector.cc ccs received
early error (or similar) because the Squid SSL object expects a server
certificate and does not know anything about the session being resumed.
With this patch, Squid detects session resumption attempts and splices
Session resumption detection
There are two mechanism in SSL/TLS for resuming sessions. The traditional
shared session IDs and the TLS ticket extensions:
* If Squid detects a shared ID in both client and server Hello messages, then
Squid decides whether the session is being resumed by comparing those client
and server shared IDs. If (and only if) the IDs are the same, then Squid
assumes that it is dealing with a resuming session (using session IDs).
* If Squid detects a TLS ticket in the client Hello message and TLS ticket
support in the server Hello message as well as a Change Cipher Spec or a New
TLS Ticket message (following the server Hello message), then (and only then)
Squid assumes that it is dealing with a resuming session (using TLS tickets).
The TLS tickets check is not performed if Squid detects a shared session ID
in both client and
Re: [squid-dev] [PATCH] splicing resumed sessions
On 11/04/2015 10:01 p.m., Tsantilas Christos wrote:
Patch applied as r14012.
I am attaching the t13 patch for squid-3.5 too.
I've backported the server_name ACL patch before this one and your 3.5
patch does not seem to apply well on top of it.
However the regular backport method bzr merge -r15011..14013 trunk
seems to have only one minor collision.
I'm unsure its doing the right things though. Comparing your patch to
the changeset bzr merge produces I see these two odd chunks (in relation
to the server_name ACL changes):
--- christos.patch 2015-04-13 04:54:19.678645218 -0700
+++ bzr_merge.patch 2015-04-13 04:33:23.514618772 -0700
@@ -599,19 +598,15 @@
+return helloMsgSize;
+}
+
- bool
--Ssl::Bio::sslFeatures::get(const unsigned char *hello)
++bool
+Ssl::Bio::sslFeatures::checkForCcsOrNst(const unsigned char *msg,
size_t size)
- {
--// The SSL handshake message should starts with a 0x16 byte
--if (hello[0] == 0x16) {
--return parseV3Hello(hello);
++{
+while (size 5) {
+const int msgType = msg[0];
+const int msgSslVersion = (msg[1] 8) | msg[2];
+debugs(83, 7, SSL Message Version : std::hex
std::setw(8) std::setfill('0') msgSslVersion);
+// Check for Change Cipher Spec message
-+// RFC5246 section 6.2.1
++// RFC5246 section 6.2.1
+if (msgType == 0x14) {// Change Cipher Spec message found
+debugs(83, 7, SSL Change Cipher Spec message found);
+return true;
@@ -636,9 +631,13 @@
+return false;
+}
+
-+bool
+ bool
+-Ssl::Bio::sslFeatures::get(const unsigned char *hello)
+Ssl::Bio::sslFeatures::get(const MemBuf buf, bool record)
-+{
+ {
+-// The SSL handshake message should starts with a 0x16 byte
+-if (hello[0] == 0x16) {
+-return parseV3Hello(hello);
+int msgSize;
+if ((msgSize = parseMsgHead(buf)) = 0) {
+debugs(83, 7, Not a known SSL handshake message);
If you can try the bzr merge and confirm the diff/changeset is correct I
will do that normal cherrypicking backport.
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
Patch applied as r14012.
I am attaching the t13 patch for squid-3.5 too.
On 04/11/2015 06:18 AM, Amos Jeffries wrote:
On 11/04/2015 1:49 a.m., Tsantilas Christos wrote:
I am attaching patch for trunk and squid-3.5
Thank you. Looks pretty good now.
On 04/09/2015 04:13 PM, Amos Jeffries wrote:
* Ssl::Bio::sslFeatures::parseV3Hello()
- similar issues with s/Client Hello/ClientHello/ and SSL Extension
as above.
I did it only for the comments added or modified by this patch to avoid
increasing the size of this patch.
If required we can do it as separate patch.
Theres still one s/SSL Extension/TLS Extension/ near the end of this method.
* Ssl::Bio::sslFeatures::print()
- seems to be lacking display of ALPN received
- missing the format details for sslVersion used elsewhere:
std::hex std::setw(8) std::setfill('0')
I did not fix this. The ALPN is in an encoded form and requires some
development to correctly print it. We do not have to gain something
implementing this.
Also the Ssl::Bio::sslFeatures::print currently is not used. It is here
for using it for debugging if required.
That means the print() will be incomplete, and needs a TODO added about
the above.
* parseMsgHead() documentation about return result 0 is wrong.
- it does not return 0 when the contents of the buffer are a TLS
(non-SSL) message.
This is what it does!
The .h comment says it will return a negative number if the Hello
message is not SSL. TLS != SSL.
For the case of head[0] == 0x16 the only SSL indicator (SSLv3) is
*exactly* {0x16, 0x03, 0x00}
For TLS versions that final 0x00 byte changes. The method correctly
accepts those and returns the Hello size (N0) - in contradiction to
what is documented.
The comment should either say SSLv3 or TLS, or not mention the
protocol name at all.
OK I made this fixes too.
+1. I dont think this needs another review, once those comments are
added/updated it can merge.
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Splicing resuming sessions
This patch adds code in squid to control SslBump behavior when dealing with
resuming SSL/TLS sessions. Without these changes, SslBump usually terminates
all resuming sessions with an error because such sessions do not include
server certificates, preventing Squid from successfully validating the server
identity.
After these changes, Squid splices resuming sessions. Splicing is the right
because Squid most likely has spliced the original connections that the client
and server are trying to resume now.
Without SslBump, session resumption would just work, and SslBump behaviour
should approach that ideal.
Future projects may add ACL checks for allowing resuming sessions and may
add more complex algorithms, including maintaining an SMP-shared
cache of sessions that may be resumed in the future and evaluating
client/server attempts to resume a session using that cache.
This patch also makes SSL client Hello message parsing more robust and
adds an SSL server Hello message parser.
Also add support for NPN (next protocol negotiation) and ALPN (Application-Layer Protocol Negotiation) tls extensions, required to correctly bump web clients
support these extensions
Technical details
-
In Peek mode, the old Squid code would forward the client Hello message to the
server. If the server tries to resume the previous (spliced) SSL session with
the client, then Squid SSL code gets an ssl/PeerConnector.cc ccs received
early error (or similar) because the Squid SSL object expects a server
certificate and does not know anything about the session being resumed.
With this patch, Squid detects session resumption attempts and splices
Session resumption detection
There are two mechanism in SSL/TLS for resuming sessions. The traditional
shared session IDs and the TLS ticket extensions:
* If Squid detects a shared ID in both client and server Hello messages, then
Squid decides whether the session is being resumed by comparing those client
and server shared IDs. If (and only if) the IDs are the same, then Squid
assumes that it is dealing with a resuming session (using session IDs).
* If Squid detects a TLS ticket in the client Hello message and TLS ticket
support in the server Hello message as well as a Change Cipher Spec or a New
TLS Ticket message (following the server Hello message), then (and only then)
Squid assumes that it is dealing with a resuming session (using TLS tickets).
The TLS tickets check is not performed if Squid detects a shared session ID
in both client and server Hello messages.
NPN and ALPN tls extensions
---
Even if squid has some SSL hello messages parsing code, we are relying to
openSSL for full parsing. The openSSL used in peek and splice mode to parse
server hello message, check for errors and verify server
Re: [squid-dev] [PATCH] splicing resumed sessions
On 11/04/2015 1:49 a.m., Tsantilas Christos wrote:
I am attaching patch for trunk and squid-3.5
Thank you. Looks pretty good now.
On 04/09/2015 04:13 PM, Amos Jeffries wrote:
* Ssl::Bio::sslFeatures::parseV3Hello()
- similar issues with s/Client Hello/ClientHello/ and SSL Extension
as above.
I did it only for the comments added or modified by this patch to avoid
increasing the size of this patch.
If required we can do it as separate patch.
Theres still one s/SSL Extension/TLS Extension/ near the end of this method.
* Ssl::Bio::sslFeatures::print()
- seems to be lacking display of ALPN received
- missing the format details for sslVersion used elsewhere:
std::hex std::setw(8) std::setfill('0')
I did not fix this. The ALPN is in an encoded form and requires some
development to correctly print it. We do not have to gain something
implementing this.
Also the Ssl::Bio::sslFeatures::print currently is not used. It is here
for using it for debugging if required.
That means the print() will be incomplete, and needs a TODO added about
the above.
* parseMsgHead() documentation about return result 0 is wrong.
- it does not return 0 when the contents of the buffer are a TLS
(non-SSL) message.
This is what it does!
The .h comment says it will return a negative number if the Hello
message is not SSL. TLS != SSL.
For the case of head[0] == 0x16 the only SSL indicator (SSLv3) is
*exactly* {0x16, 0x03, 0x00}
For TLS versions that final 0x00 byte changes. The method correctly
accepts those and returns the Hello size (N0) - in contradiction to
what is documented.
The comment should either say SSLv3 or TLS, or not mention the
protocol name at all.
+1. I dont think this needs another review, once those comments are
added/updated it can merge.
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
A new version of the patch. This is removes the ssl_bump_resuming_sessions directive, includes many fixes over the previous patch. Also include support for NPN and ALPN tls extensions, required to correctly bump SSL connections. Please read carefully the patch preamble , specially the technical note part. The resumed sessions and the NPN/ALPN extensions problem appeared in squid after our decision to not allow splicing of connections for which we do not have access on the server certificates. The resumed sessions does not include server certificates, and the NPN/ALPN extensions causes openSSL to abort before retrieve and verify server certificates. The problem affects the ssl bumping and make it unusable for many cases. Many of the problems which reported by the users for squid-3.5 should be related to this. So probably this patch should applied to squid-3.5 too. If yes I will post the patch for squid-3.5 too. Regards, Christos On 03/17/2015 07:21 PM, Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. After these changes, Squid either terminates or splices resuming sessions, depending on configuration. Splicing is the right default because Squid most likely has spliced the original connections that the client and server are trying to resume now. Most likely, the splicing decision would not change now (but the lack of the server certificate information means we cannot repeat the original ACL checks and need a special directive to tell Squid what to do). Also, without SslBump, session resumption would just work, and SslBump default should approach that ideal. In many deployment scenarios, this straightforward splice or terminate resuming sessions implementation is exactly what the admin wants. Future projects may add more complex algorithms, including maintaining an SMP-shared cache of sessions that may be resumed in the future and evaluating client/server attempts to resume a session using that cache. Example: # splice all resuming sessions [this is the default] ssl_bump_resuming_sessions allow all This patch also makes SSL client Hello message parsing more robust and adds an SSL server Hello message parser. This patch also prevents occasional segfaults when dealing with SSL cache_peer negotiation failures. The last two changes should applied to squid-3.5 even if this patch will not go into squid-3.5. Regards, Christos Added ssl_bump_resuming_sessions to control treatment of resuming sessions by SslBump. This patch adds code in squid to control SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. After these changes, Squid splices resuming sessions. Splicing is the right because Squid most likely has spliced the original connections that the client and server are trying to resume now. Without SslBump, session resumption would just work, and SslBump behaviour should approach that ideal. Future projects may add ACL checks for allowing resuming sessions and may add more complex algorithms, including maintaining an SMP-shared cache of sessions that may be resumed in the future and evaluating client/server attempts to resume a session using that cache. This patch also makes SSL client Hello message parsing more robust and adds an SSL server Hello message parser. Also add support for NPN (next protocol negotiation) and ALPN (Application-Layer Protocol Negotiation) tls extensions, required to correctly bump web clients support these extensions Technical details - In Peek mode, the old Squid code would forward the client Hello message to the server. If the server tries to resume the previous (spliced) SSL session with the client, then Squid SSL code gets an ssl/PeerConnector.cc ccs received early error (or similar) because the Squid SSL object expects a server certificate and does not know anything about the session being resumed. With this patch, Squid detects session resumption attempts and splices Session resumption detection There are two mechanism in SSL/TLS for resuming sessions. The traditional shared session IDs and the TLS ticket extensions: * If Squid detects a shared ID in both client and server Hello messages, then Squid decides whether the session is being resumed by comparing those client and server shared IDs. If (and only if) the IDs are the same, then Squid assumes that it is dealing with a resuming session (using session IDs). * If Squid detects a TLS ticket in the
Re: [squid-dev] [PATCH] splicing resumed sessions
On 04/09/2015 07:13 AM, Amos Jeffries wrote: So for now this patch is okay, but we/you should already be thinking about how to auto-translate NPN from clients into ALPN to servers. Please keep in mind that it is not possible to translate something and still splice a new SSL session (the client checksum will mismatch if we alter its handshake bytes). I am not 100% sure about resumed sessions, but I would expect them to use the same level of handshake modification protection, preventing splicing of resumed SSL connections with translated handshakes. Optional translation for bumped sessions sounds like a potentially useful feature, but let's wait for somebody actually needing it. For regular (no SslBump) reverse proxy connections to SSL servers, there is no _translation_ because Squid just sends whatever extensions it (i.e., OpenSSL) supports, including NPN and/or ALPN. Cheers, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
On 04/08/2015 07:13 PM, Amos Jeffries wrote: On 4/04/2015 9:17 a.m., Alex Rousskov wrote: On 03/27/2015 05:58 AM, Amos Jeffries wrote: Indeed. Its the hostname vs SNI case we can check and SHOULD do so. The raw-IP ones we can skip the check. Some nasties will still get passed, but less than without any checks. This is all outside this patch scope though, right?! Whether or not Squid should compare peeked SNI with CONNECT hostname seems totally unrelated to splicing of resumed sessions. If so, let's get this fix in and [continue to] discuss what kind of additional checks to add to SslBump separately. While I disagree that adding the security related checks after the fact is a good approach, I can live with it. Great. If it is any comfort, this is not really after the fact. The two issues are orthogonal. One could add more checks before, after, or even instead the fix proposed on this thread. The config directive does need to go though. No objections. Christos said on IRC there were some issues after updating the patch. So I'm unsure if it will need another review before merge. If you want to make that call, I'll go with it. Christos is in a better position to make that call than I am. Either way is OK with me. Just do not want to delay this much further without a good reason because there are several pending patches and fixes that start to conflict with each other. Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
On 25/03/2015 8:35 p.m., Tsantilas Christos wrote: On 03/21/2015 07:45 AM, Amos Jeffries wrote: On 21/03/2015 10:47 a.m., Alex Rousskov wrote: On 03/20/2015 12:11 PM, Amos Jeffries wrote: On 21/03/2015 4:35 a.m., Alex Rousskov wrote: On 03/20/2015 02:06 AM, Amos Jeffries wrote: On 18/03/2015 6:21 a.m., Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. Squid is behaving wrong right now. Fix that. Yes, the proposed patch fixes that. I dont see any need for the new directive to exist We can remove the configuration option. The option was added to minimize the chance that you will object to the patch because, without the new option, the old terminate behavior would be lost. Clearly, my plan to avoid that discussion has failed! Removing the configuration option is totally fine with me because nobody has asked for it [yet]. We can always add it later if needed. The validation I'm talking about is the spec requirement that we dont allow the following scenario: CONNECT example.com:443 HTTP/1.1 ... ClientHello SNI: example.org I still think this part of the discussion is out of scope, but is there really a spec requirement that talks about comparing CONNECT- and SNI-provided server names? Where? CONNECT is about blind TCP *tunnels*. SNI is about becoming an *end point* of a TLS connection. Those two things seem mutually exclusive to me. * if the CONNECT and SNI both contain different domains the specification says SSL MUST terminate with a specified reject error. Which specification? RFC 2818 does not mention CONNECT. It contains some requirements for HTTP clients, but Squid is not an HTTP client in this issue scope; Squid is a TCP client. Sorry the specifics here are mostly in RFC 6066 section 3, the final two paragraphs. http://tools.ietf.org/html/rfc6066#section-3 If an application negotiates a server name using an application protocol and then upgrades to TLS, and if a server_name extension is sent, then the extension SHOULD contain the same name that was negotiated in the application protocol. If the server_name is established in the TLS session handshake, the client SHOULD NOT attempt to request a different server name at the application layer. ie CONNECT with hostname contains the same value as SNI from the client. RFC 2818 section 3.1 leads into that with its paragraph 2, If the client has external information as to the expected identity of the server, the hostname check MAY be omitted. ... In such cases, it is important to narrow the scope of acceptable certificates as much as possible in order to prevent man in the middle attacks. In special cases, it may be appropriate for the client to simply ignore the server's identity, but it must be understood that this leaves the connection open to active attack. such narrowing can be Squid doing the check that CONNECT == SNI and aborting if not. My emphasis is in doing what we can to avoid that risk indicated in the final line. At the first stages of the ssl bumping , I remember that there was cases where the browser used the IP address in CONNECT request and not the hostname. I do not know if there are still browsers which uses the same form. I covered that. We ignore raw-IP in CONNECT. The vulnerability is when the client sending false CONNECT URI/domain gets past security permissions. With raw-IP that does not work nearly so easily. I must note that the SSL-bumping is not covered by any RFC. It is not something it should happen, because IT IS a man in the middle. Which is precisely why we should be careful to be well behaved and do what verification we can to avoid bad behaviour happening over the proxy. We cannot catch every case, but preventing even some cases is a good increase in security. The proxy is not expected to validate the client HELLO SNI information, with the CONNECT hostname, should not interposed into the client/server negotiation. In the case of a CONNECT we are essentially pretending that SSL-Bump was an Upgrade request: If an application negotiates a server name using an application protocol and then upgrades to TLS, then the extension SHOULD contain the same name that was negotiated in the application protocol. (http://tools.ietf.org/html/rfc6066#section-3) All I am saying is that we should / SHOULD enforce that requirement in the spirit of the standard even though technically MITM cases are officially not mentioned by any IETF document. The browsers can use either the IP or hostname in CONNECT string, nothing prevents them for doing this. Indeed. Its the hostname
Re: [squid-dev] [PATCH] splicing resumed sessions
On 18/03/2015 6:21 a.m., Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. The RFC 2818 has mandatory validation of the HTTP CONNECT, client SNI (with server ack), and server certificate dNSname values before session resume may be performed. I dont think we have any such validation in Squid yet. So the current behaviour of terminate is mandatory for compliance with TLS. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
On 18/03/2015 6:21 a.m., Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. After these changes, Squid either terminates or splices resuming sessions, depending on configuration. Splicing is the right default because Squid most likely has spliced the original connections that the client and server are trying to resume now. Most likely, the splicing decision would not change now (but the lack of the server certificate information means we cannot repeat the original ACL checks and need a special directive to tell Squid what to do). Also, without SslBump, session resumption would just work, and SslBump default should approach that ideal. In many deployment scenarios, this straightforward splice or terminate resuming sessions implementation is exactly what the admin wants. Future projects may add more complex algorithms, including maintaining an SMP-shared cache of sessions that may be resumed in the future and evaluating client/server attempts to resume a session using that cache. Example: # splice all resuming sessions [this is the default] ssl_bump_resuming_sessions allow all This patch also makes SSL client Hello message parsing more robust and adds an SSL server Hello message parser. This patch also prevents occasional segfaults when dealing with SSL cache_peer negotiation failures. The last two changes should applied to squid-3.5 even if this patch will not go into squid-3.5. As discussed in the other branch of this thread, please remove the access control directive until its actually required. I think the whole thing can go back to 3.5 if its applying cleanly. in src/ssl/bio.cc * has many places with unnecessary whitespace additions. Such as size ), size), and sessIdLen); in src/ssl/bio.h * parseV3ServerHello() documented as what is a v3 server Hello A message. ? - looks like the 'A' is misplaced, or there is a RFC reference missing that would explain better. +1. Otherwise, looks good to me. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
On 21/03/2015 4:35 a.m., Alex Rousskov wrote: On 03/20/2015 02:06 AM, Amos Jeffries wrote: On 18/03/2015 6:21 a.m., Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. The RFC 2818 has mandatory validation of the HTTP CONNECT, client SNI (with server ack), and server certificate dNSname values before session resume may be performed. I am afraid you have misunderstood the problem domain because RFC 2818 is irrelevant here. That RFC is about sending HTTP over TLS. Here, Squid does not establish a secure HTTP connection with the client or server so Squid is not subject to that RFC (the client and server are). Without the patch, a client and server cannot resume secure sessions. Users suffer from disconnects and various associated errors even though Squid does not bump their connections. With the patch, things work, and work without violating any TLS/SSL rules. Moreover, an admin can now explicitly control whether resumed sessions are allowed or not (the default allow is what nearly everybody should want). Squid is behaving wrong right now. Fix that. I dont see any need for the new directive to exist because Squid should be resuming by default and terminate the SSL only when the domain checks which are possible confirm bad behaviour. It sounds a bit like a fail open, but is actually an opportunistic terminate [see below]. I dont think we have any such validation in Squid yet. So the current behaviour of terminate is mandatory for compliance with TLS. As Christos has said, proper validation is impossible in this case even if we wanted to do it: Squid most likely have already spliced the previous TLS session (a compliant behavior!) and now does not have enough information to validate the new connection to the server (because the server does not send its certificate during session resumption) and splices the connections again. I think we are talking past each other here again, and again its the word validation. Previous connection has nothing to do with it. HTTP messages inside the previous connection have nothing to do with it. HTTP, TLS, and SSL RFCs do not apply to traffic on spliced connections. When splicing, Squid works as a TCP proxy. The fact that Squid may have peeked into the connections (before deciding to splice them) is irrelevant because that peeking did not affect the security of those connections. The validation I'm talking about is the spec requirement that we dont allow the following scenario: CONNECT example.com:443 HTTP/1.1 ... ClientHello SNI: example.org * if the CONNECT contains a raw-IP we can ignore it. - this includes our fake-CONNECT on intercept. * if the SNI is missing we can ignore it. * if the CONNECT and SNI both contain different domains the specification says SSL MUST terminate with a specified reject error. - CONNECT having a domain means we are in the RFC 2818 scenario of TLS-over-HTTP. - checking and terminating ourselves helps protect naive/old servers against SNI abuses. - given what Squid is doing, we are allowed to just splice always and let the endpoints do the error stuff. BUT, that is a waste of resources when we could abort early and log a nice alert. In all other cases we can allow the splice to resume with no loss of safety. The endpoints will do (or not) anything they would do anyway without the proxy. So ... if we allow an admin directive it would be to force terminate when resume was otherwise allowed. 1) Do you have any existing need for that? 2) Would not ssl_bump terminate blah be a better way to express the policy? I think we should fix the currently broken Squid behaviour without a new directive. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] splicing resumed sessions
On 03/20/2015 12:11 PM, Amos Jeffries wrote: On 21/03/2015 4:35 a.m., Alex Rousskov wrote: On 03/20/2015 02:06 AM, Amos Jeffries wrote: On 18/03/2015 6:21 a.m., Tsantilas Christos wrote: This patch adds the ssl_bump_resuming_sessions directive that controls SslBump behavior when dealing with resuming SSL/TLS sessions. Without these changes, SslBump usually terminates all resuming sessions with an error because such sessions do not include server certificates, preventing Squid from successfully validating the server identity. Squid is behaving wrong right now. Fix that. Yes, the proposed patch fixes that. I dont see any need for the new directive to exist We can remove the configuration option. The option was added to minimize the chance that you will object to the patch because, without the new option, the old terminate behavior would be lost. Clearly, my plan to avoid that discussion has failed! Removing the configuration option is totally fine with me because nobody has asked for it [yet]. We can always add it later if needed. The validation I'm talking about is the spec requirement that we dont allow the following scenario: CONNECT example.com:443 HTTP/1.1 ... ClientHello SNI: example.org I still think this part of the discussion is out of scope, but is there really a spec requirement that talks about comparing CONNECT- and SNI-provided server names? Where? CONNECT is about blind TCP *tunnels*. SNI is about becoming an *end point* of a TLS connection. Those two things seem mutually exclusive to me. * if the CONNECT and SNI both contain different domains the specification says SSL MUST terminate with a specified reject error. Which specification? RFC 2818 does not mention CONNECT. It contains some requirements for HTTP clients, but Squid is not an HTTP client in this issue scope; Squid is a TCP client. - CONNECT having a domain means we are in the RFC 2818 scenario of TLS-over-HTTP. No, CONNECT means establish a TCP tunnel by splicing client and server TCP connections together. CONNECT does not imply SSL or TLS. When Squid bumps a secure connection, then Squid becomes subject to various SSL and TLS rules. Here, we are not bumping anything, just splicing at TCP level. - given what Squid is doing, we are allowed to just splice always and let the endpoints do the error stuff. BUT, that is a waste of resources when we could abort early and log a nice alert. Yes, there are cases where doing error stuff in Squid is useful, and there are cases where it is harmful. There are cases where logging a nice alert is useful and cases where it is at best pointless (unless the client gets an error message as well). This patch does not try to improve any of that doing error stuff. I am certainly not against adding [more] SNI checks, but not as a part of the fix for an SNI-unrelated error. In all other cases we can allow the splice to resume with no loss of safety. The endpoints will do (or not) anything they would do anyway without the proxy. I do not think the proposed changes _introduce_ a safety loss in *any* case. They just make session resumption work in the presence of a splicing Squid. Could Squid check more things? Yes! Are those additional checks both trivial to add and relevant to this fix? No. So ... if we allow an admin directive it would be to force terminate when resume was otherwise allowed. Yes. 1) Do you have any existing need for that? None other than avoiding this discussion, which has already failed to work. 2) Would not ssl_bump terminate blah be a better way to express the policy? Probably not. We have considered that design, but most ssl_bump rules rely on information unavailable when sessions are resumed. If we force admins to use ssl_bump to control which resuming sessions are terminated, the ssl_bump rules become needlessly complex and difficult to write correctly. I think we should fix the currently broken Squid behaviour without a new directive. Sounds good to me! If we do not hear otherwise, we will commit the proposed patch without the new [already optional] directive. Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
