[openssl-dev] [PATCH] Add support for minimum and maximum protocol version supported by a cipher

[David Woodhouse]

On Fri, 2016-07-08 at 17:43 +0100, David Woodhouse wrote:
> On Sun, 2016-02-07 at 20:17 +0100, Kurt Roeckx wrote:
> > Reviewed-by: Viktor Dukhovni <viktor at openssl.org>
> > 
> > MR: #1595
> > ---
> >  ssl/s3_lib.c             | 534 +++++++++++++++++++++++++++++++----------------
> >  ssl/ssl_ciph.c           | 196 +++++++++--------
> >  ssl/ssl_lib.c            |   4 +-
> >  ssl/ssl_locl.h           |  21 +-
> >  ssl/ssl_txt.c            |   2 +-
> >  ssl/statem/statem_clnt.c |  18 +-
> >  ssl/statem/statem_lib.c  |   6 +-
> >  ssl/t1_lib.c             |  41 ++--
> >  8 files changed, 504 insertions(+), 318 deletions(-)
> > 
> > diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
> > index 51fb161..093ff09 100644
> > --- a/ssl/s3_lib.c
> > +++ b/ssl/s3_lib.c
> > @@ -171,7 +171,8 @@ static const SSL_CIPHER ssl3_ciphers[] = {
> >       SSL_aRSA,
> >       SSL_eNULL,
> >       SSL_MD5,
> > -     SSL_SSLV3,
> > +     SSL3_VERSION, TLS1_2_VERSION,
> > +     DTLS1_VERSION, DTLS1_2_VERSION,
> 
> This broke the OpenConnect VPN client, which now fails thus:
> 
> DTLS handshake failed: 1
> 67609664:error:141640B5:SSL routines:tls_construct_client_hello:no ciphers available:ssl/statem/statem_clnt.c:927:
> 
> I tried the naïvely obvious step of changing all instances of
> DTLS1_VERSION as the minimum, to DTLS1_BAD_VER. That didn't help.

Of course, it's because DTLS_VERSION_LT and friends are doing precisely
the opposite of what their names imply, numerically. I hesitate to call
this a 'fix' but it highlights the issue:

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index ef5eb8c..218dcce 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -259,10 +259,11 @@
                           c[1]=(unsigned char)(((l)>> 8)&0xff), \
                           c[2]=(unsigned char)(((l)    )&0xff)),c+=3)
 
-#define DTLS_VERSION_GT(v1, v2) ((v1) < (v2))
-#define DTLS_VERSION_GE(v1, v2) ((v1) <= (v2))
-#define DTLS_VERSION_LT(v1, v2) ((v1) > (v2))
-#define DTLS_VERSION_LE(v1, v2) ((v1) >= (v2))
+#define dtls_ver_cmp(v1) (((v1) == DTLS1_BAD_VER) ? 0xff00 : (v1))
+#define DTLS_VERSION_GT(v1, v2) (dtls_ver_cmp(v1) < dtls_ver_cmp(v2))
+#define DTLS_VERSION_GE(v1, v2) (dtls_ver_cmp(v1) <= dtls_ver_cmp(v2))
+#define DTLS_VERSION_LT(v1, v2) (dtls_ver_cmp(v1) > dtls_ver_cmp(v2))
+#define DTLS_VERSION_LE(v1, v2) (dtls_ver_cmp(v1) >= dtls_ver_cmp(v2))
 
 /* LOCAL STUFF */
 
> 
> Having said that, reverting this change isn't *sufficient* to fix
> OpenSSL 1.1; it still fails with 
> 
> DTLS handshake failed: 1
> 67609664:error:14160098:SSL routines:read_state_machine:excessive message size:ssl/statem/statem.c:586:
> 
> ... which goes back to before 1.1.0-pre1. I'll find that one later...

That one's simpler:

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 26c4d10..b2160cd 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -704,6 +704,8 @@ unsigned long ossl_statem_client_max_message_size(SSL *s)
             return SERVER_HELLO_DONE_MAX_LENGTH;
 
         case TLS_ST_CR_CHANGE:
+            if (s->client_version == DTLS1_BAD_VER)
+                return 3;
             return CCS_MAX_LENGTH;
 
         case TLS_ST_CR_SESSION_TICKET:
-- 
dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5760 bytes
Desc: not available
URL: <http://mta.openssl.org/pipermail/openssl-dev/attachments/20160708/bb12b160/attachment-0001.bin>