882310 - Configurable DTLS handshake timeouts

Status: UNCONFIRMED

(NSS :: Libraries, defect, P4)

Description

[Eric Rescorla (:ekr)]

The DTLS handshake timeouts are currently hardwired and match the RFC recommendations.

It should be possible for NSS library consumers to reconfigure the initial timeout,
e.g.,

SSL_SetDtlsRetransmitTimeout(ms)....

Comment 1

[Meadhbh Hamrick :mhamrick]

Attachment: first hack at a fix

here's the first hack at the fix. did rudimentary testing.

a. I only added an interface to set the handsake retransmit timeout value. lemme know if there are other values I should be adding interfaces for.

b. I recommend we change the name of this to DTLS_RetransmitTimeoutSetDefault() to be in line with existing naming conventions.

Comment 2

[Eric Rescorla (:ekr)]

Thanks Meadhbh. I'm traveling till Sun, so will try to get to this early next week.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 781888 [details] [diff] [review]
first hack at a fix

Review of attachment 781888 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the

::: security/nss/lib/ssl/dtlscon.c
@@ +13,5 @@
>  #ifndef PR_ARRAY_SIZE
>  #define PR_ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0]))
>  #endif
>  
> +PRIntervalTime dtls_timeout_ms = INITIAL_DTLS_TIMEOUT_MS;

Instead of a global variable, this should be a field in sslOptionsStr (see sslSocketStr::opt) so that it can be set per-socket like all other options.

