Refuse min proto version > max proto version by tiran · Pull Request #5128 · openssl/openssl

tiran · 2018-01-21T12:21:16Z

SSL_CTX_set_min_proto_version() and SSL_CTX_set_max_proto_version() as
well as their SSL equivalents now validate that the mininum protocol
version is lower or requal the maximum protocol version.

Add test cases for min/max protocol API

Closes: #5127
Signed-off-by: Christian Heimes christian@python.org

Also see #5126

SSL_CTX_set_min_proto_version() and SSL_CTX_set_max_proto_version() as well as their SSL equivalents now validate that the mininum protocol version is lower or requal the maximum protocol version. Closes: openssl#5127 Signed-off-by: Christian Heimes <christian@python.org>

Signed-off-by: Christian Heimes <christian@python.org>

kroeckx · 2018-01-21T13:45:24Z

I think checking max > min was intentionally not done at the moment of setting the variables. The problem is that depending on the old values you might need to change min before max, or the other way around.

richsalz

This is awesome! A few nits on the new test program.

richsalz · 2018-01-21T14:03:21Z

test/ssl_ctx_test.c

+    int testresult = 0;
+    version_test t = version_testdata[idx_tst];
+
+    ctx = SSL_CTX_new(TLS_server_method());


Can you use TEST_PTR for these?

richsalz · 2018-01-21T14:03:59Z

test/ssl_ctx_test.c

+
+    if (!TEST_int_eq(SSL_CTX_set_min_proto_version(ctx, t.min_version), t.min_ok))
+        goto end;
+    if (!TEST_int_eq(SSL_CTX_set_max_proto_version(ctx, t.max_version), t.max_ok))


We tend to cascade all the if tests together for this kind of thing.

Like this? It's still short circuit evaluation.

if (!TEST_int_eq(SSL_CTX_set_min_proto_version(ctx, t.min_version), t.min_ok) || !TEST_int_eq(SSL_CTX_set_max_proto_version(ctx, t.max_version), t.max_ok) || !TEST_int_eq(SSL_CTX_get_min_proto_version(ctx), t.expected_min) || !TEST_int_eq(SSL_CTX_get_max_proto_version(ctx), t.expected_max) ) goto end;

yes like that, except put the || at the start of the line and double-indent the tests and move up the close paren. See enctest.c, etc., for examples.

richsalz · 2018-01-21T14:05:19Z

ooops, meant to check 'request changes'

davidben · 2018-01-21T19:18:23Z

+1 to what @kroeckx said. The problem with this sort of thing is that it causes an interdependence between two config options, which is really messy. (See the complexities around configuring private key and certificate and when you check for consistency.)

For something like this where the check doesn't really do much useful (the handshake will just fail immediately), it's much simpler to defer to that rather than add logic everywhere.

davidben · 2018-01-21T19:29:34Z

For instance, consider this:

void SetOnlyVersion(SSL *ssl, uint16_t version) {
#if 1
  SSL_set_min_proto_version(ssl, version);
  SSL_set_max_proto_version(ssl, version);
#else
  SSL_set_max_proto_version(ssl, version);
  SSL_set_min_proto_version(ssl, version);
#endif
}

void ChangeMyMind(SSL *ssl) {
  SetOnlyVersion(ssl, TLS1_1_VERSION);  // 1
  SetOnlyVersion(ssl, TLS1_2_VERSION);  // 2
  SetOnlyVersion(ssl, TLS1_VERSION);  // 3
}

Before, either version would work fine. Versions may be temporarily out of sync and fixed later. With this PR, setting the min version first doesn't work because going from 1 to 2 breaks. Setting the max version first doesn't work because going from 2 to 3 breaks.

kaduk · 2018-01-21T19:49:02Z

This is similar to the situation in #5057 and #5065, where the configuration APIs can be applied in different orders, and as such is too soon to effectively apply sanity checks to the configured values. As such, I think this PR should be rejected.

The getters for min and max proto version wrongly passed NULL instead of 0 as third argument to SSL_ctrl() and SSL_CTX_ctrl(). The third argument is not used, but the error results in a compiler warning: warning: passing argument 3 of ‘SSL_CTX_ctrl’ makes integer from pointer without a cast [-Wint-conversion] int v = SSL_CTX_get_max_proto_version(self->ctx); See #4364 Signed-off-by: Christian Heimes <christian@python.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from #5128)

The getters for min and max proto version wrongly passed NULL instead of 0 as third argument to SSL_ctrl() and SSL_CTX_ctrl(). The third argument is not used, but the error results in a compiler warning: warning: passing argument 3 of ‘SSL_CTX_ctrl’ makes integer from pointer without a cast [-Wint-conversion] int v = SSL_CTX_get_max_proto_version(self->ctx); See #4364 Signed-off-by: Christian Heimes <christian@python.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from #5128) (cherry picked from commit 1f82eba)

kaduk · 2018-01-24T19:13:27Z

Whoops, I put some links in commits to this pull request that were supposed to go to #5126.
So, for future reference...

