Add option to disable Extended Master Secret by tmshort · Pull Request #3910 · openssl/openssl

tmshort · 2017-07-11T18:42:46Z

Add SSL_OP_NO_EXTENDED_MASTER_SECRET, that can be set on either
an SSL or an SSL_CTX. When processing a ClientHello, if this flag
is set, do not indicate that the EMS TLS extension was received in
either the ssl3 object or the SSL_SESSION.

Add a commit to increase SSL options to be 64 bits on all platforms.

Checklist

documentation is added or updated
tests are added or updated

kroeckx · 2017-07-12T16:29:43Z

Can you explain the use case? I could maybe understand that a client might not want to send that extension because it might break some other implementation, but I see no point in the server not using it when the client supports it.

tmshort · 2017-07-12T17:25:14Z

Yes, it's a broken server.
I see your point, on the other hand, I don't see a reason not to offer feature parity server/client where possible (it's also a fairly small change, which doesn't help either argument).

davidben · 2017-07-12T17:59:40Z

Out of curiosity, what is the server and how is it broken? (EMS-specific, or is this extension intolerance?) We ship EMS in all our clients with no way to disable it. I don't recall seeing evidence of compatibility problems.

tmshort · 2017-07-12T18:11:19Z

Specifically, it's IBM HTTP Server for WebSphere Application Server, Version 8.5.5.7, which runs on top of Java. If we remove the EMS extension, it works; but reordering extensions might also work (but that's harder to do in 1.1.0 vs master due to code changes), and it could be related to a server not handling empty extensions at the end of the extension list correctly.

A possibly similar problem was reported on NSS:

https://bugzilla.mozilla.org/show_bug.cgi?id=1243641

Which reordered the extensions. Disabling EMS was the easiest solution vs. reordering extensions.

There's also #2669, which may be related.

kroeckx · 2017-07-12T18:44:01Z

I think you mean #2691. The other seems to be about encrypt then mac?

tmshort · 2017-07-12T18:45:35Z

Definitely related to #2691, possibly related to #2669.

kroeckx · 2017-07-12T18:45:58Z

I guess I prefer a solution that tries to avoid the problem, instead of having to disable it.

tmshort · 2017-07-12T18:49:06Z

@kroeckx, I agree. I'm looking into that now, but I have to wait for the test server to be setup.

That being said, we have use for disabling EMS; and there's also an option to disable EtM, which has similar arguments for/against the ability to disable.

tmshort · 2017-07-12T18:51:23Z

And I just found this on the IETF mailing list:

https://www.ietf.org/mail-archive/web/tls/current/msg19719.html

So, I will be looking at moving EMS, EtM and SessionTickets to not be last. As pointed above, this is easy on master, harder on 1.1.0.

tmshort · 2017-07-13T16:07:56Z

Further investigation has shown that WebSphere 8.5.5 cannot handle EtM, so porting the SSL_OP_NO_ENCRYPT_THEN_MAC option from master to 1.1.0 might be a good idea.

kaduk · 2017-07-13T16:08:40Z

backporting SSL_OP_NO_ENCRYPT_THEN_MAC

#3922 FWIW

mattcaswell · 2017-08-21T10:29:40Z

Is this resolved now by moving the sig algs extension?

tmshort · 2017-08-21T12:51:07Z

@mattcaswell the fundamental issue of WebSphere is fixed with the sigalgs changes. However, being able to disable EMS might still be a useful feature.

kaduk · 2017-09-08T00:01:27Z

@mattcaswell do you think we should leave this open or close it after the sigalg extension was moved?

tmshort · 2017-09-08T00:23:17Z

I think the change still has merit, regardless of the sigalg commit. But I'm not going to force the issue.

mattcaswell · 2017-09-08T08:53:51Z

ssl/statem/extensions_clnt.c

I'm not sure if this change is necessary. If the option is set then we will not have proposed EMS. If the server sends an unsolicited ems response then the extensions system should barf. So we should only ever get here if we proposed it - which means the option is not set.

Correct. This is just safety, but not strictly necessary.

I will point out that tls_parse_stoc_etm() - immediately above this function - does check for SSL_OP_NO_ENCRYPT_THEN_MAC. This behavior should fall under the same umbrella.

mattcaswell · 2017-09-08T08:58:13Z

test/sslapitest.c

Doesn't this function need to set the max protocol to TLSv1.2?

tmshort · 2018-01-10T20:31:27Z

Rebased

kaduk

@mattcaswell did your concerns get adequately addressed?

davidben · 2018-01-18T19:08:46Z

What's the current motivation for this change? It sounds from the thread that issues with WebSphere were since resolved.

kaduk · 2018-01-18T19:14:33Z

There are some setups such as those described in https://tools.ietf.org/html/draft-erb-lurk-rsalg-01 that are not easily adopted to be compatible with EMS.

kaduk · 2018-01-29T23:20:47Z

ping @mattcaswell ?

tmshort · 2019-02-06T16:04:29Z

Done @mattcaswell

mattcaswell · 2019-02-06T17:32:57Z

