Fix lack of cookie check on hard reconnect

Section 4.2.8 of RFC 6347 describes how to handle the case of a DTLS client
establishing a new connection using the same UDP quartet as an already active
connection, which we implement under the compile option
MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE. Relevant excerpts:

    [the server] MUST NOT destroy the existing
    association until the client has demonstrated reachability either by
    completing a cookie exchange or by completing a complete handshake
    including delivering a verifiable Finished message.
    [...]
    The reachability requirement prevents
    off-path/blind attackers from destroying associations merely by
    sending forged ClientHellos.

Our code chooses to use a cookie exchange for establishing reachability, but
unfortunately that check was effectively removed in a recent refactoring,
which changed what value ssl_handle_possible_reconnect() needs to return in
order for ssl_get_next_record() (introduced in that refactoring) to take the
proper action. Unfortunately, in addition to changing the value, the
refactoring also changed a return statement to an assignment to the ret
variable, causing the function to reach the code for a valid cookie, which
immediately destroys the existing association, effectively bypassing the
cookie verification.

This commit fixes that by immediately returning after sending a
HelloVerifyRequest when a ClientHello without a valid cookie is found. It also
updates the description of the function to reflect the new return value
convention (the refactoring updated the code but not the documentation).

The commit that changed the return value convention (and introduced the bug)
is 2fddd3765e, whose commit message explains the
change.

Note: this bug also indirectly caused the ssl-opt.sh test case "DTLS client
reconnect from same port: reconnect" to occasionally fail due to a race
condition between the reception of the ClientHello carrying a valid cookie and
the closure of the connection by the server after noticing the ClientHello
didn't carry a valid cookie after it incorrectly destroyed the previous
connection, that could cause that ClientHello to be invisible to the server
(if that message reaches the server just before it does `net_close()`). A
welcome side effect of this commit is to remove that race condition, as the
new connection will immediately start with a ClientHello carrying a valid
cookie in the SSL input buffer, so the server will not call `net_close()` and
not risk discarding a better ClientHello that arrived in the meantime.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

ChangeLog

@@ -6,6 +6,14 @@ New deprecations
    * Deprecate MBEDTLS_SSL_HW_RECORD_ACCEL that enables function hooks in the
      SSL module for hardware acceleration of individual records.
 
+Security
+   * Fix issue in DTLS handling of new associations with the same parameters
+     (RFC 6347 section 4.2.8): an attacker able to send forged UDP packets to
+     the server could cause it to drop established associations with
+     legitimate clients, resulting in a Denial of Service. This could only
+     happen when MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE was enabled in config.h
+     (which it is by default).
+
 Bugfix
    * Fix compilation failure when both MBEDTLS_SSL_PROTO_DTLS and
      MBEDTLS_SSL_HW_RECORD_ACCEL are enabled.

library/ssl_msg.c

@@ -3197,16 +3197,17 @@ static int ssl_check_dtls_clihlo_cookie(
  * that looks like a ClientHello.
  *
  * - if the input looks like a ClientHello without cookies,
- *   send back HelloVerifyRequest, then
- *   return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED
+ *   send back HelloVerifyRequest, then return 0
  * - if the input looks like a ClientHello with a valid cookie,
  *   reset the session of the current context, and
  *   return MBEDTLS_ERR_SSL_CLIENT_RECONNECT
  * - if anything goes wrong, return a specific error code
  *
- * mbedtls_ssl_read_record() will ignore the record if anything else than
- * MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned, although this function
- * cannot not return 0.
+ * This function is called (through ssl_check_client_reconnect()) when an
+ * unexpected record is found in ssl_get_next_record(), which will discard the
+ * record if we return 0, and bubble up the return value otherwise (this
+ * includes the case of MBEDTLS_ERR_SSL_CLIENT_RECONNECT and of unexpected
+ * errors, and is the right thing to do in both cases).
  */
 static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
 {
@@ -3237,7 +3238,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
          * If the error is permanent we'll catch it later,
          * if it's not, then hopefully it'll work next time. */
         (void) ssl->f_send( ssl->p_bio, ssl->out_buf, len );
-        ret = 0;
+        return( 0 );
     }
 
     if( ret == 0 )