Additional TLS min/max version sanity checks by kaduk · Pull Request #3422 · openssl/openssl

kaduk · 2017-05-10T00:13:53Z

If the application sets an identical minimum and maximum supported protocol version on an SSL/SSL_CTX, but support for that version was disabled at configure-time, there is no chance of a handshake succeeding; we should error out immediately.

(A previous version of this patchset that errored out when only one of the min/max was an unsupported version failed make test, in one of the configurations that does an exhaustive cross product of the version matrix.)

kroeckx · 2017-05-10T17:35:19Z

Is there a reason not to move this in ssl_set_version_bound() itself? It would also cover the ssl_conf case.

kroeckx · 2017-05-10T17:36:42Z

Oh, I guess you need to know the max version.

kaduk · 2017-05-10T17:37:14Z

Well, we could change the prototype of ssl_set_version_bound(), since it's just an internal helper.

kroeckx · 2017-05-10T17:38:29Z

ssl/ssl_lib.c

Please add some minimal comment at least documenting the return value.

kroeckx · 2017-05-10T17:42:31Z

ssl/ssl_lib.c

I prefer that nothing is modified in the error case, and so that you first call ssl_check_allowed_versions().

kroeckx · 2017-05-10T17:44:05Z

Should we check that a version between min and max is enabled?

kaduk · 2017-05-10T17:54:02Z

Should we check that a version between min and max is enabled?

I thought (briefly) about it, and my first reaction was that it would be a lot of complexity, especially since we have to handle the case where one or the other is 0/any. But I could put something together if you think it's worthwhile.

kroeckx · 2017-05-10T18:22:31Z

I don't think there is much point in making it more complex.

kaduk · 2017-05-10T19:27:52Z

Modified version force-pushed, to comment ssl_check_allowed_versions() and put the calls to it before anything is written to, per @kroeckx review

kroeckx · 2017-05-10T20:10:37Z

ssl/ssl_lib.c

You need to pass larg here instead of the old value.

Whoops, worrying too hard about efficient editor use and not enough about reading the logic. Thanks for spotting it.

kaduk · 2017-05-17T18:10:26Z

Anyone want to be a second reviewer here?

mattcaswell · 2017-05-17T18:13:11Z

ssl/ssl_lib.c

Is this good enough? What if min_version != max_version but both are disabled (and all versions in between)?

I actually asked the same question. It would get a complex function, and I don't see an easy way to reuse the existing functions.

This is just a case of giving an error earlier, they'd get an error later anyway.

It's a tradeoff between comprehensiveness of check and complexity of code. Handling the case where one or the other is zero gets kind of awkward. (Kurt had asked a similar question.) But let me know if you want me to put something together.

I'm not really convinced it needs to be that complicated. Check if max_version is 0 at the beginning and if so set it to TLS1_3_VERSION. Check if min_version is 0 at the beginning and if so set it to SSL3_VERSION. Then a series of checks like this:

#ifdef OPENSSL_NO_TLS1_3 if (max_version == TLS1_3_VERSION) max_version = TLS1_2_VERSION; #endif #ifdef OPENSSL_NO_TLS1_2 if (max_version == TLS1_2_VERSION) max_version = TLS1_1_VERSION; #endif ...and so on... #ifdef OPENSSL_NO_SSL3 if (max_version == SSL3_VERSION) max_version = 0; #endif if (max_version < min_version) return 0; return 1;

Although, thinking about it you need to cope with DTLS too which this PR doesn't at the moment.

kroeckx · 2017-06-11T11:09:14Z

@kaduk: Care to address Matt's comment? At least the part about DTLS.

kaduk · 2017-06-12T16:07:40Z

Yes, I'll work on an update.

richsalz · 2017-06-16T21:03:56Z

ping @kaduk for update (and no second reviewer needed now:)

kaduk · 2017-06-17T00:23:13Z

I got distracted fixing some issues I noticed while reviewing the "allow ciphersuites to change on resumption" change, sorry.

If the result of a SSL_{CTX_,}set_{min,max}_proto_version() call leaves the min and max version identical, and support for that version is compiled out of the library, return an error. Such an object has no hope of successfully completing a handshake, and this error may be easier to decipher than the resulting handshake failure.

So let that succeed.

kaduk · 2017-06-19T21:12:27Z

I think I have to detect whether we're doing DTLS or TLS and have separate checking logic for the two cases.
Also, @dwmw2 can you confirm that I should translate a default min_proto_ver to DTLS1_VERSION and not DTLS1_BAD_VER? I seem to have been confused about how it worked, since I thought it only got used as a minimum version (but test_bad_dtls uses it as a max version as well).

dwmw2 · 2017-06-19T21:30:48Z

Yes, default to DTLS1_VERSION not DTLS1_BAD_VER.

DTLS1_BAD_VER will only ever be used where it is known in advance that that's exactly what we're going to use — hence being used for max and min simultaneously. We never do in-band negotiation and settle on DTLS1_BAD_VER.

As long as bad_dtls_test is working, I think we're fine there. It's basically the only use case.

kaduk · 2017-06-28T23:18:07Z

@kroeckx this is more like a re-review than a reconfirm at this point, if you're interested.

richsalz · 2017-07-03T17:56:55Z

+1 from me.

If the result of a SSL_{CTX_,}set_{min,max}_proto_version() call leaves the min and max version identical, and support for that version is compiled out of the library, return an error. Such an object has no hope of successfully completing a handshake, and this error may be easier to decipher than the resulting handshake failure. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3422)

kaduk · 2017-07-05T18:42:15Z

c8feba7 on master.
1.1.0 and 1.0.2 lack TLS1_3_VERSION, so probably ought to get separate pull requests. (The commit cherry-picks cleanly but fails to compile.)

If the result of a SSL_{CTX_,}set_{min,max}_proto_version() call leaves the min and max version identical, and support for that version is compiled out of the library, return an error. Such an object has no hope of successfully completing a handshake, and this error may be easier to decipher than the resulting handshake failure. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from openssl#3422) (cherry picked from commit c8feba7) Updated the cherry-pick to not reference TLS1_3_VERSION, which does not exist on this branch.

kaduk force-pushed the version branch from f2334a9 to c07776d Compare May 10, 2017 00:20

kroeckx reviewed May 10, 2017

View reviewed changes

ssl/ssl_lib.c Outdated

Copy link

Member

kroeckx May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some minimal comment at least documenting the return value.

kroeckx reviewed May 10, 2017

View reviewed changes

ssl/ssl_lib.c Outdated

Copy link

Member

kroeckx May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that nothing is modified in the error case, and so that you first call ssl_check_allowed_versions().

kaduk force-pushed the version branch from c07776d to c088148 Compare May 10, 2017 19:27

kroeckx approved these changes May 10, 2017

View reviewed changes

kroeckx added branch: master Applies to master branch 1.1.0 labels May 10, 2017

kroeckx reviewed May 10, 2017

View reviewed changes

kaduk force-pushed the version branch from c088148 to 85339f8 Compare May 10, 2017 20:44

kroeckx approved these changes May 11, 2017

View reviewed changes

mattcaswell reviewed May 17, 2017

View reviewed changes

kaduk added 2 commits June 19, 2017 16:10

test_bad_dtls sets DTLS1_BAD_VER as max_version

493571d

So let that succeed.

kaduk force-pushed the version branch from 85339f8 to 493571d Compare June 19, 2017 21:10

richsalz added the approval: done This pull request has the required number of approvals label Jul 3, 2017

kaduk closed this Jul 5, 2017

kaduk removed the 1.1.0 label Jul 5, 2017

kaduk mentioned this pull request Jul 27, 2017

Procedural request[s] #3963

Closed

kaduk mentioned this pull request Sep 15, 2017

Backport some protocol-version-related changes #4376

Closed

Uh oh!

Conversation

kaduk commented May 10, 2017

Uh oh!

kroeckx commented May 10, 2017

Uh oh!

kroeckx commented May 10, 2017

Uh oh!

kaduk commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented May 10, 2017

Uh oh!

kaduk commented May 10, 2017

Uh oh!

kroeckx commented May 10, 2017

Uh oh!

kaduk commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk commented May 17, 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!

kroeckx commented Jun 11, 2017

Uh oh!

kaduk commented Jun 12, 2017

Uh oh!

richsalz commented Jun 16, 2017

Uh oh!

kaduk commented Jun 17, 2017

Uh oh!

kaduk commented Jun 19, 2017

Uh oh!

dwmw2 commented Jun 19, 2017

Uh oh!

kaduk commented Jun 28, 2017

Uh oh!

richsalz commented Jul 3, 2017

Uh oh!

kaduk commented Jul 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants