Fix ec point formats by mattcaswell · Pull Request #2153 · openssl/openssl

mattcaswell · 2016-12-28T15:11:45Z

Checklist

tests are added or updated
CLA is signed

Description of change

Fix the EC point formats extension. This should be sent in the ServerHello if an EC based ciphersuite is
negotiated (<= TLS1.2). The relevant flag to do this was missed off in the recent extensions refactor. This bug should have been caught by 70-test_sslmessages but that test never tries an EC ciphersuite, so we also update the test to do that.

Fixes #2133

This should be sent in the ServerHello if a EC based ciphersuite is negotiated. The relevant flag to do this was missed off in the recent extensions refactor. Fixes GitHub Issue openssl#2133

richsalz · 2016-12-28T15:47:49Z

util/TLSProxy/Message.pm

should this match the name in ssl.h?

None of the other constants in this file match the equivalents in the C. To me this is a fairly intuitive name (it more or less matches the OpenSSL "string" name of the ciphersuite (except "_" not "-" and a "CIPHER_" prefix)).

richsalz · 2016-12-28T15:48:38Z

util/TLSProxy/Proxy.pm

class or self? is this line needed?

class. $ciphersuite is a class variable (i.e. a module global variable), not an instance variable. It's the same as is_tls13 about 10 lines up.

levitte · 2016-12-28T16:39:52Z

test/recipes/70-test_sslmessages.t

You should wrap this with a diabled("ec") check...

In #2154 I disable the whole test for no-ec. I think its better than adding lots of complexity to the test to deal with this option.

It's not a whole lot of complexity, here's how you do it:

(disabled("ec") ? () : [TLSProxy::Message::MT_SERVER_KEY_EXCHANGE, checkhandshake::EC_HANDSHAKE]),

On, reflection I think you are right, and your suggested approach is easier than I thought. I have done the above and also moved the commits from #2154 into this one as they are too closely related to deal with them separately.

levitte · 2016-12-28T16:40:02Z

test/recipes/70-test_sslmessages.t

... and this too

This isn't actually necessary because we skip the test that uses this information anyway.

levitte · 2016-12-28T16:41:46Z

test/recipes/70-test_sslmessages.t

Wrap this with:

