backport SSL_OP_NO_ENCRYPT_THEN_MAC to 1.1.0 by kaduk · Pull Request #3922 · openssl/openssl

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

As well as the tests for mac-then-encrypt that Emilia added.

I resolved a couple conflicts (1.1.0 disallows ETM entirely for DTLS already, so the conditional changes, etc.) and moved the CHANGES entry to reflect the new reality.

We're seeing reports that WebSphere 8.x can't handle ETM at all (regardless of whether empty extensions are last). :(

Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (cherry picked from commit cde6145)

tmshort · 2017-07-13T16:10:05Z

CHANGES

Spacing is off.

Verify that the encrypt-then-mac negotiation is handled correctly. Additionally, when compiled with no-asm, this test ensures coverage for the constant-time MAC copying code in ssl3_cbc_copy_mac. The proxy-based CBC padding test covers that as well but it's nevertheless better to have an explicit handshake test for mac-then-encrypt. Reviewed-by: Andy Polyakov <[email protected]> (cherry picked from commit b3618f4)

kaduk · 2017-07-13T19:03:55Z

@openssl/omc any advice on how to work around the supposed lack of CLA for dwwm2's Intel address?

richsalz · 2017-07-13T20:31:25Z

He is no longer at Intel. We have Intel’s CCLA with his address. Treat it as covered.

mattcaswell · 2017-07-14T09:03:54Z

The git hooks might complain when you try and push it though. We might need to temporarily add him back into the CLA db while you do the push.

kaduk · 2017-07-21T13:55:39Z

I will remove the need-cla label from github for the time being, then.
(I do not see anything that looks like an OMC +1 yet.)

kaduk · 2017-09-07T23:59:00Z

ping @openssl/omc to review this backport

t-j-h · 2017-09-08T00:06:59Z

This looks good to me - @mattcaswell ?

mattcaswell · 2017-09-08T08:49:27Z

Remind me why we are backporting a feature? I understood that the original Websphere motivation was no longer relevant.

kaduk · 2017-09-08T14:39:58Z

I can tickle a Websphere bug by sending encrypt-then-mac at all, whether or not it's the last extension.
(I don't know much about the version of Websphere on the peer, etc., but it is definitely not just choking on empty final extension(s).)

kaduk · 2017-10-04T14:51:19Z

Picking up this old one again. It looks like I have an approval from @t-j-h, so I just want @mattcaswell to confirm that his question about "backporting a feature" has been adequately addressed.

mattcaswell · 2017-10-04T14:55:55Z

It doesn't seem to have been answered in the above? Am I missing it somewhere?

kaduk · 2017-10-04T14:57:51Z

I claim that if I send encrypt-then-mac in my ClientHello to a Websphere server, regardless of whether it is the last extension, Websphere sends me back malformed HTTP on the resulting connection.

mattcaswell · 2017-10-04T15:04:56Z

CHANGES

     which is the minimum version we support.
     [Richard Levitte]
+  *) Support for SSL_OP_NO_ENCRYPT_THEN_MAC in SSL_CONF_cmd.
+     [Emilia Käsper]


It seems this CHANGES entry is in the wrong place

mattcaswell · 2017-10-04T15:07:05Z

I claim that if I send encrypt-then-mac in my ClientHello to a Websphere server, regardless of whether it is the last extension, Websphere sends me back malformed HTTP on the resulting connection.

Oh, right. I had misunderstood your earlier comment.

[to be squashed] [skip ci]

kaduk · 2017-10-04T15:12:53Z

Okay, new commit (to be squashed) moving the CHANGES entry added.

mattcaswell

You may need some support from @levitte to get this committed due to the CLA issue noted above.

mattcaswell · 2017-10-04T16:10:07Z

CHANGES

  *) Ignore the '-named_curve auto' value for compatibility of applications
     with OpenSSL 1.0.2.
     [Tomas Mraz <[email protected]>]
+  *) Support for SSL_OP_NO_ENCRYPT_THEN_MAC in SSL_CONF_cmd.


Nit: we normally have a space between different CHANGES entries.

kaduk · 2017-10-04T17:29:10Z

You may need some support from @levitte to get this committed due to the CLA issue noted above.

Indeed, gitaddrev refuses to process the first commit, which I can of course work around manually, but even if I do so the server's hook is not convinced. @levitte do we want to just temporarily add David's Intel address to the DB?

levitte · 2017-10-05T07:22:03Z

Considering that David's commit is from before we started with the CLAs, I do find the proposed hack justifiable. I'll do the merge myself.

levitte · 2017-10-05T07:24:32Z

BTW, do we really need to do a gitaddrev on all the commits? Except for the last commit, they have already been reviewed...

levitte · 2017-10-05T07:25:34Z

And duh, that commit is to be squashed anyway...

levitte · 2017-10-05T07:43:28Z

Ok, the deed is done.

Merged. 619c589 and c5e8bd1

levitte · 2017-10-05T07:45:25Z

I made a false statement earlier... David's commit was made after we started with CLAs, but at a time when his Intel address was covered... Sorry for the confusion.

Add SSL_OP_NO_ENCRYPT_THEN_MAC

d4324fb

Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (cherry picked from commit cde6145)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 13, 2017

kaduk mentioned this pull request Jul 13, 2017

Add option to disable Extended Master Secret #3910

Closed

2 tasks

tmshort reviewed Jul 13, 2017

View reviewed changes

CHANGES Outdated

Copy link

Contributor

tmshort Jul 13, 2017

Choose a reason for hiding this comment

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

Spacing is off.

kaduk force-pushed the mte branch from 951b800 to a181de6 Compare July 13, 2017 16:12

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 13, 2017

kaduk added 1.1.0 and removed hold: cla required The contributor needs to submit a license agreement labels Jul 21, 2017

tmshort mentioned this pull request Oct 4, 2017

Use ChaCha only if prioritized by clnt #4436

Closed

2 tasks

mattcaswell requested changes Oct 4, 2017

View reviewed changes

Move CHANGES entry to the proper block

68f3722

[to be squashed] [skip ci]

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 4, 2017

mattcaswell approved these changes Oct 4, 2017

View reviewed changes

levitte closed this Oct 5, 2017

Uh oh!

Comments

Conversation

kaduk commented Jul 13, 2017

Uh oh!

tmshort Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

kaduk commented Jul 13, 2017

Uh oh!

richsalz commented Jul 13, 2017 via email

Uh oh!

mattcaswell commented Jul 14, 2017

Uh oh!

kaduk commented Jul 21, 2017

Uh oh!

kaduk commented Sep 7, 2017

Uh oh!

t-j-h commented Sep 8, 2017

Uh oh!

mattcaswell commented Sep 8, 2017

Uh oh!

kaduk commented Sep 8, 2017

Uh oh!

kaduk commented Oct 4, 2017

Uh oh!

mattcaswell commented Oct 4, 2017

Uh oh!

kaduk commented Oct 4, 2017

Uh oh!

mattcaswell Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Oct 4, 2017

Uh oh!

kaduk commented Oct 4, 2017

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

kaduk commented Oct 4, 2017

Uh oh!

levitte commented Oct 5, 2017

Uh oh!

levitte commented Oct 5, 2017

Uh oh!

levitte commented Oct 5, 2017

Uh oh!

levitte commented Oct 5, 2017

Uh oh!

levitte commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants