Re: [secdir] Review of draft-ietf-tram-stun-dtls-03

[Marc Petit-Huguenin <[e-mail]>]

Hi Simon,

Thanks for the review.  Please see my responses inline.

On 6/19/14, 6:10 AM, Simon Josefsson wrote:
> Hello,
> 
> I have reviewed this document, and believe it is ready with nits.
> 
> MAJOR:
> 
> * The document is silent on whether DTLS cookies is required or not
>   for STUN.  DTLS cookies is one of few security differences between
>   TLS and DTLS.  The DTLS document says servers SHOULD use cookies,
>   but as not requiring this can lead to amplification attacks, I
>   suggest adding a MUST here.  For example, add the following to the
>   end of section 3:
> 
>     Servers implementing this document MUST send DTLS cookies to protect
>     against some Denial of Service attacks.
> 
>   Of course, I'm not to decide this, and I'm certainly no STUN expert,
>   so the authors and/or WG should think about whether mandating DTLS
>   cookies makes sense for STUN.
> 
>   Note that section 4.6 require DTLS cookies for TURN.  Is this
>   difference intentional?  Or did I overlook where this draft requires
>   it for STUN too?

Yes, you are right, but I do not think that adding this for all the STUN
Usages is right, it should be added on a usage by usage basis.

The Nat Discovery Usage (section 4.1) and NAT Behavior Discovery Usage
(section 4.5) MUST definitively use the cookie in the same way the TURN
Usage is using it.

As for the other Usages (Connectivity Check - 4.2, Media Keep-Alive -
4.3 and SIP Keep-Alive - 4.4), I do not think that the cookie is useful,
based on slide 3 of the following presentation by Martin Thomson:

http://www.ietf.org/proceedings/interim/2012/06/12/rtcweb/slides/slides-interim-2012-rtcweb-2-7.pptx

This said, I'd like some guidance there.

So I will add the same text that is in section 4.6 to sections 4.1 and
4.5.  I will also add to section 4 some text saying that new STUN Usages
should discuss if it is appropriate to use the cookie or not.

> 
> * There is a bit of terminology mixup, and I think the origin of the
>   mixup is section 1 referring to "TLS-over-UDP" as "DTLS".  That is
>   unfortunate for two reasons: 1) DTLS is not bound to UDP; DTLS can
>   be used over TCP or other transports,

Well, the same can be said for TLS-over-TCP, e.g. RFC 3436 predates RFC
5389, but RFC 5389 still uses the TLS-over-TCP terminology.

I think what RFC 5389 meant is "in this spec, TLS means TLS-over-TCP",
and in the same way in -stun-dtls we mean "in this spec DTLS means
DTLS-over-UDP".

> and 2) DTLS is similar but not
>   identical to TLS.  DTLS is not the same as TLS run over UDP.

I agree.

>   Quoting section 1:
> 
>    TLS-over-UDP (referred to as DTLS [RFC6347]) offers the same security
>    advantages as TLS-over-TCP, but without the undesirable latency
>    concerns.
> 
>   I suggest to rewrite the paragraph into:
> 
>    STUN transported over DTLS [RFC6347] over UDP offers the same
>    security advantages as TLS-over-TCP, but without the undesirable
>    latency concerns.

I would prefer the following, as it establishes DTLS as an alias for
DTLS-over-UDP for the remaining of the spec:

"DTLS-over-UDP (referred to as DTLS [RFC6347]) offers the same security
 advantages as TLS-over-TCP, but without the undesirable latency
 concerns."

> 
>   Further, in section 3 clarify that the STUN transport "DTLS" is DTLS
>   over UDP which isn't otherwise explicit:
> 
>   OLD:
>    STUN [RFC5389] defines three transports: UDP, TCP, and TLS.  This
>    document adds DTLS as a valid transport for STUN.
>   NEW:
>    STUN [RFC5389] defines three transports: UDP, TCP, and TLS.  This
>    document adds DTLS (over UDP) as a valid transport for STUN.

With the text I propose above, we do not need this modification.

> 
> * Cipher suites.  Quoting the document:
> 
>    Instead of TLS_RSA_WITH_AES_128_CBC_SHA, which is the default cipher
>    suite for STUN over TLS, STUN over DTLS, at a minimum, MUST support
>    TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 and MUST support
>    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256. STUN over DTLS MUST prefer
>    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 and any other Perfect Forward
>    Secrecy (PFS) cipher suites over non-PFS cipher suites.
> 
>   I think this is good text, but still, some concerns:
> 
>    1) The word "support" is not precise.  Does it mean 1) that
>       implementers MUST implement it (but not necessarily enable
>       them), or that 2) all deployed clients MUST offer at least these
>       two ciphersuites, and that all servers MUST accept at least
>       these two ciphersuites if the client doesn't offer anything
>       else?  I assume the latter was the intent, but it can be read as
>       the former.
> 
>    2) I don't think "PFS cipher suites" is well-defined in this
>       context, as PFS is only briefly discussed in the TLS spec.
>       There is a bunch of DHE key exchange algorithms defined for TLS,
>       are all of them relevant?  Probably not the anonymous ones?
> 
>    3) The requirement to prefer some cipher suites is generally for both
>       client and server, but in DTLS they behave differently.  Clients
>       list a set of acceptable ciphersuites, and the server picks one.
>       Is it permitted for clients to list non-PFS ciphersuites at all?
> 
>    4) The text can lead to poor decisions for broken ciphers.  , There
>       are PFS ciphersuites out there based on broken ciphers, and
>       there are non-PFS ciphersuites that are not known to be broken.
>       For example, TLS_DHE_DSS_WITH_DES_CBC_SHA is PFS but DES is
>       clearly broken.  I don't think you want a server to prefer
>       TLS_DHE_DSS_WITH_DES_CBC_SHA over, say,
>       TLS_DH_RSA_WITH_AES_256_CBC_SHA256, right?
> 
>    5) Since you are explicit about ciphersuites, and appear to worry
>       about ciphersuite selection, consider forbidding DES and RC4
>       directly since they are known weak cipher.  This would resolve
>       the last concern too, I think.
> 
>   A straw-man:
> 
>    Implementations of STUN over DTLS, and deployed clients and
>    servers, MUST support TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 and
>    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, MAY support other
>    ciphersuites, and SHOULD NOT support any insecure ciphersuites.
>    Perfect Forward Secrecy (PFS) cipher suites MUST be preferred over
>    non-PFS cipher suites.  Cipher suites with known weaknesses, such
>    as those based on (single) DES and RC4, MUST NOT be used.

OK.  I just prepended your first sentence with "Instead of
TLS_RSA_WITH_AES_128_CBC_SHA, which is the default cipher suite for STUN
over TLS, "

> 
> MINOR:
> 
> * TCP ports != UDP ports.  Typo fix:
> 
> OLD:
>    By default, STUN over DTLS MUST use port 5349, the same port as STUN
> ..
>    By default, TURN over DTLS uses port 5349, the same port as TURN over
> NEW:
>    By default, STUN over DTLS MUST use port 5349, the same port number
> as STUN ..
>    By default, TURN over DTLS uses port 5349, the same port number as
> TURN over

OK

> 
> * SRV terminology mixup.
> 
> OLD:
>    SRV records, the service name MUST be set to "turns" and the
>    application name to "udp".
> ...
>    SRV records, the service name MUST be set to "stuns" and the
>    application name to "udp".
> NEW:
>    SRV records, the service name MUST be set to "turns" and the
>    protocol name to "udp".
> ...
>    SRV records, the service name MUST be set to "stuns" and the
>    protocol name to "udp".

OK

-- 
Marc Petit-Huguenin
Developer  |  Jive Communications, Inc.
Jive.com  |  [e-mail]