WIP: Mitigate regression errors when linking older apps with libssl 1.1.1 by levitte · Pull Request #5945 · openssl/openssl

levitte · 2018-04-13T13:39:05Z

Applications that were built for libssl 1.1.0 will most probably get
into trouble when re-linked against libssl 1.1.1. This is especially
serious when re-linking happens due to a shared library upgrade.

We try to mitigate against this with a sneaky trick, where re-linked
application will simply not have access to TLSv1.3, while programs
that are re-compiled will, through a macro that redirects a call to
SSL_CTX_new() to SSL_CTX_new_111(). For applications where access to
TLSv1.3 is undesirable (because it requires some source code changes),
it can still be disabled by defining the macro OPENSSL_DISABLE_TLSv13
when building it.

Fixes #5944, #5661

levitte · 2018-04-13T13:39:51Z

Note that it doesn't entirely fix the regressions observed in #5661. It will remain in WIP until #5661 isn't an issue any more.

mattcaswell · 2018-04-13T13:43:35Z

SSL_CTX_new() to SSL_CTX_new_111()

I'm wondering whether we could use symbol versioning to do this same thing? It wouldn't work on platforms without symbol versioning though.

richsalz · 2018-04-13T13:44:54Z

This requires a special-built 1.1.1 library which seems to defeat the purpose. Can this library be used for both 1.3 and earlier protocol versions?

I think that simply not enabling 1.3 by default is much easier to understand.

levitte · 2018-04-13T13:48:35Z

This requires a special-built 1.1.1 library which seems to defeat the purpose.

Uhmmm, no, no special building should be required. If you're thinking about OPENSSL_DISABLE_TLSv13, all it does is decide if SSL_CTX_new() gets redirected to SSL_CTX_new_111() or not.

mattcaswell

It would need some documentation and probably a FAQ entry

mattcaswell · 2018-04-13T13:48:14Z

ssl/ssl_lib.c

I can't say I'm massively fond of this name. What happens when we have 1.1.2 or 1.1.3...or...

Maybe SSL_CTX_new_ex()?

SSL_CTX_ new_tls13()?

why not SSL_CTX_new_ex?
it is not yet specified which paramters it will have.

Exactly because of that, and because I see this as an interim solution that should disappear with 1.2.0 at the latest.

levitte · 2018-04-13T13:49:46Z

I'm wondering whether we could use symbol versioning to do this same thing? It wouldn't work on platforms without symbol versioning though.

We could, but as you said, this isn't universally available, so we'll get the pleasure of maintaining two ways of doing this instead of just one. I'm not at all keen to do this.

mattcaswell · 2018-04-13T13:53:11Z

This requires a special-built 1.1.1 library which seems to defeat the purpose.

No. The same library would be able to support pre-1.1.1 apps and 1.1.1 apps. If an app was compiled against 1.1.0 headers but run against the 1.1.1 library then it will get a maximum of TLSv1.2. Apps compiled against the 1.1.1 headers would get TLSv1.3 by default unless you build the app with OPENSSL_DISABLE_TLSv13 defined, e.g. gcc -DOPENSSL_DISABLE_TLSv13 myapp.c ...

mattcaswell · 2018-04-13T13:53:42Z

include/openssl/ssl.h

Is that a lower case v? Seems not quite right when everything else is in caps.

You originally suggested DISABLE_TLSv13, with lower case v. However, considering all other macros, this should really be OPENSSL_DISABLE_TLS1_3, should it not?

Oh right...well that was just an example :-)

Yes, dropping the "v" seems best.

mattcaswell · 2018-04-13T13:58:45Z

I like this idea because it removes a significant barrier to upgrading without removing TLSv1.3 being on-by-default.

richsalz · 2018-04-13T14:02:23Z

So how about the internal API being called like this:

SSL_CTX_new_ex(..., int flags)
// or
SSL_CTX_new_ex(..., int minproto, int maxproto, int flags);

That seems more general purpose and future-proof. min or max can be "0" to mean lowest or highest supported by the library. And for 1.1.1 the highest should be explicitly set to 1.3

And, of course, that function needs to be documented.

mattcaswell · 2018-04-13T14:05:30Z

