TLSv1.3: Use TLSv1.3 style nonce construction (new approach) by mattcaswell · Pull Request #1947 · openssl/openssl

mattcaswell · 2016-11-17T23:29:56Z

Checklist

tests are added or updated
CLA is signed

Description of change

This is an alternative approach to that in #1940. Here we use the existing AEAD interface to libcrypto, rather than modifying the existing <=TLS1.2 cipher implementation in libcrypto. This goes slightly further than #1940 in that it actually starts to use the new nonce construction in the encryption of records. Due to the differing approach I have had to implement a new test for this as well.

davidben

With the caveat that I'm not familiar this code and can only give a cursory look, I think this is a much better approach. Probably worth doing something similar to the 1.2 code at some point too. (Though I guess you have to keep EVP_CTRL_AEAD_TLS1_AAD around for stability reasons...)

davidben · 2016-11-20T16:05:54Z

ssl/record/ssl3_record_tls13.c

Since none of these will ever set read_hash and thus never hit the MAC code and also will never hit the constant-time CBC business, I believe the 0 / -1 distinction and constant-time worries aren't meaningful. (Hopefully once the refactoring in #1438 happens, it'll be less confusing.)

Er, I guess that bug is talking about lots of different things. By "the refactoring in #1438" I mean folding tls1_mac into tls1_enc, so that you always do encrypt + MAC checks as a unit, rather than the one to tidy up tls1_cbc_remove_padding's return value.

Perhaps not, although the return code values need to be consistent between tls1_enc and tls13_enc because this gets called by this line in ssl3_get_record() which has no awareness of TLSv1.3:

enc_err = s->method->ssl3_enc->enc(s, rr, num_recs, 0); /*- * enc_err is: * 0: (in non-constant time) if the record is publically invalid. * 1: if the padding is valid * -1: if the padding is invalid */ if (enc_err == 0) { al = SSL_AD_DECRYPTION_FAILED; SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_BLOCK_CIPHER_PAD_IS_WRONG); goto f_err; }

I guess the difference is that 0 means a failure occurred before we even attempted to decrypt because it is clearly wrong before we even start, a -1 means some failure occurred after decryption was attempted. 0 gets you decryption_failed, while -1 gets you bad_record_mac. I'm not sure whether we care about that difference or not.

davidben · 2016-11-20T16:06:23Z

ssl/record/ssl3_record_tls13.c

Glancing at tls1_enc, I think this wants to be -1?

(The only zero return in tls1_enc is when tls1_cbc_remove_padding returns 0. This matches ssl3_get_record which sets SSL_R_BLOCK_CIPHER_PAD_IS_WRONG on zero return.)

The thinking here is that this goes into the category of "publicly invalid" (which is defined as a 0 return). We have received an encrypted record that is shorter than the tag length.

mattcaswell · 2016-11-21T17:32:42Z

New commit added to this to fix a Travis failure.

FdaSilvaYY · 2016-11-21T22:37:31Z

test/tls13encryptiontest.c

You get a double-free after this, if you don't add here a *key = NULL;

Fixed. Thanks.

mattcaswell · 2016-11-22T10:17:01Z

New commits pushed. Fixed another Travis failure, and a double free in tls13encryptiontest.

mattcaswell · 2016-11-23T15:59:49Z

I rebased this on the latest master. All commits in this PR are now for review.

mattcaswell · 2016-11-23T17:08:29Z

I rebased again to pick up the latest master travis fixes.

mattcaswell · 2016-11-29T10:25:52Z

Rebased again. Ping?

richsalz · 2016-11-29T14:07:47Z

engines/e_ossltest.c

Please add a comment that OPENSSL_malloc returns NULL if size is zero (since that's not guaranteed by regular malloc).

richsalz · 2016-11-29T14:08:06Z

engines/e_ossltest.c

squash to a single comment line.

richsalz · 2016-11-29T14:09:06Z

engines/e_ossltest.c

move up to the decl? it's what we usually do. or if not, then remove the blank line before the if.

richsalz · 2016-11-29T14:12:39Z

ssl/record/ssl3_record_tls13.c

"can't" --> "doesn't"

richsalz · 2016-11-29T14:16:17Z

test/tls13encryptiontest.c

that looks longer than 509 chars and will therefore break on some platforms. and you need a comment saying how this data was generated or where it came from.

They came from: https://www.ietf.org/id/draft-thomson-tls-tls13-vectors-01.txt

I added a comment for that. The 509 chars thing is problematic...I might need to make a few changes to get around that. I'll give it some thought.

Maybe look at what I did in #1775 about gluing things together.

richsalz · 2016-11-29T14:20:59Z

test/tls13encryptiontest.c

I know you don't like it, but "if ((a=..)==NULL" is the construct we use throughout the rest of the code.

No we don't!!!! There are examples of that, sure. But there are plenty of examples where we don't. It is definitely not our standard, and it isn't in our style guide. There are occasions where it makes sense, but those are rare and this isn't one of them.

interestingly, you're right that it's not style:

; g grep 'if.*==.*NULL' > /tmp/x ; wc /tmp/x 3056 20404 156963 /tmp/x ; grep '=.*==' /tmp/x | wc 939 8288 64894

richsalz · 2016-11-29T14:21:54Z

test/tls13encryptiontest.c

If you're going to keep this multi-line comment, then the blank lines between the memcpy and the EVP_CIPHER setup should be removed. Comments should go only for a single "paragraph" of code ... Or split into three comments.

I split it into three.

richsalz · 2016-11-29T14:22:21Z

test/tls13encryptiontest.c

remove this blank line, it's part of the same paragraph as above.

I think you mean the one between tls13_enc() and test_record(), rather than this one...at least that makes more sense to me in terms of paragraphs of code - so I removed that one.

richsalz · 2016-11-29T14:22:31Z

test/tls13encryptiontest.c

remove this blank line too.

richsalz · 2016-11-29T14:22:56Z

test/tls13encryptiontest.c

move this blank line to just before the err label :)

