Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-17 Thread Bodo Moeller via RT 
I think from the point of view of both interoperability and security, the
original empty-fragment approach is best when a cipher using 8-byte blocks
has been negotiated (usually 3DES), while 1 / n-1 splitting is better for
interoperability and fully adequate for large block sizes (AES).

Regardless of which of these splitting techniques is chosen, we'd want it
to be enabled by default, but it always should be possible to entirely
disable this.

So I'd suggest to rename SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS to cover 1 /
n-1 splitting (e.g., SSL_OP_DONT_SPLIT_FRAGMENTS) while retaining the old
name as an alias (#define SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS
SSL_OP_DONT_SPLIT_FRAGMENTS).

To slightly simplify the code, we could stop worrying about the 8-byte
block ciphers (i.e., never use empty fragments, use 1 / n-1 splitting
instead), but while using 8-byte blocks comes with known security issues,
there's probably no good justification to make this worse [*]. Data that
Yngve Pettersen has shown actually suggests that interoperability with 3DES
implementations is better with the empty-fragment approach anyway.


[*] For a 8-byte block cipher, the 1 / n-1 splitting approach means you 56
MAC bits for randomization in the first block. With some luck (p = 2^-56),
these bits will come out as needed by the attacker. You'd typically have
other security problems too when using an 8-byte block cipher, but why add
to them?