kroeckx · 2018-01-26T11:06:06Z

So I think we should just keep the test suite changes?

tiran · 2018-01-26T11:22:43Z

@kroeckx Sounds reasonable to me.

I'm currently at a conference. I'll attend to this PR and my other PRs either next week or the week after.

kaduk · 2018-02-28T19:55:13Z

@tiran it sounds like we are waiting for an update to trim this down to just the tes suite changes, does that match your understanding?

tiran · 2018-02-28T19:58:42Z

Yes.

I'm sorry that I haven't had time to rework my patches. I spent all my free OSS time to get Python 3.7 improvement done before final beta freeze.

kaduk · 2018-02-28T20:03:28Z

Understandable. (But note that we've only got two weeks until the openssl 1.1.1 feature freeze with the current schedule, https://www.openssl.org/policies/releasestrat.html)

mattcaswell · 2018-03-21T11:50:06Z

Ping @tiran

levitte · 2018-03-29T09:35:07Z

This needs a rebase, and code needs to be corrected (see the CI results)

levitte · 2018-03-29T09:38:30Z

Actually, looking more closely at the CI issues, I figure that a rebase will fix them as well.

Going to change to just the test cases.

levitte · 2018-06-21T08:48:40Z

I think we lost @tiran. I can take this over like I've done with others... good idea?

richsalz · 2018-06-21T14:56:19Z

Please do take it over! I gave it a try and failed.

levitte · 2018-06-21T18:36:42Z

Continued in #6553, closing this

from Refuse min proto version > max proto version openssl#5128, otherwise ssl_ctx_test / iteration 4 fails openssl@3bc53c8

This reverts commit 89a2668. The fix is not needed anymore, as the test have been changed to reflect that min is allowed to be larger than max. See link below for more information: openssl#10337

tiran added 2 commits January 21, 2018 13:17

Add test cases for min/max protocol API

0648bff

Signed-off-by: Christian Heimes <christian@python.org>

richsalz previously approved these changes Jan 21, 2018

View reviewed changes

davidben mentioned this pull request Jan 24, 2018

Replace setters and empty states with better constructors #5158

Open

mattcaswell added this to the 1.1.1 milestone Jan 24, 2018

kaduk mentioned this pull request Feb 28, 2018

set_min_proto_version() does not validate that min <= max #5127

Closed

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

levitte closed this Jun 21, 2018

kroeckx mentioned this pull request Jun 21, 2018

Refuse min proto version > max proto version (cont.) #6553

Closed

patricthysell pushed a commit to archive-br-automation-com/openssl-ar-dev that referenced this pull request Sep 7, 2020

implemented fix openssl#5128

6511228

from Refuse min proto version > max proto version openssl#5128, otherwise ssl_ctx_test / iteration 4 fails openssl@3bc53c8

liljaq pushed a commit to archive-br-automation-com/openssl-ar-dev that referenced this pull request Sep 4, 2024

implemented fix openssl#5128

9e3fec6

from Refuse min proto version > max proto version openssl#5128, otherwise ssl_ctx_test / iteration 4 fails openssl@3bc53c8

liljaq pushed a commit to archive-br-automation-com/openssl-ar-dev that referenced this pull request Sep 5, 2024

implemented fix openssl#5128

6526d47

from Refuse min proto version > max proto version openssl#5128, otherwise ssl_ctx_test / iteration 4 fails openssl@3bc53c8

liljaq pushed a commit to archive-br-automation-com/openssl-ar-dev that referenced this pull request Oct 21, 2024

implemented fix openssl#5128

89a2668

from Refuse min proto version > max proto version openssl#5128, otherwise ssl_ctx_test / iteration 4 fails openssl@3bc53c8

Uh oh!

Conversation

tiran commented Jan 21, 2018

Uh oh!

kroeckx commented Jan 21, 2018 via email

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

richsalz Jan 21, 2018

Choose a reason for hiding this comment

Uh oh!

richsalz Jan 21, 2018

Choose a reason for hiding this comment

Uh oh!

tiran Jan 21, 2018

Choose a reason for hiding this comment

Uh oh!

richsalz Jan 21, 2018

Choose a reason for hiding this comment

Uh oh!

richsalz commented Jan 21, 2018 • edited by kroeckx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidben commented Jan 21, 2018

Uh oh!

davidben commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaduk commented Jan 21, 2018

Uh oh!

kaduk commented Jan 24, 2018

Uh oh!

kroeckx commented Jan 26, 2018

Uh oh!

tiran commented Jan 26, 2018

Uh oh!

kaduk commented Feb 28, 2018

Uh oh!

tiran commented Feb 28, 2018

Uh oh!

kaduk commented Feb 28, 2018

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

levitte commented Mar 29, 2018

Uh oh!

levitte commented Mar 29, 2018

Uh oh!

levitte commented Jun 21, 2018

Uh oh!

richsalz commented Jun 21, 2018

Uh oh!

levitte commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

richsalz commented Jan 21, 2018 •

edited by kroeckx

Loading

davidben commented Jan 21, 2018 •

edited

Loading