Add support for fragmenting of DTLS handshake messages #1939

mpg · 2018-08-13T10:54:00Z

Description

Add support for fragmenting of DTLS handshake messages based on maximum fragment length and/or an explicit MTU setting.

Supersedes and continues #1879
Partially fixes #387 (for DTLS only).
Note to gatekeeper: please close internal ref IOTSSL-592 when merging this.

Based on #1942 and merging #1915 and #1955 along the way as they're all necessary for tests to pass here.
Reviewers, please only review new commits in this branch.
Note to gatekeeper: you probably want to merge #1942, #1915 and #1955 before this one for clarity.

Status

READY

Requires Backporting

NO

Migrations

Compatible API extensions

ABI change in libmbedtls (added mtu field to stuct mbedtls_ssl_config).

`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum difference between the size of a protected record and the size of the encapsulated plaintext. It had the following two bugs: (1) It did not consider the new ChaChaPoly ciphersuites, returning the error code #MBEDTLS_ERR_SSL_INTERNAL_ERROR in this case. (2) It did not correctly estimate the maximum record expansion in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which case the ciphertext is prefixed by an explicit IV. This commit fixes both bugs.

hanno-becker · 2018-08-13T15:32:03Z

library/ssl_tls.c

-    /*
-     * Check if a smaller max length was negotiated
-     */
+    /* Check if a smaller max length was negotiated */
    if( ssl->session_out != NULL &&


Some tests fail here because of #1941.

I believe this was fixed by basing this PR on #1942.

Fixes Mbed-TLS#1941.

hanno-becker · 2018-08-14T14:55:29Z

include/mbedtls/ssl.h

@@ -958,6 +958,10 @@ struct mbedtls_ssl_config
    unsigned int dhm_min_bitlen;    /*!< min. bit length of the DHM prime   */
 #endif

+#if defined(MBEDTLS_SSL_PROTO_DTLS)
+    uint16_t mtu;                   /*!< path mtu, used to fragment outoing messages */


Thanks, fixed.

hanno-becker · 2018-08-14T14:57:01Z

include/mbedtls/ssl.h

+ *
+ * \note           This only controls the size of the packet we send.
+ *                 Client-side, you can request the server to use smaller
+ *                 records with \c mbedtls_conf_max_frag_len().


I think the Doxygen links are automatically inserted when using the func_name() syntax, without \c.

Autolinks also work with the \c, so I'll keep it for consistency with the rest of the file. In that case, the link didn't work because the function name was wrong - missing the ssl_ part.

hanno-becker · 2018-08-14T14:59:52Z

include/mbedtls/ssl.h

+ *                 Client-side, you can request the server to use smaller
+ *                 records with \c mbedtls_conf_max_frag_len().
+ *
+ * \note           If both a MTU and a maximum fragment length have been


This slightly reads as if MTU and MFL would limit the same thing, but MTU limits the maximum size of (the list of) protected records (within the current datagram), while MFL limits the maximum payload size for each single record (as far as I understand).

I'll try to improve the wording here.

hanno-becker · 2018-08-14T15:02:03Z

include/mbedtls/ssl.h

+ *                 configured (or negotiated with the peer), the lower limit
+ *                 is used.
+ *
+ * \note           Values larger than \c MBEDTLS_SSL_OUT_CONTENT_LEN have no


I would rather expect MBEDTLS_SSL_OUT_BUFFER_LEN here, because MBEDTLS_SSL_OUT_CONTENT_LEN is about the unprotected payload.

I'm rephrasing that too.

hanno-becker · 2018-08-14T15:03:26Z

include/mbedtls/ssl.h

+/**
+ * \brief          Return the current maximum outgoing record payload in bytes.
+ *                 This takes into account the config.h setting \c
+ *                 MBEDTLS_SSL_OUT_CONTENT_LEN, the configured and negotiated


I think Doxygen links are inserted when using #MBEDTLS_SSL_OUT_CONTENT_LEN.

Thanks, fixing.

hanno-becker · 2018-08-14T15:17:07Z

library/ssl_tls.c

+ *  - ssl->out_msg[0]: the handshake type (ClientHello, ServerHello, etc)
+ *  - ssl->out_msg + 4: the handshake message body
+ *
+ * Ouputs, ie state before passing to flight_append() or write_record():


Thanks, fixing.

hanno-becker · 2018-08-14T15:28:33Z

@mpg How shall we resolve the duplicated fix for #1913 and #1914? Do you want to rebase this PR on top of #1915?

- take advantage of the fact that we're only called for first send - put all sanity checks at the top - rename and constify shortcut variables - improve comments

This will allow fragmentation to always happen in the same place, always from a buffer distinct from ssl->out_msg, and with the same way of resuming after returning WANT_WRITE

Note: no interop tests in ssl-opt.sh for now, as some of them make us run into bugs in (the CI's default versions of) OpenSSL and GnuTLS, so interop tests will be added later once the situation is clarified. <- TODO

hanno-becker · 2018-08-22T15:21:31Z

@mpg I pushed commits 2c98db2 and 551835d to address the failure mentioned in the review. Could you have a look if you think they are appropriate, and potentially cherry-pick them here?

simonbutcher · 2018-08-22T18:39:51Z

retest

k-stachowiak

In general I approve the PR. I am just curious about the local vs "global" nature of the pointers in the SSL context structure. I'd be happy to see it clarified.

Depending on the settings of the local machine, gnutls-cli will either try IPv4 or IPv6 when trying to connect to localhost. With TLS, whatever it tries first, it will notice if any failure happens and try the other protocol if necessary. With DTLS it can't do that. Unfortunately for now there isn't really any good way to specify an address and hostname independently, though that might come soon: https://gitlab.com/gnutls/gnutls/issues/344 A work around is to specify an address directly and then use --insecure to ignore certificate hostname mismatch; that is OK for tests that are completely unrelated to certificate verification (such as the recent fragmenting tests) but unacceptable for others. For that reason, don't specify a default hostname for gnutls-cli, but instead let each test choose between `--insecure 127.0.0.1` and `localhost` (or `--insecure '::1'` if desired). Alternatives include: - having test certificates with 127.0.0.1 as the hostname, but having an IP as the CN is unusual, and we would need to change our test certs; - have our server open two sockets under the hood and listen on both IPv4 and IPv6 (that's what gnutls-serv does, and IMO it's a good thing) but that obviously requires development and testing (esp. for windows compatibility) - wait for a newer version of GnuTLS to be released, install it on the CI and developer machines, and use that in all tests - quite satisfying but can't be done now (and puts stronger requirements on test environment).

In SSLv3, the client sends a NoCertificate alert in response to a CertificateRequest if it doesn't have a CRT. This previously lead to failure in ssl_write_handshake_msg() which only accepted handshake or CCS records.

The previous code appended messages to flights only if their handshake type, as derived from the first byte in the message, was different from MBEDTLS_SSL_HS_HELLO_REQUEST. This check should only be performed for handshake records, while CCS records should immediately be appended.

mpg · 2018-08-23T17:31:19Z

@hanno-arm @k-stachowiak I quick-fixed the gnutls-cli tests so that they pass also on machines where IPv6 does not work or where localhost resolves as IPv4 first, as discussed on skype; I also cherry-picked fixes for the SSLv3 issues as suggested by Hanno above. Please review again.

@hanno-arm Please be aware that my commit changes the $G_CLI default, so if you're adding other tests using $G_CLI in some other PR of yours , you'll need to adapt them too by appending localhost to the command line, which was previously part of the default. This is a non-local change, so git will not show it as a conflict but it needs resolution nonetheless.

mpg · 2018-08-23T20:27:13Z

Note: tests/scripts/all.sh -k -r -m ran successfully on the latest commit 081bd81 Travis passed on that commit too. @sbutcher-arm I'm not quite sure what the label "needs-ci" means (I know we're going to clarify that soon), but since you added it yesterday, please consider updating in light of that information.

simonbutcher · 2018-08-23T21:42:41Z

retest

simonbutcher · 2018-08-23T21:44:11Z

Hi @mpg. 'needs CI' means, the CI hasn't passed or is yet to run. In this instance, the CI (I think) had failed. Right now it's still failing but the build seems to be stale so I've triggered the build again.

mpg · 2018-08-24T08:29:57Z

I checked the output of the "mbed TLS build & test" CI and it appears the only failing test is the timing suite, which is known to be flaky, see #1517, so is not a blocker.

mpg · 2018-08-24T08:30:07Z

retest

mpg · 2018-08-24T10:29:41Z

The new CI run failed again only in the timing test. I'm not triggering a new run, though feel free to do so if you feel it's useful.

mpg · 2018-08-24T22:37:53Z

retest

simonbutcher

I haven't given this a deep nor full review, but I have some reservations and have given some feedback. I don't want to hold up merging the PR, so @mpg, could you please look at my review feedback and submit a second PR that addresses my review feedback? So we can get this merged, and do fixes as a followup.

Treat the review feedback as normal - and decide for yourself whether you agree or not to the suggestions. Reply to this PR for anything you disagree with.

(Also - apologies in advance if I've given feedback on a commit introduced by a different PR).

simonbutcher · 2018-08-24T13:52:01Z

include/mbedtls/ssl.h

+ *                 encapsulation and encryption/authentication if any.
+ *
+ * \note           This can be called at any point during the connection, for
+ *                 example when a PMTU estimate becomes available from other


PMTU would be better expanded as:

Path Maximum Transfer Unit (PMTU)

Not a blocker.

Addressed in hanno-becker@eefe084.

simonbutcher · 2018-08-24T14:05:46Z

include/mbedtls/ssl.h

- *                 Client-side, you can request the server to use smaller
- *                 records with \c mbedtls_ssl_conf_max_frag_len().
+ * \note           This setting only controls the size of the packets we send,
+ *                 and does not restrict the size of the datagrams we're


It's perfectly acceptable and breaks no language rules. You don't contract when you want to place emphasis, speak clearly or sound like a robot.

simonbutcher · 2018-08-24T14:30:02Z

include/mbedtls/ssl.h

+
+/**
+ * \brief          Return the current maximum outgoing record payload in bytes.
+ *                 This takes into account the config.h setting \c


We don't have to, and probably shouldn't call it the config.h setting MBEDTLS_SSL_OUT_CONTENT_LEN but instead got with the setting MBEDTLS_SSL_OUT_CONTENT_LEN. Just makes the documentation easier to maintain, and it's not necessary.

Not a blocker.

simonbutcher · 2018-08-24T15:50:49Z

library/ssl_tls.c

            memset( ssl->out_msg + 6, 0x00, 3 );
            memcpy( ssl->out_msg + 9, ssl->out_msg + 1, 3 );
        }
 #endif /* MBEDTLS_SSL_PROTO_DTLS */

-        if( out_msg_type != MBEDTLS_SSL_HS_HELLO_REQUEST )
-            ssl->handshake->update_checksum( ssl, ssl->out_msg, len );
+        /* Update running hashes of hanshake messages seen */


typo - 'hanshake' -> 'handshake'

Addressed in hanno-becker@0207e53.

simonbutcher · 2018-08-24T17:35:02Z

library/ssl_tls.c

 *
 * Need to remember the current message in case flush_output returns
 * WANT_WRITE, causing us to exit this function and come back later.
 * This function must be called until state is no longer SENDING.
 */
-int mbedtls_ssl_resend( mbedtls_ssl_context *ssl )
+int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl )


I think this function would have made more sense called mbedtls_ssl_transmit_flight(), to make it verb noun, rather than noun verb, but it's not a problem.

@sbutcher-arm I prefer this naming scheme because it allows to broadly categorize functions according to the parts of their name. For example, there are also (pre-existing) functions mbedtls_ssl_flight_append() and mbedtls_ssl_flight_free(), and for all of them the mbedtls_ssl_flight prefix indicates that they're about flight handling.

simonbutcher · 2018-08-25T16:18:11Z

library/ssl_tls.c

+                ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) )
+#endif /* MBEDTLS_SSL_PROTO_SSL3 && MBEDTLS_SSL_SRV_C */
+        {
+            MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) );


We should aim for better than "should never happen" messages in debug. yes, I know the code is full of them already, but they can be really irritating when you see them in debug output (and unhelpful), and would have once got us a spot on the Daily WTF.

Instead we should state the impossible condition and/or why it's invalid. It would be better to say "invalid state - not a handshake message" or something similar.

Not a blocker.

I think this should be addressed as a separate issue, going through all uses of should never happen we have in the library.

@sbutcher-arm @mpg I opened #1989 to track this.

simonbutcher · 2018-08-25T16:18:38Z

library/ssl_tls.c

+        hs_type != MBEDTLS_SSL_HS_HELLO_REQUEST &&
+        ssl->handshake == NULL )
+    {
+        MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) );


Again, we should aim for better than "should never happen" error messages.

simonbutcher · 2018-08-25T16:20:23Z

library/ssl_tls.c


 #if defined(MBEDTLS_SSL_PROTO_DTLS)
    if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM &&
        ssl->handshake != NULL &&
        ssl->handshake->retransmit_state == MBEDTLS_SSL_RETRANS_SENDING )
    {
-        ; /* Skip special handshake treatment when resending */
+        MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) );


Again, we should aim for better than "should never happen" error messages.

simonbutcher · 2018-08-25T17:04:51Z

tests/ssl-opt.sh

+            -C "error"
+
+# This ensures things still work after session_reset(),
+# for example it would have caught #1941.


We shouldn't reference GitHub issues in the test code, and especially not without explaining it's a GitHub issue. Instead if it's worth stating we should name the issue and explain it.

However - I'm not sure we have to - we should just state the test case and why we have it without mentioning bugs we've had before.

Addressed in hanno-becker@b841b4f.

simonbutcher · 2018-08-25T17:13:52Z

tests/ssl-opt.sh

+## https://gitlab.com/gnutls/gnutls/issues/543
+## We can re-enable them when a fixed version fo GnuTLS is available
+## and installed in our CI system.
+##


I'd rather we didn't comment these tests out, but instead skipped them, so they're included in the stats of what test cases we have, but also so as 'skipped' tests.

I suggest we add a forcing function of test_disabled which sets SKIP_NEXT="YES" to force the test to be skipped. Better than commenting it out.

Followed your suggestion in hanno-becker@3b8b40c.

simonbutcher · 2018-08-25T17:27:45Z

retest

mpg · 2018-08-27T10:54:18Z

Note to self: when creating the follow-up PR, see also #1918 (comment)

Hanno Becker added 2 commits August 3, 2018 10:07

Adapt ChangeLog

4481464

mpg requested review from hanno-becker and k-stachowiak August 13, 2018 10:54

mpg added enhancement mbed TLS team needs-work needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-tls labels Aug 13, 2018

mpg mentioned this pull request Aug 13, 2018

IOTSSL-165 DTLS handshake fragmentation #1879

Closed

hanno-becker reviewed Aug 13, 2018

View reviewed changes

Hanno Becker added 2 commits August 13, 2018 16:35

Reset session_in/out pointers in ssl_session_reset_int()

7864090

Fixes Mbed-TLS#1941.

Adapt ChangeLog

361f254

mpg mentioned this pull request Aug 14, 2018

Add support for datagram packing #1918

Merged

mpg force-pushed the iotssl-165-dtls-hs-fragmentation-new branch from a078c8c to 8c75a5d Compare August 14, 2018 10:11

Improve wording in ChangeLog

9dc3be7

hanno-becker reviewed Aug 14, 2018

View reviewed changes

mpg mentioned this pull request Aug 16, 2018

Handshake Messages are not fragmented as per MaxFragmentLength Extension Negotiation. #387

Closed

mpg added 5 commits August 16, 2018 10:00

Start separating handshake from record writing

31c1586

Clarify code a bit in write_handshake_msg()

9c3a8ca

- take advantage of the fact that we're only called for first send - put all sanity checks at the top - rename and constify shortcut variables - improve comments

Always save flight first, (re)send later

87a346f

This will allow fragmentation to always happen in the same place, always from a buffer distinct from ssl->out_msg, and with the same way of resuming after returning WANT_WRITE

Start implementing fragmentation

28f4bea

Make handshake fragmentation follow max_frag_len

2cb17e2

Note: no interop tests in ssl-opt.sh for now, as some of them make us run into bugs in (the CI's default versions of) OpenSSL and GnuTLS, so interop tests will be added later once the situation is clarified. <- TODO

simonbutcher added the needs-ci Needs to pass CI tests label Aug 22, 2018

simonbutcher self-requested a review August 22, 2018 18:40

k-stachowiak previously approved these changes Aug 23, 2018

View reviewed changes

mpg and others added 3 commits August 23, 2018 19:07

ssl_write_handshake_msg(): Allow alert on client-side SSLv3

c83d2b3

In SSLv3, the client sends a NoCertificate alert in response to a CertificateRequest if it doesn't have a CRT. This previously lead to failure in ssl_write_handshake_msg() which only accepted handshake or CCS records.

mpg dismissed k-stachowiak’s stale review via 081bd81 August 23, 2018 17:22

hanno-becker approved these changes Aug 24, 2018

View reviewed changes

k-stachowiak approved these changes Aug 24, 2018

View reviewed changes

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Aug 24, 2018

simonbutcher reviewed Aug 25, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports approved for design and removed needs-ci Needs to pass CI tests labels Aug 25, 2018

hanno-becker mentioned this pull request Aug 28, 2018

DTLS fragmentation and reordering support: Minor rework #1988

Merged

mpg mentioned this pull request Aug 28, 2018

Disable interop tests without a working OpenSSL version #1990

Merged

simonbutcher merged commit 081bd81 into Mbed-TLS:development Aug 29, 2018

mpg deleted the iotssl-165-dtls-hs-fragmentation-new branch December 27, 2018 09:30

Add support for fragmenting of DTLS handshake messages #1939

Add support for fragmenting of DTLS handshake messages #1939

Conversation

mpg commented Aug 13, 2018 • edited Loading

Description

Status

Requires Backporting

Migrations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Aug 14, 2018

hanno-becker commented Aug 22, 2018

simonbutcher commented Aug 22, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

mpg commented Aug 23, 2018 • edited Loading

mpg commented Aug 23, 2018

simonbutcher commented Aug 23, 2018

simonbutcher commented Aug 23, 2018

mpg commented Aug 24, 2018

mpg commented Aug 24, 2018

mpg commented Aug 24, 2018

mpg commented Aug 24, 2018

simonbutcher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Aug 25, 2018

mpg commented Aug 27, 2018

mpg commented Aug 13, 2018 •

edited

Loading

mpg commented Aug 23, 2018 •

edited

Loading

simonbutcher left a comment •

edited

Loading