I think from the point of view of both interoperability and security, the original empty-fragment approach is best when a cipher using 8-byte blocks has been negotiated (usually 3DES), while 1 / n-1 splitting is better for interoperability and fully adequate for large block sizes (AES).
Regardless of which of these splitting techniques is chosen, wed want it to be enabled by default, but it always should be possible to entirely disable this.So Id suggest to rename SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS to cover 1 / n-1 splitting (e.g., SSL_OP_DONT_SPLIT_FRAGMENTS) while retaining the old name as an alias (#define SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS SSL_OP_DONT_SPLIT_FRAGMENTS).
To slightly simplify the code, we could stop worrying about the 8-byte block ciphers (i.e., never use empty fragments, use 1 / n-1 splitting instead), but while using 8-byte blocks comes with known security issues, theres probably no good justification to make this worse [*]. Data that Yngve Pettersen has shown actually suggests that interoperability with 3DES implementations is better with the empty-fragment approach anyway.
[*] For a 8-byte block cipher, the 1 / n-1 splitting approach means you 56 MAC bits for randomization in the first block. With some luck (p = 2^-56), these bits will come out as needed by the attacker. Youd typically have other security problems too when using an 8-byte block cipher, but why add to them?

Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-16 Thread Tomas Mraz via RT 
On Sun, 2012-04-15 at 16:45 +0200, Andy Polyakov via RT wrote: 
  Here is an experimental patch I wrote that implements the 1/n-1
  record splitting technique for OpenSSL. I am sending it here for
  consideration by OpenSSL upstream developers.
  
  By default the 0/n split is used but in case the
  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
  record with 1/n-1.
 
 What would you [and others] say about this alternative? Non-committed,
 relative to HEAD...


The patch seems OK however it is not clear whether this change really
brings much.

The original experimental patch is not really usable as there are
already known applications which are even broken by the 1/n-1 split. So
for SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS the split cannot be done at all
anyway. Your patch will improve the compatibility of the case where
SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS is not used however I have not seen
any reports, at least in our Bugzilla, that would ask for that. So it's
just a matter of preference whether you want to change the 0/n split to
1/n-1 one.
-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
  Turkish proverb


__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-16 Thread Andy Polyakov via RT 
 Here is an experimental patch I wrote that implements the 1/n-1
 record splitting technique for OpenSSL. I am sending it here for
 consideration by OpenSSL upstream developers.

 By default the 0/n split is used but in case the
 SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
 record with 1/n-1.
 What would you [and others] say about this alternative? Non-committed,
 relative to HEAD...
 
 
 The patch seems OK however it is not clear whether this change really
 brings much.
 
 The original experimental patch is not really usable as there are
 already known applications which are even broken by the 1/n-1 split. So
 for SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS the split cannot be done at all
 anyway. Your patch will improve the compatibility of the case where
 SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS is not used however I have not seen
 any reports, at least in our Bugzilla, that would ask for that. So it's
 just a matter of preference whether you want to change the 0/n split to
 1/n-1 one.

Have you heard of *clients* that suffer from 1/n-1 split? I mean if
clients are tolerant to it, it might make sense to favor 1/n-1 on server
side. Major obstacle for 0/n used to be Microsoft TLS or in more
practical terms IE, and with 1/n-1 IE would work...

As for client side, arguably it would make things worth. I mean  if
client plays smart and implements 1/n-1 split itself depending on
situation, e.g. not when using POST, then such split in libssl would be
counterproductive.


__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-16 Thread Tomas Mraz via RT 
On Mon, 2012-04-16 at 11:49 +0200, Andy Polyakov via RT wrote: 
  Here is an experimental patch I wrote that implements the 1/n-1
  record splitting technique for OpenSSL. I am sending it here for
  consideration by OpenSSL upstream developers.
 
  By default the 0/n split is used but in case the
  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
  record with 1/n-1.
  What would you [and others] say about this alternative? Non-committed,
  relative to HEAD...
  
  
  The patch seems OK however it is not clear whether this change really
  brings much.
  
  The original experimental patch is not really usable as there are
  already known applications which are even broken by the 1/n-1 split. So
  for SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS the split cannot be done at all
  anyway. Your patch will improve the compatibility of the case where
  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS is not used however I have not seen
  any reports, at least in our Bugzilla, that would ask for that. So it's
  just a matter of preference whether you want to change the 0/n split to
  1/n-1 one.
 
 Have you heard of *clients* that suffer from 1/n-1 split? I mean if
 clients are tolerant to it, it might make sense to favor 1/n-1 on server
 side. Major obstacle for 0/n used to be Microsoft TLS or in more
 practical terms IE, and with 1/n-1 IE would work...

I did not hear about any HTTPS clients that would be intolerant of the
1/n-1 split but other TLS usage (VPN, Jabber, ?) might be different in
this respect. But I do not know of any concrete cases where the client
is intolerant of the split.

 As for client side, arguably it would make things worth. I mean  if
 client plays smart and implements 1/n-1 split itself depending on
 situation, e.g. not when using POST, then such split in libssl would be
 counterproductive.

I do not know of any client that uses libssl as TLS backend that would
do such 1/n-1 split on itself.

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
  Turkish proverb


__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-16 Thread Kurt Roeckx via RT 
On Mon, Oct 31, 2011 at 05:56:53PM +0100, Tomas Mraz via RT wrote:
 By default the 0/n split is used but in case the
 SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
 record with 1/n-1.

There are terminators that also have a problem with this 1/n-1
splitting.  You might want to read this for instance:
http://www.imperialviolet.org/2012/01/15/beastfollowup.html

So it would be nice to still have the option to not have either
splitting, but I think we should default to 1/n-1.


Kurt


__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2012-04-15 Thread Andy Polyakov via RT 
 Here is an experimental patch I wrote that implements the 1/n-1
 record splitting technique for OpenSSL. I am sending it here for
 consideration by OpenSSL upstream developers.
 
 By default the 0/n split is used but in case the
 SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
 record with 1/n-1.

What would you [and others] say about this alternative? Non-committed,
relative to HEAD...

Index: ssl/s3_pkt.c
===
RCS file: /e/openssl/cvs/openssl/ssl/s3_pkt.c,v
retrieving revision 1.94
diff -u -w -r1.94 s3_pkt.c
--- ssl/s3_pkt.c15 Apr 2012 14:14:22 -  1.94
+++ ssl/s3_pkt.c15 Apr 2012 14:41:08 -
@@ -685,13 +685,14 @@
/* countermeasure against known-IV weakness in CBC ciphersuites
 * (see http://www.openssl.org/~bodo/tls-cbc.txt) */
 
-   if (s-s3-need_empty_fragments  type == 
SSL3_RT_APPLICATION_DATA)
+   if (s-s3-need_empty_fragments  type == 
SSL3_RT_APPLICATION_DATA  len1)
{
/* recursive function call with 'create_empty_fragment' 
set;
 * this prepares and buffers the data for an empty 
fragment
 * (these 'prefix_len' bytes are sent out later
 * together with the actual payload) */
-   prefix_len = do_ssl3_write(s, type, buf, 0, 1);
+   prefix_len = do_ssl3_write(s, type, buf, 1, 1);
+   buf++, len--;
if (prefix_len = 0)
goto err;
 
@@ -827,6 +828,10 @@
 */
return wr-length;
}
+   else if (prefix_len)
+   {
+   buf--, len++;
+   }
 
/* now let's set up wb */
wb-left = prefix_len + wr-length;

[openssl.org #2635] 1/n-1 record splitting technique for CVE-2011-3389

2011-10-31 Thread Tomas Mraz via RT 
Here is an experimental patch I wrote that implements the 1/n-1
record splitting technique for OpenSSL. I am sending it here for
consideration by OpenSSL upstream developers.

By default the 0/n split is used but in case the
SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag is set, we split the first
record with 1/n-1.

Tomas Mraz

diff -up openssl-1.0.0e/ssl/s3_both.c.beast openssl-1.0.0e/ssl/s3_both.c
--- openssl-1.0.0e/ssl/s3_both.c.beast	2010-03-25 00:16:49.0 +0100
+++ openssl-1.0.0e/ssl/s3_both.c	2011-10-13 14:05:50.0 +0200
@@ -758,15 +758,12 @@ int ssl3_setup_write_buffer(SSL *s)
 	if (s-s3-wbuf.buf == NULL)
 		{
 		len = s-max_send_fragment
-			+ SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD
-			+ headerlen + align;
+			+ 2 * (SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD
+			+ headerlen + align);
 #ifndef OPENSSL_NO_COMP
 		if (!(s-options  SSL_OP_NO_COMPRESSION))
 			len += SSL3_RT_MAX_COMPRESSED_OVERHEAD;
 #endif
-		if (!(s-options  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS))
-			len += headerlen + align
-+ SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD;
 
 		if ((p=freelist_extract(s-ctx, 0, len)) == NULL)
 			goto err;
diff -up openssl-1.0.0e/ssl/s3_enc.c.beast openssl-1.0.0e/ssl/s3_enc.c
--- openssl-1.0.0e/ssl/s3_enc.c.beast	2011-09-07 14:00:41.0 +0200
+++ openssl-1.0.0e/ssl/s3_enc.c	2011-10-13 14:05:50.0 +0200
@@ -428,23 +428,20 @@ int ssl3_setup_key_block(SSL *s)
 
 	ret = ssl3_generate_key_block(s,p,num);
 
-	if (!(s-options  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS))
+	/* enable vulnerability countermeasure for CBC ciphers with
+	 * known-IV problem (http://www.openssl.org/~bodo/tls-cbc.txt)
+	 */
+	s-s3-need_empty_fragments = 1 + (s-options  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS ? 1 : 0);
+
+	if (s-session-cipher != NULL)
 		{
-		/* enable vulnerability countermeasure for CBC ciphers with
-		 * known-IV problem (http://www.openssl.org/~bodo/tls-cbc.txt)
-		 */
-		s-s3-need_empty_fragments = 1;
+		if (s-session-cipher-algorithm_enc == SSL_eNULL)
+			s-s3-need_empty_fragments = 0;
 
-		if (s-session-cipher != NULL)
-			{
-			if (s-session-cipher-algorithm_enc == SSL_eNULL)
-s-s3-need_empty_fragments = 0;
-			
 #ifndef OPENSSL_NO_RC4
-			if (s-session-cipher-algorithm_enc == SSL_RC4)
-s-s3-need_empty_fragments = 0;
+		if (s-session-cipher-algorithm_enc == SSL_RC4)
+			s-s3-need_empty_fragments = 0;
 #endif
-			}
 		}
 
 	return ret;
diff -up openssl-1.0.0e/ssl/s3_pkt.c.beast openssl-1.0.0e/ssl/s3_pkt.c
--- openssl-1.0.0e/ssl/s3_pkt.c.beast	2011-05-25 17:21:12.0 +0200
+++ openssl-1.0.0e/ssl/s3_pkt.c	2011-10-13 14:05:50.0 +0200
@@ -685,7 +685,10 @@ static int do_ssl3_write(SSL *s, int typ
 			 * this prepares and buffers the data for an empty fragment
 			 * (these 'prefix_len' bytes are sent out later
 			 * together with the actual payload) */
-			prefix_len = do_ssl3_write(s, type, buf, 0, 1);
+			prefix_len = do_ssl3_write(s, type, buf,
+		   s-s3-need_empty_fragments-1, 1);
+			buf += s-s3-need_empty_fragments-1;
+			len -= s-s3-need_empty_fragments-1;
 			if (prefix_len = 0)
 goto err;
 
diff -up openssl-1.0.0e/ssl/t1_enc.c.beast openssl-1.0.0e/ssl/t1_enc.c
--- openssl-1.0.0e/ssl/t1_enc.c.beast	2011-09-07 14:00:41.0 +0200
+++ openssl-1.0.0e/ssl/t1_enc.c	2011-10-13 14:07:55.0 +0200
@@ -608,23 +608,20 @@ printf(\nkey block\n);
 { int z; for (z=0; znum; z++) printf(%02X%c,p1[z],((z+1)%16)?' ':'\n'); }
 #endif
 
-	if (!(s-options  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS))
+	/* enable vulnerability countermeasure for CBC ciphers with
+	 * known-IV problem (http://www.openssl.org/~bodo/tls-cbc.txt)
+	 */
+	s-s3-need_empty_fragments = 1 + (s-options  SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS ? 1 : 0);
+
+	if (s-session-cipher != NULL)
 		{
-		/* enable vulnerability countermeasure for CBC ciphers with
-		 * known-IV problem (http://www.openssl.org/~bodo/tls-cbc.txt)
-		 */
-		s-s3-need_empty_fragments = 1;
+		if (s-session-cipher-algorithm_enc == SSL_eNULL)
+			s-s3-need_empty_fragments = 0;
 
-		if (s-session-cipher != NULL)
-			{
-			if (s-session-cipher-algorithm_enc == SSL_eNULL)
-s-s3-need_empty_fragments = 0;
-			
 #ifndef OPENSSL_NO_RC4
-			if (s-session-cipher-algorithm_enc == SSL_RC4)
-s-s3-need_empty_fragments = 0;
+		if (s-session-cipher-algorithm_enc == SSL_RC4)
+			s-s3-need_empty_fragments = 0;
 #endif
-			}
 		}
 		
 	ret = 1;