Re: [PATCH] Fix IV check and padding removal.

2013-02-12 Thread Ben Laurie 
On 11 February 2013 13:19, David Woodhouse dw...@infradead.org wrote:
 On Mon, 2013-02-11 at 20:59 +, David Woodhouse wrote:
  From 32cc2479b473c49ce869e57fded7e9a77b695c0d Mon Sep 17 00:00:00 2001
  From: Dr. Stephen Henson st...@openssl.org
  Date: Thu, 7 Feb 2013 21:06:37 +
  Subject: [PATCH] Fix IV check and padding removal.
 ...
 +   if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)


 That's redundant, isn't it? DTLS1_VERSION (0xfeff) is greater than
 TLS1_1_version (0x302) anyway.

 DTLS1_BAD_VER isn't though. Changing the DTLS1_VERSION to DTLS1_BAD_VER
 makes OpenConnect work again...

 diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c
 index 2e93657..1db1d8c 100644
 --- a/ssl/s3_cbc.c
 +++ b/ssl/s3_cbc.c
 @@ -146,7 +146,7 @@ int tls1_cbc_remove_padding(const SSL* s,
   unsigned padding_length, good, to_check, i;
   const unsigned overhead = 1 /* padding length byte */ + mac_size;
   /* Check if version requires explicit IV */
 - if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)
 + if (s-version = TLS1_1_VERSION || s-version == DTLS1_BAD_VER)
   {
   /* These lengths are all public so we can test them in
* non-constant time.

 Ah, it looks like you only moved the offending code; it was actually
 Ben's fault in commit 9f27de17 / 014265eb.

Gah! I wish tests would pick up stuff like this!

 (I'm so happy you finally moved to git :)

 --
 dwmw2

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

Re: [PATCH] Fix IV check and padding removal.

2013-02-11 Thread David Woodhouse 
On Mon, 2013-02-11 at 20:59 +, David Woodhouse wrote:
  From 32cc2479b473c49ce869e57fded7e9a77b695c0d Mon Sep 17 00:00:00 2001
  From: Dr. Stephen Henson st...@openssl.org
  Date: Thu, 7 Feb 2013 21:06:37 +
  Subject: [PATCH] Fix IV check and padding removal.
 ...
 +   if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)
 
 
 That's redundant, isn't it? DTLS1_VERSION (0xfeff) is greater than
 TLS1_1_version (0x302) anyway.
 
 DTLS1_BAD_VER isn't though. Changing the DTLS1_VERSION to DTLS1_BAD_VER
 makes OpenConnect work again...
 
 diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c
 index 2e93657..1db1d8c 100644
 --- a/ssl/s3_cbc.c
 +++ b/ssl/s3_cbc.c
 @@ -146,7 +146,7 @@ int tls1_cbc_remove_padding(const SSL* s,
   unsigned padding_length, good, to_check, i;
   const unsigned overhead = 1 /* padding length byte */ + mac_size;
   /* Check if version requires explicit IV */
 - if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)
 + if (s-version = TLS1_1_VERSION || s-version == DTLS1_BAD_VER)
   {
   /* These lengths are all public so we can test them in
* non-constant time.

Ah, it looks like you only moved the offending code; it was actually
Ben's fault in commit 9f27de17 / 014265eb.

(I'm so happy you finally moved to git :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] Fix IV check and padding removal.

2013-02-11 Thread David Woodhouse 
 From 32cc2479b473c49ce869e57fded7e9a77b695c0d Mon Sep 17 00:00:00 2001
 From: Dr. Stephen Henson st...@openssl.org
 Date: Thu, 7 Feb 2013 21:06:37 +
 Subject: [PATCH] Fix IV check and padding removal.
...
+   if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)


That's redundant, isn't it? DTLS1_VERSION (0xfeff) is greater than
TLS1_1_version (0x302) anyway.

DTLS1_BAD_VER isn't though. Changing the DTLS1_VERSION to DTLS1_BAD_VER
makes OpenConnect work again...

diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c
index 2e93657..1db1d8c 100644
--- a/ssl/s3_cbc.c
+++ b/ssl/s3_cbc.c
@@ -146,7 +146,7 @@ int tls1_cbc_remove_padding(const SSL* s,
unsigned padding_length, good, to_check, i;
const unsigned overhead = 1 /* padding length byte */ + mac_size;
/* Check if version requires explicit IV */
-   if (s-version = TLS1_1_VERSION || s-version == DTLS1_VERSION)
+   if (s-version = TLS1_1_VERSION || s-version == DTLS1_BAD_VER)
{
/* These lengths are all public so we can test them in
 * non-constant time.


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] Fix IV check and padding removal.

2013-02-11 Thread David Woodhouse 
On Mon, 2013-02-11 at 13:24 -0800, Ben Laurie wrote:
  Ah, it looks like you only moved the offending code; it was actually
  Ben's fault in commit 9f27de17 / 014265eb.
 
 Gah! I wish tests would pick up stuff like this!

As far as I'm aware there are no tests for DTLS1_BAD_VER. Apart from my
users :)

We don't even have server-side support for it in OpenSSL any more.

It's only kept around for compatibility with Cisco AnyConnect, and I
don't think *they're* doing anything at all towards its upkeep — and
although they could happily run DTLS1.0 in parallel on the server and
have a migration path to sanity, they aren't doing that *either*.

However, we do *only* ever use it in 'session resume' mode. The
session-id and master secret are negotiated out-of-band with the
authentication, over TCP. When I was introducing SSL_OP_CISCO_ANYCONNECT
and subsequently hacking on stuff to find breakage, I've hard-coded
those and the random number generator and have at least used basic
replay techniques for sanity checking. I ended up with stuff like
http://david.woodhou.se/dtls-test.c

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] Fix IV check and padding removal.

2013-02-11 Thread David Woodhouse 
Same fix for 1.0.0 branch:

diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c
index 5b3f371..61413b8 100644
--- a/ssl/s3_cbc.c
+++ b/ssl/s3_cbc.c
@@ -148,7 +148,7 @@ int tls1_cbc_remove_padding(const SSL* s,
unsigned padding_length, good, to_check, i;
const unsigned overhead = 1 /* padding length byte */ + mac_size;
/* Check if version requires explicit IV */
-   if (s-version == DTLS1_VERSION)
+   if (s-version == DTLS1_VERSION || s-version == DTLS1_BAD_VER)
{
/* These lengths are all public so we can test them in
 * non-constant time.


-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature