TLSv1.3: Record Construction by mattcaswell · Pull Request #1975 · openssl/openssl

mattcaswell · 2016-11-21T17:20:03Z

Checklist

tests are added or updated
CLA is signed

Description of change

This modifies the record layer to use TLSv1.3 record construction. I also updated various parts of the record layer to use PACKET/WPACKET.

mattcaswell · 2016-11-23T16:05:23Z

I rebased this now that #1932 has gone in. It still depends on #1947 so please only look at the last 6 commits.

Add some tests for the new record construction

At the moment the msg callback only received the record header with the outer record type in it. We never pass the inner record type - we probably need to at some point.

mattcaswell · 2016-12-01T10:22:40Z

I rebased this again. All previous PRs this depended on have now been merged into master, so please review all commits.

richsalz · 2016-12-02T05:10:54Z

ssl/packet.c

    }
-    *allocbytes = GETBUF(pkt) + pkt->curr;
+    if (allocbytes != NULL)
+        *allocbytes = GETBUF(pkt) + pkt->curr;


shouldn't this call the new get_curr routine you just added?

richsalz · 2016-12-02T05:12:16Z

ssl/record/rec_layer_s3.c

 #endif
-        outbuf[0] = SSL3_BUFFER_get_buf(wb) + align;
        SSL3_BUFFER_set_offset(wb, align);
+        if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb),


I dislike &foo[0] constructs as a parameter, since foo is the same thing by definition

Yes they are the same, but in my mind it makes it a lot clearer that you are referring to a single entity and not the array as a whole. Especially when you mix it in scenarios where on other lines we refer to a single entity which is not at index 0. I dislike the inconsistency.

richsalz · 2016-12-02T05:12:53Z

ssl/record/rec_layer_s3.c

        wb = &s->rlayer.wbuf[0];
-        outbuf[0] = SSL3_BUFFER_get_buf(wb) + SSL3_BUFFER_get_offset(wb)
-            + prefix_len;
+        if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb),


maybe break after the pkt param, so that the get_buf/get_len calls could appear on the same line.

They still will not fit on the same line.

richsalz · 2016-12-02T05:13:11Z

ssl/record/rec_layer_s3.c

