Move DTLS code into the new write record layer #19424

mattcaswell · 2022-10-17T15:59:18Z

We move the DTLS write side into the new record layer architecture. This represents the last major component in the move to the new architecture. Some subsequent PRs are still required to resolve some loose ends and outstanding "TODO" items.

do_dtls1_write() was never called with a value for create_empty_fragment that was ever non-zero - so this is dead code and can be removed. The equivalent code in the TLS processing is used for TLS1.0/SSLv3 to protect against known IV weaknesses because those protocol versions do not have an explicit IV. However DTLS1.0 is based on TLSv1.1 and *does* have an explicit IV - so this is not useful there.

In preparation for moving the DTLS code to use the new write record layer architecture we first restructure the code to create a dtls_write_records() function that mirrors the functionality that the record layer will provide.

At the this stage we just move the code and don't restructure it to do it the record layer way yet.

We now use standard record layer return values for this function. We also convert the code to use RLAYERfatal instead of SSLfatal.

In practice this just means have a DTLS specific write_records that the common tls_write_records() just calls. We also replace the use of ssl3_write_pending() with tls_retry_write_records().

Previously this was writing to the buffers directly. We use the safer WPACKET instead

We have standard functions for most of the work that dtls_write_records does - so we convert it to use those functions instead.

Don't calculate the potential record layer expansion outside of the record layer. We move some code that was doing that into the record layer.

The sequence counter was incremented in numerous different ways in numerous different locations. We introduce a single function to do this inside the record layer.

We already set the record type on the SSL3_RECORD structure. We don't need to do it again (inconsistently).

This change make dtls_write_records virtuall the same as tls_write_records_default, which will enable us to merge them in a subsequent commit.

The dtls_write_records function, after the previous series of commits, was functionally equivalent to tls_write_records_default - so it can be removed completely.

We no longer use the old buffer management code now that it has all been moved to the new record layer.

paulidale · 2022-10-18T01:09:24Z

CI failure looks relevant.

Calling SSL_free() will call BIO_free_all() on the rbio and wbio. We keep references to the rbio and wbio inside the record layer object. References to that object are held directly, as well as in fragment retransmission queues. We need to ensure all record layer objects are cleaned up before we call BIO_free_all() on rbio/wbio - otherwise the "top" BIO may not have its reference count drop to 0 when BIO_free_all() is called. This means that the rest of the BIOs in the chain don't get freed and a memory leak can occur.

mattcaswell · 2022-10-18T11:27:28Z

CI failure looks relevant.

Fixed (hopefully).

hlandau

I can't see anything at issue here.

What are we going to do about the disabled test? Can it be fixed in this PR, or does it need to be deferred?

mattcaswell · 2022-10-19T10:57:06Z

What are we going to do about the disabled test? Can it be fixed in this PR, or does it need to be deferred?

This is one of the "loose ends" I plan to follow up on in subsequent PRs. There are a handful of "TODOs" left over in the code that need some attention - this being one of them. I decided to defer it because it wasn't strictly related to the DTLS code - its a TLSv1.3 test, which was missed in the earlier TLSv1.3 work.

levitte · 2022-10-19T12:37:24Z

ssl/record/rec_layer_s3.c

+    /*
+     * TODO(RECLAYER): Need a way to release the write buffers in the record
+     * layer on demand
+     */


I did notice the change from a RECORD_LAYER_get_wbuf() call to direct memory allocation in ssl/d1_lib.c, I assume that this comment is kinda sorta connected to that change...

Well kinda sorta, but only indirectly.

Previously the record layer write buffers were entirely open and available for any of libssl to dive into and play around with. Part of what the refactor has done is make the record layer opaque to the rest of libssl. We want it to be this way to ensure we can "plug in" any alternative implementation of the record layer. Therefore the write buffers used by the record layer are no longer accessible to the rest of libssl.

The change in ssl/d1_lib.c that you are referring to is in the implementation of DTLSv1_listen(). This function is a bit of an oddball because it works quite independently form the rest of libssl. It works entirely statelessly and does not use the record layer at all. Nonetheless in the old implementation it "reused" the record layer write buffers - simply because they were handy and already allocated - but not because it was using the record layer code at all. The change I made was so that DTLSv1_listen() would use its own temporary buffer rather than reusing a buffer that it had no real business touching.