So how about the internal API being called like this:

I was thinking about that myself, but actually I think it would be wrong to encourage people to hard code a maximum protocol version into their apps.

mattcaswell · 2018-04-13T14:08:11Z

I was thinking about that myself, but actually I think it would be wrong to encourage people to hard code a maximum protocol version into their apps.

But maybe some of the other params you suggest would be ok.

richsalz · 2018-04-13T14:10:02Z

It would not be wrong. There are special-cases where someone wants fine-grain control over which protocol version. The implementation is trivial

if (minproto!=0) SSL_CTX_set_min_protocol(...

In other words, we provide functions to allow is. This just makes it more convenient.

t8m · 2018-04-13T14:14:16Z

+1 to Rich's proposal.

levitte · 2018-04-13T14:21:11Z

I was thinking that this would be an interim solution. With OpenSSL 1.2.0, SSL_CTX_new_111() and the hackery to redirect SSL_CTX_new to it would simply be removed and SSL_CTX_new() would go back to normal.

How often would you expect that people would call SSL_CTX_new_ex with a minimum and maximum version other than 0?

mattcaswell · 2018-04-13T14:34:19Z

With OpenSSL 1.2.0, SSL_CTX_new_111() and the hackery to redirect SSL_CTX_new to it would simply be removed and SSL_CTX_new() would go back to normal.

Agreed. I still think we should avoid version specific names These things have a tendency to stick around even when we don't plan them to. We should probably have a comment in the header saying something like "don't use this directly - always call SSL_CTX". Unless that is we go with Rich's proposal.

richsalz · 2018-04-13T14:35:36Z

I think the new _ex routine will become more common. I do not know how often folks will want to clamp to specific versions. I think, for example, that this library should clamp to 1.3 so that 1.4 requires a recompile.

I also know that in my $DAYJOB we have many customers who want us to limit what versions we support.

richsalz · 2018-04-13T14:36:56Z

I would like that this not get approved until we have more discussion. For example, I feel strongly that the full "ex" API is the way to go.

kaduk · 2018-04-13T14:43:53Z

SSL_CTX_new_ex()

Note that we had previous related discussions in #2397 and #4848

levitte · 2018-04-13T15:05:50Z

I would like that this not get approved until we have more discussion. For example, I feel strongly that the full "ex" API is the way to go.

Sure. Either way, I would say that the regression situation is a release show stopper.

(I'll note, btw, that this simple move greatly improves the tests that failed in #5661. Not entirely, but quite a lot)

tmshort · 2018-04-13T15:23:07Z

SSL_CTX_new_ex()

Note that we had previous related discussions in #2397 and #4848

PR #3761 is the fix for #2397, and it defines SSL_CTX_new_ex()
While I like @richsalz's idea of SSL_CTX_new_ex(), it makes changes such as #3761 more complicated to incorporate: do we add SSL_CTX_new_ex2() ?

That being said, there are ways to tag inputs to allow the number of inputs to expand without redefining the function each time (kinda like named arguments, but in C):

/* 
 * Use DONE to flag no more arguments, or redefine to include a count of
 * extra parameters
 */
#define SSL_CTX_NEW_DONE 0 
#define SSL_CTX_NEW_MIN_VERSION (1 | SSL_EX_TYPE_INT)
#define SSL_CTX_NEW_MAX_VERSION (2 | SSL_EX_TYPE_INT)
#define SSL_CTX_NEW_CACHE (3 | SSL_EX_TYPE_PTR)
#define SSL_CTX_FLAGS (4 | SSL_EX_TYPE_LONG)

SSL_CTX *SSL_CTX_new_ex(const SSL_METHOD *meth, ...)
ctx = SSL_CTX_new_ex(TLS_method(),
                     SSL_CTX_NEW_MIN_VERSION, TLS1_2_VERSION,
                     SSL_CTX_NEW_CACHE, session_cache,
                     SSL_CTX_NEW_DONE);
/* or */
SSL_CTX *SSL_CTX_new_ex(const SSL_METHOD *meth, int argc, ...)
ctx = SSL_CTX_new_ex(TLS_method(), 2,
                     SSL_CTX_NEW_MIN_VERSION, TLS1_2_VERSION,
                     SSL_CTX_NEW_CACHE, session_cache);

Edit: include a type in the argument type, so that unknown arguments can be ignored

Applications that were built for libssl 1.1.0 will most probably get into trouble when re-linked against libssl 1.1.1. This is especially serious when re-linking happens due to a shared library upgrade. We try to mitigate against this with a sneaky trick, where re-linked application will simply not have access to TLSv1.3, while programs that are re-compiled will, through a macro that redirects a call to SSL_CTX_new() to SSL_CTX_new_111(). For applications where access to TLSv1.3 is undesirable (because it requires some source code changes), it can still be disabled by defining the macro OPENSSL_DISABLE_TLSv13 when building it. Fixes openssl#5944

… 1.1.1

tmshort · 2018-04-13T16:28:47Z

Too late for now, but knowing in a library initialization function, what version the application was compiled with, might be useful.

levitte · 2018-04-13T16:33:57Z

That would require cooperation from the application, or exactly the kind of hackery this PR does...

tmshort

Maybe something like:

__owur SSL_CTX *SSL_CTX_new_ver(const SSL_METHOD *meth, int ver);
# define SSL_CTX_new(meth) SSL_CTX_new_ver(meth, OPENSSL_VERSION_NUMBER)

Then it's something that could be kept for future use?

levitte · 2018-04-14T07:02:00Z

Ah, you meant like that... That could be made part of this PR. I assume, btw, that the version passed to SSL_CTX_new_ver can be expressed as "the version we're building for", so it makes some kind of sense to have something like this:

/* Users should never call this function directly */
__owur SSL_CTX *SSL_CTX_new_ver(const SSL_METHOD *meth, int ver);
/* OPENSSL_DISABLE_TLSV1_3 is optional, defined by the user when building their programs */
#ifdef OPENSSL_DISABLE_TLS1_3
/*
 * TLSv1.3 is new in version 1.1.1, and behaves differently than earlier TLS versions.
 * When the user disables it to be able to compile older programs without having to
 * change them, we simply pretend that they build for a pre-1.1.1 version, i.e. 1.1.0
 */
# define SSL_CTX_new(meth) SSL_CTX_new_ver((meth), 0x10100000UL)
#else
# define SSL_CTX_new(meth) SSL_CTX_new_ver((meth), OPENSSL_VERSION_NUMBER)
#endif

dot-asm · 2018-04-14T08:12:41Z

include/openssl/ssl.h

+/*
+ * With TLSv1.3 enabled, we make sure that apps linked against the shared
+ * libraries of earlier OpenSSL versions don't get it by mistake, as TLSv1.3
+ * is different enough to be able to cause faults for the unknowing.


Is this actually true? Is common 1.1.0 application bound to fail if library negotiates TLS 1.3? At least 1.1.0 s_client and s_server don't. And if they don't, then why would we want to deny them the option to use the protocol? I'd say opposite is more natural. Indeed, they concentrate on application logic and we concentrate on transport mechanism. If anything, we should facilitate use on new protocol with old applications.

I've been trying to ask @mattcaswell if there's a subset of functionality that still works, and I keep getting the answer back that TLSv1.3 works differently. I take that as there not being any guarantee, and that it's pure luck that s_client and s_server work when no extra options are added (have you tried with different options? How long before it fails surprisingly?)

One of the test programs that I've been worried about is test/sslapitest. After all, it tests the behaviour of the SSL API! I'm thinking that if that one fails (i.e. that one from 1.1.0 linked with 1.1.1 libraries), we do have a regression problem.

This is, btw, a discussion that could happen in #5944

Ahm, do you mean when a program is just re-compiled then it will fail in a surprising way?

I keep getting the answer back that TLSv1.3 works differently.

It does work differently. That doesn't mean that all applications will start failing. Quite the opposite. I expect most applications to work without noticing TLSv1.3. However, there are certain scenarios where unexpected things might start happening. Some examples:

if you expect your session to have been created during the handshake then you're going to be disappointed (it happens post-handshake). This means some callbacks could happen when an application isn't expecting them.

If you're expecting a particular ciphersuite to be negotiated (because you disabled others) then that might not happen because TLSv1.3 ciphersuite configuration happens differently. This could actually cause an application to fail in some circumstances.

If your application is using custom extensions you might suddenly find them not being used when they always were before because they are not compatible with TLSv1.3

etc

There's probably a whole load of other things. Most of these problems are edge cases. What we don't know is what kind of proportion of applications will fall foul of them.

I'd like to repeat the suggestion that libssl could look at certain facts (sigalgs derived from server certs, ciphersuites, whatever else that's possible to check) and adapt the range of TLS versions to be negotiated. For example, is s_server is started with a DSA server certificate, TLSv1.2 would be the newest version negotiated, rather than selecting TLSv1.3 and then generating an error because the sigalg doesn't fit...

… 1.1.1

richsalz · 2018-04-14T14:32:51Z

Why do you keep using _ver ending when everything else has used _ex? :)

levitte · 2018-04-14T14:41:33Z

Why do you keep using _ver ending when everything else has used _ex? :)

I haven't yet (see the diff)
I followed the example.
Considering the purpose is small and well defined (_ex is more general purpose), it makes sense.

vdukhovni · 2018-04-14T20:48:22Z

I'd like to suggest that we put this PR on hold until we determine whether any of this is necessary.

levitte · 2018-04-16T09:09:25Z

The discussion on openssl-project concluded that the solution in this PR isn't the right move, so I'm closing this. Further technical discussions should happen in #5944.

levitte added branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Apr 13, 2018

mattcaswell requested changes Apr 13, 2018

View reviewed changes

mattcaswell reviewed Apr 13, 2018

View reviewed changes

kaduk mentioned this pull request Apr 13, 2018

Policy issue: TLSv1.3 makes upgrade without program modification impossible #5944

Closed

levitte added 2 commits April 13, 2018 17:56

fixup! Mitigate regression errors when linking older apps with libssl…

db692e8

… 1.1.1

levitte force-pushed the rethink-regression-fixes branch from 9a9d1aa to db692e8 Compare April 13, 2018 15:56

levitte mentioned this pull request Apr 13, 2018

Regression test (1.1.0 apps against 1.1.1-dev shared libraries) failures #5661

Closed

tmshort mentioned this pull request Apr 13, 2018

Fixes #2397 Add shared session cache in SSL_CTX #3761

Closed

2 tasks

tmshort reviewed Apr 13, 2018

View reviewed changes

dot-asm reviewed Apr 14, 2018

View reviewed changes

levitte added 2 commits April 14, 2018 10:22

fixup! Mitigate regression errors when linking older apps with libssl…

ee73100

… 1.1.1

fixup! Mitigate regression errors when linking older apps with libssl…

82ea8d2

… 1.1.1

levitte closed this Apr 16, 2018

Uh oh!

Conversation

levitte commented Apr 13, 2018

Uh oh!

levitte commented Apr 13, 2018

Uh oh!

mattcaswell commented Apr 13, 2018

Uh oh!

richsalz commented Apr 13, 2018

Uh oh!

levitte commented Apr 13, 2018

Uh oh!

mattcaswell left a comment

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!

levitte commented Apr 13, 2018

Uh oh!

mattcaswell commented Apr 13, 2018

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!

mattcaswell commented Apr 13, 2018

Uh oh!

richsalz commented Apr 13, 2018

Uh oh!

mattcaswell commented Apr 13, 2018

Uh oh!

mattcaswell commented Apr 13, 2018

Uh oh!

richsalz commented Apr 13, 2018

Uh oh!

t8m commented Apr 13, 2018

Uh oh!

levitte commented Apr 13, 2018

Uh oh!

mattcaswell commented Apr 13, 2018

Uh oh!

richsalz commented Apr 13, 2018

Uh oh!

richsalz commented Apr 13, 2018

Uh oh!

kaduk commented Apr 13, 2018

Uh oh!

levitte commented Apr 13, 2018

Uh oh!

tmshort commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmshort commented Apr 13, 2018

Uh oh!

levitte commented Apr 13, 2018

Uh oh!

tmshort left a comment

Choose a reason for hiding this comment

Uh oh!

levitte commented Apr 14, 2018

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

tmshort commented Apr 13, 2018 •

edited

Loading