-            + prefix_len;
+        if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb),
+                                     SSL3_BUFFER_get_len(wb), 0)
+                || !WPACKET_allocate_bytes(&pkt[0], SSL3_BUFFER_get_offset(wb)


linebreak after the comma so that the expression can fit on a single line.

Still does not fit.

richsalz · 2016-12-02T05:13:34Z

ssl/record/rec_layer_s3.c

    } else {
        for (j = 0; j < numpipes; j++) {
            wb = &s->rlayer.wbuf[j];
 #if defined(SSL3_ALIGN_PAYLOAD) && SSL3_ALIGN_PAYLOAD!=0


space around the !=

This is in existing code...but ok, done.

richsalz · 2016-12-02T05:15:04Z

ssl/record/rec_layer_s3.c

-        /* field where we are to write out packet length */
-        plen[j] = outbuf[j];
-        outbuf[j] += 2;
+        maxcomplen = pipelens[j] + (s->compress != NULL


break after the plus to keep the ternary subexpression together.

Still doesn't fit

maxcomplen = pipelens[j] + (s->compress != NULL ? SSL3_RT_MAX_COMPRESSED_OVERHEAD : 0);

Or

maxcomplen = pipelens[j]; if (s->compress != NULL) maxcomplen += SSL3_RT_MAX_COMPRESSED_OVERHEAD;

My comment isn't getting formatted properly. In the first example line the + under the =, and then see how much fits.

I implemented the second variant.

richsalz · 2016-12-02T05:15:53Z

ssl/record/rec_layer_s3.c

 #endif
-            outbuf[j] = SSL3_BUFFER_get_buf(wb) + align;
            SSL3_BUFFER_set_offset(wb, align);
+            if (!WPACKET_init_static_len(&pkt[j], SSL3_BUFFER_get_buf(wb),


as above, try to keep the getbuf/buflen params together.

As above, still does not fit.

richsalz · 2016-12-02T05:16:32Z

ssl/record/rec_layer_s3.c

+        } else if (mode == EVP_CIPH_CCM_MODE) {
            eivlen = EVP_CCM_TLS_EXPLICIT_IV_LEN;
-        else
+        } else {


maybe init eivlen to zero so you can avoid this statement and the one at 783

richsalz · 2016-12-02T05:17:58Z

ssl/record/rec_layer_s3.c

+            }
+            SSL3_RECORD_add_length(&wr[j], 1);
+            /*
+             * TODO(TLS1.3): Padding goes here. Do we need an API to add this?


yes we need an API or a callback. which one to do requires some thought.

Ok. For a later PR.

richsalz · 2016-12-02T05:21:02Z

ssl/record/ssl3_record.c

+        if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
+            size_t end;
+
+            if (rr[j].length == 0) {


introduce a local pointer value to replace all these rr[j] instances.

and not just in this if block, that is.

Improves the readability of the code, and reduces the liklihood of errors.

mattcaswell · 2016-12-02T11:11:37Z

Updated commits addressing feedback so far pushed.

richsalz · 2016-12-02T13:45:45Z

As for the "still does not fit" I understand, but perhaps you can reformat the lines so that the subexpressions are better self-contained.

richsalz · 2016-12-02T15:29:43Z

BTW, if you say that you cannot do the reformatting then +1

mattcaswell · 2016-12-02T15:35:37Z

I have had another look at the sub-expressions, but I can't come up with a better way of formatting them that is more sane than what I currently have.

richsalz · 2016-12-02T15:56:26Z

ssl/record/rec_layer_s3.c

-            + prefix_len;
+        if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb),
+                                     SSL3_BUFFER_get_len(wb), 0)
+                || !WPACKET_allocate_bytes(&pkt[0], SSL3_BUFFER_get_offset(wb)


So I was suggesting that maybe this is better:

if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb), SSL3_BUFFER_get_len(wb), 0)

Or this

if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb), SSL3_BUFFER_get_len(wb), 0)

Ok, I did the first version.

richsalz · 2016-12-02T15:59:34Z

ssl/record/rec_layer_s3.c

-        /* field where we are to write out packet length */
-        plen[j] = outbuf[j];
-        outbuf[j] += 2;
+        maxcomplen = pipelens[j] + (s->compress != NULL


My comment isn't getting formatted properly. In the first example line the + under the =, and then see how much fits.

richsalz · 2016-12-02T16:00:45Z

ssl/record/rec_layer_s3.c

+        size_t origlen;
+
+        /* Allocate bytes for the encryption overhead */
+        if (!WPACKET_get_length(&pkt[j], &origlen)


I think all these pkt[j] and wr[j] should become pointers.

richsalz · 2016-12-02T16:01:29Z

upon re-reading, I'm holding back by plus-one; the formatting isn't the issue, it's the pkt/wr[j] stuff.

…pointer Improves the readability of the code, and reduces the liklihood of errors. Also made a few minor style changes.

mattcaswell · 2016-12-05T10:28:43Z

New commit pushed addressing the feedback.

richsalz · 2016-12-05T14:22:10Z

+1

mattcaswell · 2016-12-05T17:08:51Z

Pushed. Thanks.

kaduk · 2016-12-05T19:58:59Z

ssl/record/rec_layer_s3.c

+             * TODO(TLS1.3): Make sure we prevent compression!!!
+             */
+            if (!ssl3_do_compress(s, thiswr)
+                    || !WPACKET_allocate_bytes(thispkt, thiswr->length, NULL)) {


How does the (compressed) data get to the WPACKET in the compression case?
It looks to me like ssl3_do_compress is keeping it within the SSL3_RECORD.

Look at the SSL3_RECORD_set_data() call a few lines above. An SSL3_RECORD structure never actually stores data itself, it just holds pointers to data somewhere else. In this case we set the data to point at the data that we just reserved in the WPACKET_reserve_bytes() call.

Ah, thanks.
(Do we have any tests that exercise the SSL compression functionality? I see that ssltest_old.c takes a -zlib argument, but am not finding any recipes that actually use it.)

kaduk · 2016-12-05T20:10:30Z

ssl/record/ssl3_record.c

            p = RECORD_LAYER_get_packet(&s->rlayer);
-
+            if (!PACKET_buf_init(&pkt, RECORD_LAYER_get_packet(&s->rlayer),
+                                 RECORD_LAYER_get_packet_length(&s->rlayer))) {


Hmm, maybe a RECORD_to_PACKET helper is in order?

Well, maybe...but its probably not worth it for just one instance of this.

kaduk · 2016-12-05T21:51:02Z

ssl/record/rec_layer_s3.c

+            /*
+             * TODO(TLS1.3): Padding goes here. Do we need an API to add this?
+             * For now, use no padding
+             */


There should probably be some sort of API. One could imagine letting the application choose amongst some predefined padding profiles, like "add random chaff" or "pad up to N-byte boundaries for various fixed N". I guess one could also imagine adding a callback to give the application packet-level control, which might be useful for tor-like applications, but is probably overkill for the first cut at things.

It might make sense to always add a padding byte for now, to exercise the read-side code (unless there are boringssl tests already covering that?).

Look at what we did for CT. A callback, and provide a couple of fixed policies.

Yes, I'm thinking along similar lines. A later PR though.

kaduk · 2016-12-05T21:55:40Z

util/TLSProxy/Record.pm


+    if (TLSProxy::Proxy->is_tls13()) {
+        #Get the content type
+        my $content_type = unpack("C", substr($data, length($data) - 1));


Hmm, does this mean the proxy is unable to cope with padding?

Yes. But that's ok. The proxy is only ever used as a test tool between two communicating OpenSSL instances. At the moment OpenSSL never sends padding, so this works. At some point in the future we will have to amend it when we start to send padding.

mattcaswell force-pushed the tls1_3-record-construct branch from 55ba47e to c010aba Compare November 23, 2016 16:04

mattcaswell force-pushed the tls1_3-record-construct branch from c010aba to 54b5778 Compare November 23, 2016 17:17

mattcaswell mentioned this pull request Nov 30, 2016

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

Closed

2 tasks

mattcaswell added 7 commits December 1, 2016 09:50

Add an ability to find out the current write location from a WPACKET

0d3ebc1

Convert TLS record construction to use WPACKET

2f1eff5

Convert TLS Record receipt to use PACKET

e06cfcc

Update the record layer to use TLSv1.3 style record construction

f78cc27

Add more TLS1.3 record tests

cd46929

Add some tests for the new record construction

Add a TLS1.3 TODO for the msg callback

0831c39

At the moment the msg callback only received the record header with the outer record type in it. We never pass the inner record type - we probably need to at some point.

Ensure compressdata is always initialised

f8ea452

mattcaswell force-pushed the tls1_3-record-construct branch from 54b5778 to f8ea452 Compare December 1, 2016 10:21

richsalz suggested changes Dec 2, 2016

View reviewed changes

mattcaswell added 2 commits December 2, 2016 11:09

Change various repeated rr[someindex] references to a pointer

325673f

Improves the readability of the code, and reduces the liklihood of errors.

Various style fixes from the TLSv1.3 record changes review

942549a

richsalz reviewed Dec 2, 2016

View reviewed changes

richsalz suggested changes Dec 2, 2016

View reviewed changes

Change various repeated wr[someindex]/pkt[someindex] references to a …

1296f9b

…pointer Improves the readability of the code, and reduces the liklihood of errors. Also made a few minor style changes.

mattcaswell closed this Dec 5, 2016

kaduk reviewed Dec 5, 2016

View reviewed changes

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Description of change

Uh oh!

mattcaswell commented Nov 23, 2016

Uh oh!

mattcaswell commented Dec 1, 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!

richsalz Dec 2, 2016 • edited 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!

mattcaswell commented Dec 2, 2016

Uh oh!

richsalz commented Dec 2, 2016

Uh oh!

richsalz commented Dec 2, 2016

Uh oh!

mattcaswell commented Dec 2, 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!

mattcaswell commented Nov 21, 2016 •

edited

Loading

richsalz Dec 2, 2016 •

edited

Loading

richsalz Dec 5, 2016 •

edited

Loading