That change was necessary because DTLSv1_listen() was actually using the old record layer's write buffers. Those get deleted by this PR because (apart from that one remaining usage) they were no longer being used.

This TODO is in RECORD_LAYER_release(). That function historically had a number of roles - but the one remaining one was to free the record layer write buffers. But that was the old write buffers which are no longer used, and are deleted by this PR. So this function becomes a no-op. It could actually be completely removed except for one call site in the public API function SSL_free_buffers(). Apart from in test code we never call that function ourselves - but external applications might do so. By making RECORD_LAYER_release() a no-op we also make SSL_free_buffers() a no-op. Really though what it ought to do is tell the new record layer code to release its buffers. That's what this TODO is reminding us.

levitte

Comment aside, LGTM

We now use standard record layer return values for this function. We also convert the code to use RLAYERfatal instead of SSLfatal. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

In practice this just means have a DTLS specific write_records that the common tls_write_records() just calls. We also replace the use of ssl3_write_pending() with tls_retry_write_records(). Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Previously this was writing to the buffers directly. We use the safer WPACKET instead Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

We have standard functions for most of the work that dtls_write_records does - so we convert it to use those functions instead. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Don't calculate the potential record layer expansion outside of the record layer. We move some code that was doing that into the record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

The sequence counter was incremented in numerous different ways in numerous different locations. We introduce a single function to do this inside the record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

We already set the record type on the SSL3_RECORD structure. We don't need to do it again (inconsistently). Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

This change make dtls_write_records virtuall the same as tls_write_records_default, which will enable us to merge them in a subsequent commit. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

The dtls_write_records function, after the previous series of commits, was functionally equivalent to tls_write_records_default - so it can be removed completely. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

We no longer use the old buffer management code now that it has all been moved to the new record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Calling SSL_free() will call BIO_free_all() on the rbio and wbio. We keep references to the rbio and wbio inside the record layer object. References to that object are held directly, as well as in fragment retransmission queues. We need to ensure all record layer objects are cleaned up before we call BIO_free_all() on rbio/wbio - otherwise the "top" BIO may not have its reference count drop to 0 when BIO_free_all() is called. This means that the rest of the BIOs in the chain don't get freed and a memory leak can occur. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

do_dtls1_write() was never called with a value for create_empty_fragment that was ever non-zero - so this is dead code and can be removed. The equivalent code in the TLS processing is used for TLS1.0/SSLv3 to protect against known IV weaknesses because those protocol versions do not have an explicit IV. However DTLS1.0 is based on TLSv1.1 and *does* have an explicit IV - so this is not useful there. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

In preparation for moving the DTLS code to use the new write record layer architecture we first restructure the code to create a dtls_write_records() function that mirrors the functionality that the record layer will provide. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

At the this stage we just move the code and don't restructure it to do it the record layer way yet. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

We now use standard record layer return values for this function. We also convert the code to use RLAYERfatal instead of SSLfatal. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

In practice this just means have a DTLS specific write_records that the common tls_write_records() just calls. We also replace the use of ssl3_write_pending() with tls_retry_write_records(). Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Previously this was writing to the buffers directly. We use the safer WPACKET instead Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

We have standard functions for most of the work that dtls_write_records does - so we convert it to use those functions instead. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Don't calculate the potential record layer expansion outside of the record layer. We move some code that was doing that into the record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

The sequence counter was incremented in numerous different ways in numerous different locations. We introduce a single function to do this inside the record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

We already set the record type on the SSL3_RECORD structure. We don't need to do it again (inconsistently). Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

This change make dtls_write_records virtuall the same as tls_write_records_default, which will enable us to merge them in a subsequent commit. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

The dtls_write_records function, after the previous series of commits, was functionally equivalent to tls_write_records_default - so it can be removed completely. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

We no longer use the old buffer management code now that it has all been moved to the new record layer. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