include/openssl/ssl.h

Why not just delete all these reserved values - since they are no longer reserved, i.e. master will become OpenSSL 3.0.0.

It seems helpful to have annotations to remind anyone preparing a change for backport of the constraints involved

mattcaswell · 2019-02-06T17:33:42Z

include/openssl/ssl.h

I wonder if these signature changes will result in any compilation issues for end user applications.

For (most) 64-bit platforms, it shouldn't make much of a difference; assuming unsigned long ~ uint64_t
For 32-bit platforms, there will be some issues, and users would have to cast them away, or handle the return value properly. Since all the values are in the lower 32-bits, casting won't have issues until more options are used.

It's unclear that this counts as "API compatible; e.g., I expect modern compilers to warn about the truncated return value.

Yes, compiles will warn, but it won't break anything until upper-32-bit option values are defined.

I think this warrants a discussion on openssl-project

I started to write an email to the list about this, but in putting it together I think I answered the question I was going to ask. Specificially openssl/web#82 says:

"No existing public interface can be modified except where changes are unlikely to break source compatibility."

On thinking about this I think a warning is likely to be sufficient to break some builds. Many builds treat warnings as errors. So I'm thinking that this change is probably a step too far.

My suggestion is to remove it from this PR - since really this is just an incidental tangent to the main point of this PR. If we are only targetting 3.0 then we can reuse one of the currently "reserved" SSL_OP_ values for now. We should probably also raise an issue that we're running out of SSL_OP_ values and we can figure out in a separate PR the best way of handling it.

Thanks (I should've come back to look at this earlier). Certainly have a 64-suffix version would allow for extension. This would only be an issue with 32-bit platforms. Let's say we drop those. ;) /s

Indeed, if we are only targeting 3.0.0, then we are fine re-using the bit. But, I do see a use for adding it to 1.1.1, but not critically.

This would only be an issue with 32-bit platforms. Let's say we drop those.

Technically those with a 32-bit long. This includes 64-bit Windows. :-(

I was being sarcastic! :) But I undid the 64-bit changes.

mattcaswell · 2019-02-06T17:35:56Z

test/sslapitest.c

This could be done more easily in the arguments to create_ssl_ctx_pair

Add SSL_OP64_NO_EXTENDED_MASTER_SECRET, that can be set on either an SSL or an SSL_CTX. When processing a ClientHello, if this flag is set, do not indicate that the EMS TLS extension was received in either the ssl3 object or the SSL_SESSION. Retain most of the sanity checks between the previous and current session during session resumption, but weaken the check when the current SSL object is configured to not use EMS.

tmshort · 2019-02-12T16:12:05Z

The build failure appears to be related to the sparse_array implementation.

mattcaswell · 2019-02-13T11:22:20Z

$ ./config -d --strict-warnings no-tls1_2
$ make
$ make test
...
Test Summary Report
-------------------
../test/recipes/80-test_ssl_new.t                (Wstat: 256 Tests: 30 Failed: 1)
  Failed test:  30
  Non-zero exit status: 1
Files=155, Tests=1369, 130 wallclock secs ( 1.00 usr  0.22 sys + 131.60 cusr  8.01 csys = 140.83 CPU)
Result: FAIL

tmshort · 2019-02-13T18:50:17Z

The last commit should fix the test issues.

mattcaswell · 2019-02-14T14:08:23Z

CI red crosses are unrelated.

mattcaswell · 2019-02-14T14:10:11Z

Needs 2nd review.

@openssl/committers

mattcaswell · 2019-02-15T10:15:53Z

Pushed. Thanks.

Add SSL_OP64_NO_EXTENDED_MASTER_SECRET, that can be set on either an SSL or an SSL_CTX. When processing a ClientHello, if this flag is set, do not indicate that the EMS TLS extension was received in either the ssl3 object or the SSL_SESSION. Retain most of the sanity checks between the previous and current session during session resumption, but weaken the check when the current SSL object is configured to not use EMS. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #3910)

dwmw2 · 2019-06-12T09:32:53Z

Did we conclude that we can't have SSP_OP_NO_EXTENDED_MASTER_SECRET in OpenSSL 1.1.x?

I have used it in OpenConnect (here) to avoid asking for extms when resuming the (fake) DTLS session. But that's only for OpenSSL HEAD; with OpenSSL 1.1.0 and 1.1.1 it's still requesting it in the ClientHello. The only reason I get away with this for now is because neither Cisco's server nor the free ocserv implementation actually support it. If they did, they should refuse to resume the session as discussed.

mattcaswell · 2019-06-12T10:29:41Z

Did we conclude that we can't have SSP_OP_NO_EXTENDED_MASTER_SECRET in OpenSSL 1.1.x?

Yes, that was the decision.

dwmw2 · 2019-06-12T10:50:26Z

Thanks. Are we open to... less elegant solutions such as perhaps never allowing extms for DTLS1_BAD_VER?

That's only a partial solution though; Cisco have now finally updated to support a standard version of DTLS (v1.2) at least in the virtual ASA applicances. And they're still using the session resume hack, instead of proper negotiation with PSK, as we do in the open ocserv server.