SKIP: { skip "No EC support in this OpenSSL build", 1 if disabled("ec"); # original code here }

FdaSilvaYY · 2016-12-28T17:50:07Z

FYI, the no-ec option is broken.
Some missing #if guards in the new extension-handling code.

mattcaswell · 2016-12-28T22:48:31Z

FYI, the no-ec option is broken.
Some missing #if guards in the new extension-handling code.

Yeah, I know. I'm working on it.

…pect The previous commit fixed a bug where the EC point formats extensions did not appear in the ServerHello. This should have been caught by 70-test_sslmessages but that test never tries an EC ciphersuite. This updates the test to do that.

Previously we were omitting the extension information from ext_defs if the association no- option was defined. This doesn't work because the indexes into the table are no longer valid.

The CT tests in test_sslmessages require EC to be available, therefore we must skip these if no-ec

mattcaswell · 2016-12-29T10:52:40Z

Fixed issues above. New commits added taken from #2154. Also added a new commit to deal with a hang in test_sslmessages when using no-ec. This is because the ct tests in that file require ec to be available, so I just disabled those in the event of no-ec.

levitte · 2016-12-29T11:11:42Z

test/recipes/70-test_sslmessages.t

+    (disabled("ec") ? () :
+                      [TLSProxy::Message::MT_CLIENT_HELLO,
+                       TLSProxy::Message::EXT_EC_POINT_FORMATS,
+                       checkhandshake::DEFAULT_EXTENSIONS]),


Side comment: that can be written like this as well:

(disabled("ec") ? () : ([TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_SUPPORTED_GROUPS, checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_EC_POINT_FORMATS, checkhandshake::DEFAULT_EXTENSIONS])),

This is because lists of this kind are flattened in Perl. Also, I'm not 100% sure the outer parens are strictly needed.

But, this is a preference and your choice to make.

Actually, I did think about doing it that way, but made a deliberate decision not to. Either way is valid, but supported_groups may include non ec groups in the not too distant future if we implement support for DHE groups, so I decided to have separate expressions for them.

levitte · 2016-12-29T11:13:48Z

test/recipes/70-test_sslmessages.t

+SKIP: {
+    skip "No EC support in this OpenSSL build", 1 if disabled("ec");
+    $proxy->clear();
+    $proxy->clientflags("-no_tls1_3");


Is that flag strictly necessary? I would prefer if it's avoided as much as possible. My experience with sslvertol has shown that it's not as necessary as it was once...

Yes, it is very much necessary. Without it TLSv1.3 would be negotiated and then the connection would fail due to a "no shared cipher". WRT sslvertol, I think you have a misunderstanding and I will comment on the relevant PR.

This should be sent in the ServerHello if a EC based ciphersuite is negotiated. The relevant flag to do this was missed off in the recent extensions refactor. Fixes GitHub Issue #2133 Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

…pect The previous commit fixed a bug where the EC point formats extensions did not appear in the ServerHello. This should have been caught by 70-test_sslmessages but that test never tries an EC ciphersuite. This updates the test to do that. Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

Previously we were omitting the extension information from ext_defs if the association no- option was defined. This doesn't work because the indexes into the table are no longer valid. Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

The CT tests in test_sslmessages require EC to be available, therefore we must skip these if no-ec Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

mattcaswell · 2016-12-29T13:39:40Z

Pushed. Thanks.

See: openssl/openssl#2153

Fix the EC point formats extension

663b57c

This should be sent in the ServerHello if a EC based ciphersuite is negotiated. The relevant flag to do this was missed off in the recent extensions refactor. Fixes GitHub Issue openssl#2133

mattcaswell added the branch: master Applies to master branch label Dec 28, 2016

This was referenced Dec 28, 2016

Regression: "bad extension" when connecting to httpbin.org:443 #2133

Closed

Add missing flag to EC points format extension #2136

Closed

richsalz approved these changes Dec 28, 2016

View reviewed changes

levitte requested changes Dec 28, 2016

View reviewed changes

mattcaswell added 4 commits December 29, 2016 10:30

Fix extension for various no- options

3d17332

Previously we were omitting the extension information from ext_defs if the association no- option was defined. This doesn't work because the indexes into the table are no longer valid.

Fix compilation with no-ec

9b999a4

Fix CT test_sslmessages hangs

d48ceb3

The CT tests in test_sslmessages require EC to be available, therefore we must skip these if no-ec

mattcaswell mentioned this pull request Dec 29, 2016

Fix no-ec #2154

Closed

2 tasks

mattcaswell force-pushed the fix-ec-point-formats branch from dd15a5a to d48ceb3 Compare December 29, 2016 10:51

levitte reviewed Dec 29, 2016

View reviewed changes

levitte approved these changes Dec 29, 2016

View reviewed changes

levitte pushed a commit that referenced this pull request Dec 29, 2016

Fix compilation with no-ec

3cf96e8

Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

levitte pushed a commit that referenced this pull request Dec 29, 2016

Fix CT test_sslmessages hangs

0a6793c

The CT tests in test_sslmessages require EC to be available, therefore we must skip these if no-ec Reviewed-by: Richard Levitte <[email protected]> (Merged from #2153)

mattcaswell closed this Dec 29, 2016

yan12125 added a commit to yan12125/python3-android that referenced this pull request Dec 29, 2016

openssl: Patch landed upstream

da6757d

See: openssl/openssl#2153

mattcaswell mentioned this pull request Feb 6, 2020

Add a minimal build target for Travis and Appveyor #9982

Closed

Uh oh!

Comments

Conversation

mattcaswell commented Dec 28, 2016

Checklist

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell Dec 28, 2016 • edited by levitte Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

FdaSilvaYY commented Dec 28, 2016

Uh oh!

mattcaswell commented Dec 28, 2016

Uh oh!

mattcaswell commented Dec 29, 2016

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!

mattcaswell commented Dec 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattcaswell Dec 28, 2016 •

edited by levitte

Loading