Calling SSL_free() will call BIO_free_all() on the rbio and wbio. We keep references to the rbio and wbio inside the record layer object. References to that object are held directly, as well as in fragment retransmission queues. We need to ensure all record layer objects are cleaned up before we call BIO_free_all() on rbio/wbio - otherwise the "top" BIO may not have its reference count drop to 0 when BIO_free_all() is called. This means that the rest of the BIOs in the chain don't get freed and a memory leak can occur. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19424)

mattcaswell added 16 commits October 17, 2022 16:47

Create a dlts_write_records() function

3ca534d

In preparation for moving the DTLS code to use the new write record layer architecture we first restructure the code to create a dtls_write_records() function that mirrors the functionality that the record layer will provide.

Move dlts_write_records() function in the record layer

169a406

At the this stage we just move the code and don't restructure it to do it the record layer way yet.

Use record layer buffers for DTLS rather than the buffers in s->rlayer

ee25118

Convert dtls_write_records() to return the correct return values

33a3da2

We now use standard record layer return values for this function. We also convert the code to use RLAYERfatal instead of SSLfatal.

Use common tls_write_records() even for DTLS

aeebbdc

In practice this just means have a DTLS specific write_records that the common tls_write_records() just calls. We also replace the use of ssl3_write_pending() with tls_retry_write_records().

Start using WPACKET in the dtls write records code

4d298e3

Previously this was writing to the buffers directly. We use the safer WPACKET instead

Convert dtls_write_records to use standard record layer functions

365de2d

We have standard functions for most of the work that dtls_write_records does - so we convert it to use those functions instead.

Ensure the record layer is responsible for calculating record overheads

cbf3d5c

Don't calculate the potential record layer expansion outside of the record layer. We move some code that was doing that into the record layer.

Consolidate sequence counter incrementing code

08f4263

The sequence counter was incremented in numerous different ways in numerous different locations. We introduce a single function to do this inside the record layer.

Remove supurious set of the record type

0f7169f

We already set the record type on the SSL3_RECORD structure. We don't need to do it again (inconsistently).

Move sequence increment to post encryption processing

7b8a343

This change make dtls_write_records virtuall the same as tls_write_records_default, which will enable us to merge them in a subsequent commit.

Remove dtls_write_records

b4cedd9

The dtls_write_records function, after the previous series of commits, was functionally equivalent to tls_write_records_default - so it can be removed completely.

Remove some TODO(RECLAYER) comments now that DTLS has been moved

25ee251

Remove the old buffer management code

3dbe757

We no longer use the old buffer management code now that it has all been moved to the new record layer.

Remove some redundant code

db32f00

mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Oct 17, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 17, 2022

hlandau approved these changes Oct 19, 2022

View reviewed changes

hlandau removed the approval: otc review pending This pull request needs review by an OTC member label Oct 19, 2022

t8m added the triaged: refactor The issue/pr requests/implements refactoring label Oct 19, 2022

levitte reviewed Oct 19, 2022

View reviewed changes

levitte approved these changes Oct 19, 2022

View reviewed changes

levitte removed the approval: review pending This pull request needs review by a committer label Oct 19, 2022

openssl-machine pushed a commit that referenced this pull request Oct 20, 2022

Remove some redundant code

b92fc4a

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19424)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move DTLS code into the new write record layer #19424

Move DTLS code into the new write record layer #19424

mattcaswell commented Oct 17, 2022

paulidale commented Oct 18, 2022

mattcaswell commented Oct 18, 2022

hlandau left a comment

mattcaswell commented Oct 19, 2022

levitte Oct 19, 2022

mattcaswell Oct 20, 2022

levitte left a comment

Move DTLS code into the new write record layer #19424

Move DTLS code into the new write record layer #19424

Conversation

mattcaswell commented Oct 17, 2022

paulidale commented Oct 18, 2022

mattcaswell commented Oct 18, 2022

hlandau left a comment

Choose a reason for hiding this comment

mattcaswell commented Oct 19, 2022

levitte Oct 19, 2022

Choose a reason for hiding this comment

mattcaswell Oct 20, 2022

Choose a reason for hiding this comment

levitte left a comment

Choose a reason for hiding this comment