Not quite sure how to address that, other than relying on the server not to accept extms even if we do offer it. We are manually generating the ASN.1 session state so perhaps we could put something special in there like a SSL_SESS_FLAG_DISABLE_EXTMS flag, coupled with a patch along the lines of 02d405d

That wouldn't have the problems of the original patch because SSL_SESS_FLAG_DISABLE_EXTMS would be an explicit instruction from the client side, equivalent to SSL_OP_NO_EXTENDED_MASTER_SECRET. We wouldn't refuse to do extms for the general case of resuming a non-extms session, in the case where the client/server have been upgraded and can now support it.

Not sure I like it though. Better ideas?

chipitsine · 2019-11-19T08:25:47Z

Hello, I have some concern about it.

we have a lot of customers with CryptoPro 4.0R3 (it is russian gost implementation). CryptoPro 4.0R3 does not like EMS in server helo handshake.

so, what are we supposed to do with SSP_OP_NO_EXTENDED_MASTER_SECRET not being ported to openssl-1.1.1 ?

are we supposed to

wait for 3.0 release ?
install openssl master ?

I mean that not being ported to 1.1.1 branch is weird and left us no choice for today.
I'm open to your suggestions

beldmit · 2019-11-19T08:28:58Z

From my experience, I can confirm that some GOST implementations do not support EMS correctly, so the option (disabled by default, of cause) would be rather useful.

The option SSL_OP_NO_EXTENDED_MASTER_SECRET add in openssl#3910. And it is valid for versions below (D)TLS 1.2.

The option SSL_OP_NO_EXTENDED_MASTER_SECRET was added in #3910. And it is valid for versions below (D)TLS 1.2. Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #17597)

tmshort mentioned this pull request Jul 13, 2017

Reorder extensions to put SigAlgs last (master) #3921

Closed

1 task

mattcaswell requested changes Sep 8, 2017

View reviewed changes

tmshort force-pushed the master-no-ems branch from e309428 to dbddd9a Compare September 8, 2017 14:04

tmshort mentioned this pull request Oct 3, 2017

Use ChaCha only if prioritized by clnt #4436

Closed

2 tasks

tmshort force-pushed the master-no-ems branch from dbddd9a to 15a8aa0 Compare January 10, 2018 20:31

kaduk reviewed Jan 18, 2018

View reviewed changes

mattcaswell added this to the Post 1.1.1 milestone Jan 24, 2018

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

tmshort force-pushed the master-no-ems branch from 15a8aa0 to 56df3c9 Compare May 8, 2018 13:55

kaduk mentioned this pull request Sep 17, 2018

Extended Master Secret is sent unconditionally #7246

Closed

mattcaswell requested changes Feb 6, 2019

View reviewed changes

tmshort force-pushed the master-no-ems branch from 16ce933 to 72bef04 Compare February 12, 2019 15:33

fixup! Add option to disable Extended Master Secret

9f22291

mattcaswell approved these changes Feb 14, 2019

View reviewed changes

mattcaswell added branch: master Applies to master branch approval: review pending This pull request needs review by a committer and removed after 1.1.1 labels Feb 14, 2019

paulidale approved these changes Feb 14, 2019

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 14, 2019

mattcaswell closed this Feb 15, 2019

tmshort deleted the master-no-ems branch July 15, 2020 18:51

hou2gou added a commit to hou2gou/openssl that referenced this pull request Jan 26, 2022

apps: Add option -no_ems to s_client/s_server command line.

7ca43e9

The option SSL_OP_NO_EXTENDED_MASTER_SECRET add in openssl#3910. And it is valid for versions below (D)TLS 1.2.

hou2gou mentioned this pull request Jan 26, 2022

apps: Add option -no_ems to s_client/s_server command line. #17597

Closed

2 tasks

Uh oh!

Comments

Conversation

tmshort commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kroeckx commented Jul 12, 2017

Uh oh!

tmshort commented Jul 12, 2017

Uh oh!

davidben commented Jul 12, 2017

Uh oh!

tmshort commented Jul 12, 2017

Uh oh!

kroeckx commented Jul 12, 2017

Uh oh!

tmshort commented Jul 12, 2017

Uh oh!

kroeckx commented Jul 12, 2017

Uh oh!

tmshort commented Jul 12, 2017

Uh oh!

tmshort commented Jul 12, 2017

Uh oh!

tmshort commented Jul 13, 2017

Uh oh!

kaduk commented Jul 13, 2017

Uh oh!

mattcaswell commented Aug 21, 2017

Uh oh!

tmshort commented Aug 21, 2017

Uh oh!

kaduk commented Sep 8, 2017

Uh oh!

tmshort commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Jan 10, 2018

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

davidben commented Jan 18, 2018

Uh oh!

kaduk commented Jan 18, 2018

Uh oh!

kaduk commented Jan 29, 2018

Uh oh!

tmshort commented Feb 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Jul 11, 2017 •

edited

Loading

tmshort commented Feb 12, 2019 •

edited

Loading