This updates the record layer to use the TLSv1.3 style nonce construciton. It also updates TLSProxy and ossltest to be able to recognise the new layout.

mattcaswell · 2016-11-29T23:32:42Z

I pushed a new commit to address the feedback so far and rebased to pick up the encrypt-then-mac test fix from master. I've not yet addressed the 509 char limit thing, or the index vs pointer thing. They need a bit more thought.

richsalz · 2016-11-30T02:09:23Z

the 509 char and the index/pointer are my only remaining concerns.

richsalz · 2016-11-30T02:11:22Z

(comment hidden because the commit got superceeded). A point is better; see http://doc.cat-v.org/bell_labs/pikestyle on "the use of pointers"

mattcaswell · 2016-11-30T09:13:36Z

A point is better; see http://doc.cat-v.org/bell_labs/pikestyle on "the use of pointers"

That's a good essay. I don't agree with all of it, but there are some useful rules in there. I particularly like rule 6 in the "Complexity" section. :-)

…ndex We also split the long string literals into 3 to avoid problems where we go over the 509 character limit.

mattcaswell · 2016-11-30T10:59:50Z

New commits pushed. The first one deals with the 509 character limit problem, and converts it to pass a pointer around not an index. The second commit marks refdata as static to avoid a clang issue.

richsalz · 2016-11-30T17:54:41Z

+1

mattcaswell · 2016-12-01T09:44:40Z

Pushed. Thanks.

davidben reviewed Nov 20, 2016

View reviewed changes

mattcaswell mentioned this pull request Nov 21, 2016

TLSv1.3: Record Construction #1975

Closed

2 tasks

FdaSilvaYY reviewed Nov 21, 2016

View reviewed changes

mattcaswell force-pushed the tls1_3-ivs-alt branch from 42e3c4b to 8564d03 Compare November 22, 2016 10:17

mattcaswell force-pushed the tls1_3-ivs-alt branch from 8564d03 to 19cdeac Compare November 23, 2016 15:58

mattcaswell force-pushed the tls1_3-ivs-alt branch from 19cdeac to 27fafcd Compare November 23, 2016 17:08

mattcaswell force-pushed the tls1_3-ivs-alt branch from 27fafcd to f3affee Compare November 29, 2016 10:25

richsalz suggested changes Nov 29, 2016

View reviewed changes

mattcaswell added 5 commits November 29, 2016 23:31

Use the TLSv1.3 nonce construction

7f35c75

This updates the record layer to use the TLSv1.3 style nonce construciton. It also updates TLSProxy and ossltest to be able to recognise the new layout.

Add a test for TLSv1.3 encryption using the new nonce construction

65f0a6e

Fix a travis compilation error

d6ba018

Fix a double free in tls13encryptiontest

6869548

Fix some style issues in the TLSv1.3 nonce construction code

d918344

mattcaswell force-pushed the tls1_3-ivs-alt branch from f3affee to d918344 Compare November 29, 2016 23:31

mattcaswell added 2 commits November 30, 2016 10:53

Convert tls13encryptiontest so that we pass around a pointer not an i…

c9aa1fd

…ndex We also split the long string literals into 3 to avoid problems where we go over the 509 character limit.

Make refdata in tls13encryptest static

13ee39c

mattcaswell mentioned this pull request Nov 30, 2016

TLSv1.3: Extensions refactor and add EncryptedExtensions message #2015

Closed

2 tasks

richsalz approved these changes Nov 30, 2016

View reviewed changes

richsalz added the reviewed label Nov 30, 2016

mattcaswell closed this Dec 1, 2016

Uh oh!

Comments

Conversation

mattcaswell commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

davidben 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Nov 22, 2016

Uh oh!

mattcaswell commented Nov 23, 2016

Uh oh!

mattcaswell commented Nov 23, 2016

Uh oh!

mattcaswell commented Nov 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mattcaswell commented Nov 17, 2016 •

edited

Loading