::: security/nss/lib/ssl/ssl.def
@@ +143,5 @@
>  ;+};
>  ;+NSS_3.14 {      # NSS 3.14 release
>  ;+    global:
>  DTLS_GetHandshakeTimeout;
> +SSL_SetDtlsRetransmitTimeout;

When you modify the NSS *.def files, you need to add a new section at the bottom that indicates which (upcoming) version of NSS will first contain the new entries. (See security/nss/TAG-INFO to see the current version.)

::: security/nss/lib/ssl/ssl.h
@@ +949,5 @@
>  
> +
> +/*
> +** By default, the DTLS timeout value is set to INITIAL_DTLS_TIMEOUT_MS.
> +** This function may be used to change this value.

INITIAL_DTLS_TIMEOUT_MS is in sslimpl.h which means it is not exposed to users of libssl. So, you shouldn't mention it here.

@@ +952,5 @@
> +** By default, the DTLS timeout value is set to INITIAL_DTLS_TIMEOUT_MS.
> +** This function may be used to change this value.
> +*/
> +
> +SSL_IMPORT SECStatus SSL_SetDtlsRetransmitTimeout(PRIntervalTime timeout);

This function will need to take a pointer to the socket so it can set the timeout on the socket.

I don't think "Dtls" needs to appear in the name of the function.

Comment 4

[Meadhbh Hamrick :mhamrick]

Attachment: second try

okay. added SSL_SetRetransmitTimeoutDefault() to change the global default. SSL_SetRetransmitTimeout() modifies per-socket options field. I still think we should call these DTLS_* or change DTLS_GetHandshakeTimeout() to be SSL_GetHandshakeTimeout().

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 782836 [details] [diff] [review]
second try

Review of attachment 782836 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it's on the right track, but I'd like to see another revision.

::: security/nss/lib/ssl/dtlscon.c
@@ +1135,5 @@
> +
> +SECStatus
> +SSL_SetRetransmitTimeoutDefault(PRIntervalTime timeout)
> +{
> +    if (timeout > MAX_DTLS_TIMEOUT_MS) {

Because there is a backoff, you need to leave some slack for it to back off. Can you look at the code for backoff and figure out what the max safe value is as a fraction of MAX_DTLS_TIMEOUT_MS.

@@ +1140,5 @@
> +        return SECFailure;
> +    }
> +
> +    dtls_timeout_ms = timeout;
> +    

Whitespace.

@@ +1151,5 @@
> +    sslSocket * ss = NULL;
> +
> +    if (timeout > MAX_DTLS_TIMEOUT_MS) {
> +        return SECFailure;
> +    }

Same point here about backoff.

::: security/nss/lib/ssl/ssl.h
@@ +957,5 @@
> +/*
> +** This function changes the DTLS retransmit timeout for a specific socket.
> +*/
> +
> +SSL_IMPORT SECStatus SSL_SetRetransmitTimeout(PRFileDesc *socket,

I agree with Meadhbh that these should be DTLS_...
TLS does not have a correspondng timeout.

The semantics of this should be that you are setting what's effectively an RTT estimate. So, you don't get to set the *current* timeout but rather the current initial timeout. Maybe we need a longer name?

::: security/nss/lib/ssl/sslimpl.h
@@ +314,5 @@
>      unsigned int requireSafeNegotiation : 1;  /* 22 */
>      unsigned int enableFalseStart       : 1;  /* 23 */
>      unsigned int cbcRandomIV            : 1;  /* 24 */
>      unsigned int enableOCSPStapling     : 1;  /* 25 */
> +    unsigned int dtlsTimeoutMs          : 16;  /* 33-48 */

Having this be a bitfield is kind of odd. I wonder if we should just call it unsigned short?

@@ +1259,5 @@
>  extern sslSessionIDLookupFunc  ssl_sid_lookup;
>  extern sslSessionIDCacheFunc   ssl_sid_cache;
>  extern sslSessionIDUncacheFunc ssl_sid_uncache;
>  
> +extern PRIntervalTime          dtls_timeout_ms;

I would be tempted to rename this "dtls_default_timeout_ms"

Comment 6

[Meadhbh Hamrick :mhamrick]

Attachment: renamed dtls_timeout_ms to dtls_default_timeout_ms, eliminated trailing whitespace

ekr and i gabbed about previous comments, he says checking timeouts against MAX_DTLS_TIMEOUT_MS is okay after all. modified dtls_timeout_ms to dtls_default_timeout_ms to be more obvious. also passing patches through `sed -e 's/^\+\(.*[^ ]\)[ ]*$/\+\1/'` to eliminate trailing spaces. EMACS used to be much better about eliminating trailing spaces for you; sic transit gloria munde.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 783265 [details] [diff] [review]
renamed dtls_timeout_ms to dtls_default_timeout_ms, eliminated trailing whitespace

Review of attachment 783265 [details] [diff] [review]:
-----------------------------------------------------------------

ekr is a better reviewer than me for whether the patch is actually correct with respect to what the feature is supposed to do.

There are several minor issues with the patch: style, organization, and error handling. I recommend asking wtc to review the patch after you update it to make sure he is happy with those aspects of it.

::: security/nss/lib/ssl/dtlscon.c
@@ +13,5 @@
>  #ifndef PR_ARRAY_SIZE
>  #define PR_ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0]))
>  #endif
>  
> +PRIntervalTime dtls_default_timeout_ms = INITIAL_DTLS_TIMEOUT_MS;

Normally all the code that deals with options on a socket is in sslsock.c. In this case, the option is DTLS-specific but I am not sure how much that matters given that you modify sslsock.c anyway. Ask wtc where he prefers these things to be (sslsock.c or this file).

I suggest you remove the INITIAL_DTLS_TIMEOUT_MS macro, moving the comment and initial value constant to the definition of dtls_default_timeout_ms.

@@ +1136,5 @@
> +SECStatus
> +SSL_SetRetransmitTimeoutDefault(PRIntervalTime timeout)
> +{
> +    if (timeout > MAX_DTLS_TIMEOUT_MS) {
> +        return SECFailure;

You must always call PORT_SetError before returning SECFailure.

In this case, PORT_SetError(SEC_ERROR_INVALID_ARGS);

@@ +1150,5 @@
> +{
> +    sslSocket * ss = NULL;
> +
> +    if (timeout > MAX_DTLS_TIMEOUT_MS) {
> +        return SECFailure;

missing PORT_SetError

@@ +1154,5 @@
> +        return SECFailure;
> +    }
> +
> +    ss = ssl_FindSocket(socket);
> +

no blank line here.

@@ +1159,5 @@
> +    if (!ss)
> +        return SECFailure;
> +
> +    if (!IS_DTLS(ss))
> +        return SECFailure;

PORT_SetError here.

@@ +1161,5 @@
> +
> +    if (!IS_DTLS(ss))
> +        return SECFailure;
> +
> +    ss->opt.dtlsTimeoutMs = timeout;

Look at how SSL_OptionSet and SSL_VerionRangeSet deal with locks. It seems like you should be doing the same thing in this function.

::: security/nss/lib/ssl/ssl.def
@@ +168,5 @@
> +SSL_SetRetransmitTimeout;
> +SSL_SetRetransmitTimeoutDefault;
> +;+    local:
> +;+*;
> +;+};

You should (almost) always write NSS patches against the NSS repository at ssh://hg.mozilla.org/projects/nss and NOT against mozilla-central. Please rebase the patch on top of projects/nss.

When you modify *.def files in NSS, you must treat them as (more-or-less) append-only. In this case, you can add your new entries to the NSS 3.15.2 section because we haven't released NSS 3.15.2 yet. If we had already released NSS 3.15.2 then you would have to create an NSS 3.15.3 section. (see TAG-INFO). Don't add any "beta" designations.

::: security/nss/lib/ssl/ssl.h
@@ +949,5 @@
>  
> +
> +/*
> +** This function changes the global default retransmit timeout for DTLS.
> +*/

No blank line.

@@ +956,5 @@
> +
> +/*
> +** This function changes the DTLS retransmit timeout for a specific socket.
> +*/
> +

No blank line.

::: security/nss/lib/ssl/sslimpl.h
@@ +314,5 @@
>      unsigned int requireSafeNegotiation : 1;  /* 22 */
>      unsigned int enableFalseStart       : 1;  /* 23 */
>      unsigned int cbcRandomIV            : 1;  /* 24 */
>      unsigned int enableOCSPStapling     : 1;  /* 25 */
> +    unsigned int dtlsTimeoutMs          : 16;  /* 33-48 */

I suggest that you don't make this part of the bitfield. See "SECItem nextProtoNego" at the top of the struct.

Now that I am looking at this, it seems we haven't been consistent between putting non-boolean options in sslOptionStr vs. the socket itself. (For example, the version range is not in sslOptionsStr, but just sslSocketStr.) I do not have a strong opinion either way. Another thing to ask wtc about.

Comment 8

[Meadhbh Hamrick :mhamrick]

so... can we split this into two bugs? we could keep this bug to track the functionality that ekr requested and then open a new one to track the per-socket work and wtc review that brian wants?