Re: [PATCH 5/8] secur32: Add support for setting DTLS timeouts.

[Connor McAdams]

On Tue, Jan 25, 2022 at 10:32:11AM -0500, Connor McAdams wrote:
> Add support for setting the DTLS timeout values, and set the
> retransmission timeout value to 0 to allow for retransmission on each
> call to schan_InitializeSecurityContext.
> Signed-off-by: Connor McAdams
>
> dlls/secur32/schannel.c        |  6 ++++++
>  dlls/secur32/schannel_gnutls.c | 22 ++++++++++++++++++++++
>  dlls/secur32/secur32_priv.h    |  8 ++++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
> index fa5577d78e3..19e21da8d72 100644
> --- a/dlls/secur32/schannel.c
> +++ b/dlls/secur32/schannel.c
> @@ -809,6 +809,12 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
>              else WARN("invalid buffer size %u\n", buffer->cbBuffer);
>          }
> -
>    if (is_dtls_context(ctx))
> -
>    {
> -
>        struct set_dtls_timeouts_params params = { ctx->transport.session, 0, 60000 };
> -
>        GNUTLS_CALL( set_dtls_timeouts, &params );
> -
>    }
> -
>    phNewContext->dwLower = handle;
>    phNewContext->dwUpper = 0;
> }
>
> diff --git a/dlls/secur32/schannel_gnutls.c b/dlls/secur32/schannel_gnutls.c
> index 31fdb769677..ac51cbb5d9f 100644
> --- a/dlls/secur32/schannel_gnutls.c
> +++ b/dlls/secur32/schannel_gnutls.c
> @@ -60,6 +60,7 @@ static int (*pgnutls_cipher_get_block_size)(gnutls_cipher_algorithm_t);
>  static void (*pgnutls_transport_set_pull_timeout_function)(gnutls_session_t,
>                                                             int (*)(gnutls_transport_ptr_t, unsigned int));
>  static void (*pgnutls_dtls_set_mtu)(gnutls_session_t, unsigned int);
> +static void (*pgnutls_dtls_set_timeouts)(gnutls_session_t, unsigned int, unsigned int);
>
> /* Not present in gnutls version < 3.2.0. */
>  static int (*pgnutls_alpn_get_selected_protocol)(gnutls_session_t, gnutls_datum_t *);
> @@ -198,6 +199,12 @@ static void compat_gnutls_dtls_set_mtu(gnutls_session_t session, unsigned int mt
>      FIXME("\n");
>  }
>
> +static void compat_gnutls_dtls_set_timeouts(gnutls_session_t session, unsigned int retrans_timeout,
> -
>    unsigned int total_timeout)
>
> +{
> - FIXME("\n");
>
> +}
> -
>
> static void init_schan_buffers(struct schan_buffers *s, const PSecBufferDesc desc,
>          int (*get_next_buffer)(const struct schan_transport *, struct schan_buffers *))
>  {
> @@ -989,6 +996,15 @@ static NTSTATUS schan_set_dtls_mtu( void *args )
>      return SEC_E_OK;
>  }
>
> +static NTSTATUS schan_set_dtls_timeouts( void *args )
> +{
> - const struct set_dtls_timeouts_params *params = args;
> - gnutls_session_t s = (gnutls_session_t)params->session;
> -
> - pgnutls_dtls_set_timeouts(s, params->retrans_timeout, params->total_timeout);
> - return SEC_E_OK;
>
> +}
> -
>
> static inline void reverse_bytes(BYTE *buf, ULONG len)
>  {
>      BYTE tmp;
> @@ -1245,6 +1261,11 @@ static NTSTATUS process_attach( void *args )
>          WARN("gnutls_dtls_set_mtu not found\n");
>          pgnutls_dtls_set_mtu = compat_gnutls_dtls_set_mtu;
>      }
> - if (!(pgnutls_dtls_set_timeouts = dlsym(libgnutls_handle, "gnutls_dtls_set_timeouts")))
> - {
> -
>    WARN("gnutls_dtls_set_timeouts not found\n");
> -
>    pgnutls_dtls_set_timeouts = compat_gnutls_dtls_set_timeouts;
> - }
> if (!(pgnutls_privkey_export_x509 = dlsym(libgnutls_handle, "gnutls_privkey_export_x509")))
> {
>     WARN("gnutls_privkey_export_x509 not found\n");
>
> @@ -1308,6 +1329,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] =
>      schan_set_application_protocols,
>      schan_set_dtls_mtu,
>      schan_set_session_target,
> - schan_set_dtls_timeouts,
>
> };
>
> #endif /* SONAME_LIBGNUTLS */
> diff --git a/dlls/secur32/secur32_priv.h b/dlls/secur32/secur32_priv.h
> index c17adc96dc6..64edc0581d6 100644
> --- a/dlls/secur32/secur32_priv.h
> +++ b/dlls/secur32/secur32_priv.h
> @@ -203,6 +203,13 @@ struct set_session_target_params
>      const char *target;
>  };
>
> +struct set_dtls_timeouts_params
> +{
> - schan_session session;
> - unsigned int retrans_timeout;
> - unsigned int total_timeout;
>
> +};
> -
>
> enum schan_funcs
>  {
>      unix_process_attach,
> @@ -225,6 +232,7 @@ enum schan_funcs
>      unix_set_application_protocols,
>      unix_set_dtls_mtu,
>      unix_set_session_target,
> - unix_set_dtls_timeouts,
>
> };
> #endif /* __SECUR32_PRIV_H__ */
>
> 2.25.1

All patches prior to this one are good, but this one and beyond will
break things. Maybe this is the universe's way of telling me to send
smaller patch sets. ;)

Essentially, the issue seems to be that we need to go back to
non-blocking mode with gnutls on DTLS contexts. We behave in a
non-blocking manner to begin with, seeing as we return immediately from
the pull_timeout callback. If we don't do this, it takes all sorts of
effort to work around it, and make it not get into a weird state when no
data is received. If it isn't in non-blocking mode, it doesn't seem to
complete a handshake if retransmission is involved.

Is there any good reason we switched away from GNUTLS_NONBLOCK in the
first place? If so, I can try to work around it. But from my testing,
having it enabled doesn't seem to cause issues. But maybe I'm missing
something.