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

[David Woodhouse]

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.


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...

[David Woodhouse]

Post by David Woodhouse

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,

DTLS handshake failed: 1
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 */

Post by David Woodhouse
Having said that, reverting this change isn't *sufficient* to fix
OpenSSL 1.1; it still fails with 
DTLS handshake failed: 1
... 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:

[Viktor Dukhovni]

Post by David Woodhouse

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

Yes, unfortunately, the DTLS "bad" version of 0x0100 looks like a
very high DTLS version. So comparisons require a special case.
Given that DTLS1_VERSION is 0xFEFF, indeed the next "lower" version

Post by David Woodhouse
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))

Perhaps rename dtls_ver_cmp() to dtls_ver_ordinal(), "cmp" suggests
that you're actually doing a comparison. Given this macro, one
might consider complementing the versions, so that the ordinals
compare in the usual way:

#define dtls_ver_ordinal(v) (((v) == DTLS1_BAD_VER) ? 0x00ff : (0xffff ^ (v)))

[David Woodhouse]

Post by Viktor Dukhovni
Perhaps rename dtls_ver_cmp() to dtls_ver_ordinal(), "cmp" suggests
that you're actually doing a comparison. 

Well, 'cmp' with one argument isn't *so* easily interpreted as a
comparison, but OK :)

I've also added a comment explaining a little about what's going on.

Post by Viktor Dukhovni
Given this macro, one
might consider complementing the versions, so that the ordinals
    #define dtls_ver_ordinal(v) (((v) == DTLS1_BAD_VER) ? 0x00ff : (0xffff ^ (v)))

I suppose we can, if someone feels strongly about it. It didn't seem
worth the additional churn.

One of 4 patches in https://github.com/openssl/openssl/pull/1296 which
actually make OpenConnect work again...

[Kurt Roeckx]

Post by David Woodhouse
DTLS handshake failed: 1
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.

Can you describe how DTLS1_BAD_VER is supposed to work? Is this
version send over the wire? Is it negotiated?

We have no test suite coverage doing anything with DTLS1_BAD_VER
and I think the OpenConnect VPN is the only user of it.


Kurt

[David Woodhouse]

Can you describe how DTLS1_BAD_VER is supposed to work?  Is this
version send over the wire?  Is it negotiated?

It does indeed appear on the wire.

In the AnyConnect/OpenConnect case — which, as you rightly observe, is
the only remaining user of this version of the protocol — it's not
actually negotiated in the normal sense at all; we "resume" a session
having established the master secret and session-id over a separate
channel.

http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/dtls.c#l157

We have no test suite coverage doing anything with DTLS1_BAD_VER
and I think the OpenConnect VPN is the only user of it.

Yeah, test coverage would be useful... I'm not sure how complete our
*server* side support of DTLS1_BAD_VER is. I did start looking at it
briefly once, but got distracted. I'll have another look.

[David Woodhouse]

Post by Kurt Roeckx
We have no test suite coverage doing anything with DTLS1_BAD_VER
and I think the OpenConnect VPN is the only user of it.

I added a basic test in PR #1296. It just simulates the basic session
resume and — since it seemed relatively trivial to add while I was at
it — out-of-order packet RX:
https://github.com/openssl/openssl/pull/1296/commits/9538be65

This test catches all the bugs that the pull request fixes, and also
tests the session resume method that OpenConnect uses, of manually
building the ASN.1 with the session details and then using
d2i_SSL_SESSION().

It validates the handshake MAC, which is different for DTLS1_BAD_VER
because it doesn't include the handshake message headers.

It also checks the handling of the 3-byte Change Cipher Spec message,
in both directions.

I'm currently trying to stop it whining about DTLSv1_client_method()
being deprecated; I can't see how to make it work using
DTLS_client_method().

[David Woodhouse]

This post might be inappropriate. Click to display it.