Re: [tram] Benjamin Kaduk's No Record on draft-ietf-tram-stunbis-16: (with COMMENT)

Marc Petit-Huguenin <petithug@acm.org> Wed, 02 May 2018 22:20 UTC

Return-Path: <petithug@acm.org>
X-Original-To: tram@ietfa.amsl.com
Delivered-To: tram@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 29F4F12DA3F; Wed, 2 May 2018 15:20:19 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.442
X-Spam-Level:
X-Spam-Status: No, score=-0.442 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RDNS_NONE=0.793, SPF_SOFTFAIL=0.665] autolearn=no autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x0koTTMEXatY; Wed, 2 May 2018 15:20:16 -0700 (PDT)
Received: from implementers.org (unknown [IPv6:2001:4b98:dc0:45:216:3eff:fe7f:7abd]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 160B412DA45; Wed, 2 May 2018 15:20:16 -0700 (PDT)
Received: from [IPv6:2601:648:8301:730f:388b:e133:9a29:637a] (unknown [IPv6:2601:648:8301:730f:388b:e133:9a29:637a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "Marc Petit-Huguenin", Issuer "implementers.org" (verified OK)) by implementers.org (Postfix) with ESMTPS id 750F1AE844; Thu, 3 May 2018 00:20:11 +0200 (CEST)
From: Marc Petit-Huguenin <petithug@acm.org>
To: Benjamin Kaduk <kaduk@mit.edu>, The IESG <iesg@ietf.org>
Cc: tram-chairs@ietf.org, tasveren@rbbn.com, Gonzalo.Camarillo@ericsson.com, draft-ietf-tram-stunbis@ietf.org, tram@ietf.org
References: <152410023763.28841.5479872591399614102.idtracker@ietfa.amsl.com>
Message-ID: <facc8bd7-2f14-4960-7d4d-181afb5545f9@acm.org>
Date: Wed, 2 May 2018 15:20:05 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <152410023763.28841.5479872591399614102.idtracker@ietfa.amsl.com>
Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CrUngAgkt2PCEhO0BQvMvrURlnxO8YS99"
Archived-At: <https://mailarchive.ietf.org/arch/msg/tram/1BbeYGaH696D1PUABFiZ6qFqyxs>
Subject: Re: [tram] Benjamin Kaduk's No Record on draft-ietf-tram-stunbis-16: (with COMMENT)
X-BeenThere: tram@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: "Discussing the creation of a Turn Revised And Modernized \(TRAM\) WG, which goal is to consolidate the various initiatives to update TURN and STUN." <tram.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/tram>, <mailto:tram-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/tram/>
List-Post: <mailto:tram@ietf.org>
List-Help: <mailto:tram-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/tram>, <mailto:tram-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 02 May 2018 22:20:19 -0000

Hi Benjamin,

Thanks for the review, please see my responses inline.

On 04/18/2018 06:10 PM, Benjamin Kaduk wrote:
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-tram-stunbis-16: No Record
> 
[...]
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> I'm balloting No Objection and not DISCUSS because no individual
> item rises quite to that level of criticality, but there still are
> quite a number of things that ought to be addressed prior to
> publication.
> 
> In addition to the excellent points raised in the secdir review
> (which are mostly addressed but only in the editor's copy?), I
> would call out a few more key observations:
> 
> If I understand correctly, the server's PASSWORD-ALGORITHMS array
> from the response are echoed back by the client in the subsequent
> request in order to provide downgrade protection -- the initial
> response is at least sometimes not authenticated, and by having the
> client repeat it back under an authenticated scheme, the server can
> detect if the list was tampered to remove any new, stronger,
> algorithms.  This is probably an important enough concept to be
> called out explicitly, either where the behavior is described or in
> Section 9.2.1 ("Bid Down Attack Prevention").usb

Yes, EKR had a similar question that I still have to answer, and I'll answer both after this review.

> 
> Section 8.1 has some text that could be read as requiring a STUN
> client to implement dual-stack IPv4/IPv6, which presumably is not
> the intention:
>    In addition, instead of querying either the A or the AAAA resource
>    records for a domain name, the client MUST query both and try the
>    requests with all the IP addresses received, as specified in
>    [RFC8305].

New text:

   In addition, instead of querying either the A or the AAAA resource
   records for a domain name, a dual-stack IPv4/IPv6 client MUST query
   both and try the requests with all the IP addresses received, as
   specified in [RFC8305].

> 
> There are still some places where the text talks about
> MESSAGE-INTEGRITY when it really means "message integrity
> protection" (i.e., either the SHA1 or SHA-256 variants, for now);
> I've attempted to note them in the notes below.
> 
> There is text in both Sections 9.1 and 9.2 that talks of replay
> protection being provided, by the time-limited nature of the
> short-term credentials or the nonce for long-term credentials.  This
> seems to only, strictly speaking, be true if any given
> password/nonce can only be used once, but for the short-term
> credentials the password is used for the duration of the
> "session-equivalent", and for the long-term credentials a nonce can
> be reused for some amount of time as well.  So while these are both
> replay countermeasures that can limit replay attacks, it seems a
> stretch to claim that they prevent replay attacks (entirely).

s/prevented/limited/

> 
> The Stale Nonce behavior seems potentially worrisome, in that it
> opens up a side channel for a distinguishing attack,
> between a 401 and 438 response.  (That is, "password correct" vs.
> "password incorrect".)  The impact seems rather muted, though, since
> the gain to the attacker is to be able to precompute a bunch of
> requests using a nonce of the attacker's choosing and blindly replay
> the precomputed object against (possibly multiple) servers looking
> for a guess.  The realm and username are still in play, so the scope
> for the attacker to gain from the precomputation seems limited.
> (The same level of brute-force guessing can be obtained "live" just
> by computing the trial responses against a live server, using a
> valid nonce.)  So, while it might be nice to give guidance that 438
> should only be used when the server can validate that it did
> generate the nonce and the nonce was valid "recently", and to treat
> other cases as authentication failures, it's not clear to me that
> there's enough of a benefit from the change to make it worth doing.

I am not sure what to do there.

> 
> 
> Section 6.2.3
> 
> Maybe just cite BCP 195/RFC 7525 instead of inlining requirements?

Already fixed following a previous review.

> 
> Also, the "client SHOULD verify ..." and "client MUST verify ..." are
> similar enough that some clarification of certificate vs. identity
> verification would be in order, if both statements are believed
> accurate.

Changed SHOULD to MUST.

> 
> 
> Section 6.3
> 
>    [...] Known-but-unexpected attributes SHOULD be ignored by
>    the agent. [...]
> 
> Why is this not an error?

Probably an application of Postel's law but that text was already in RFC 5389 so we cannot change it without breaking compatibility with RFC 5389 implementations.

> 
> 
> Section 6.3.1, 6.3.2, etc.
> 
> Should the second paragraph start with "Otherwise, "?  Presumably
> once an error response is sent or processing ceases, processing stops...

Applied.

> 
> 
> Section 8.1
> 
>    [...] A "stuns" URI
>    containing an IP address MUST be rejected, unless the domain name is
>    provided by the same mechanism that provided the STUN URI, and that
>    domain name can be passed to the verification code.
> 
> "the verifcation code" is probably unnecessarily vague; we're just
> talking about the (D)TLS SNI and certifciate verification code,
> right?

New text:

   [...]
   IP address is used directly to contact the server.  A "stuns" URI
   containing an IP address MUST be rejected, unless the domain name is
   provided by the same mechanism that provided the STUN URI, and that
   domain name can be passed to the (D)TLS SNI and certificate
   verification code.

> 
> 
> 
> Section 9.2
> 
>    [...[ The nonce provides the replay protection.  It is a cookie,
>    selected by the server, and encoded in such a way as to indicate a
>    duration of validity or client identity from which it is valid.
> 
> Is this like a TLS cookie, where only the server needs to know about
> the internal structure (viz. "encoded" in the above)?  If so, the
> text could probably be more clear.

New text:

   [...]
   a nonce.  The nonce provides the replay protection.  It is a cookie,
   selected by the server, and encoded in such a way as to indicate a
   duration of validity or client identity from which it is valid.  Only
   [...]

> 
> When mentioning just "a message-integrity", maybe forward-ref both
> versions of it to be more clear that we don't mean just the old one?

New text:

   [...]
   the realm, and echoing the nonce provided by the server.  The client
   also includes one of the message-integrity attributes defined in this
   document, which provides an HMAC over the entire request, including
   the nonce.  The server validates the nonce and checks the message
   [...]

> 
> It's unclear why indications can't use a nonce, if one is known
> from a previous request/response on the same connection.

I think that the explanation is clear: "[...] indications cannot be challenged." and as there is no way to know the when a cookie will expire, we cannot risk losing data for that reason.

> 
> When encoding the value of 0 for the security feature, it may be
> worth clarifying that it is still encoded as a 24-bit integer and
> base64-encoded, though the "13 character string" text ought to make
> this clear to the reader.

New text:

   [...]
   as the less significant bit of the third byte.  If no security
   features are used, then an integer with all 24 bit set to zero MUST
   be encoded instead.  For the remainder of this document the term
   [...]

> 
> 
> Section 9.2.4
> 
>       *  If the request contains neither PASSWORD-ALGORITHMS nor
>          PASSWORD- ALGORITHM, then the request is processed as though
>          PASSWORD- ALGORITHM were MD5 (Note that if the original
>          PASSWORD-ALGORITHMS attribute did not contain MD5, this will
>          result in a 400 Bad Request in a later step below).
> 
> This Note seems a bit odd, since we're predicated on
> PASSWORD-ALGORITHMS being absent.

New text:

      *  If the request contains neither PASSWORD-ALGORITHMS nor
         PASSWORD-ALGORITHM, then the request is processed as though
         PASSWORD-ALGORITHM were MD5 (Note that if the PASSWORD-
         ALGORITHMS attribute is present but does not contain MD5, this
         will result in a 400 Bad Request in a later step below).

> 
>       *  Otherwise, unless (1) PASSWORD-ALGORITHM and PASSWORD-
>          ALGORITHMS are both present, (2) PASSWORD-ALGORITHMS matches
>          the value sent in the response that sent this NONCE, and (3)
>          PASSWORD-ALGORITHM matches one of the entries in PASSWORD-
>          ALGORITHMS, the server MUST generate an error response with an
>          error code of 400 (Bad Request).
> 
> Maybe we could be more clear about "the value of the
> PASSWORD-ALGORITHMS attribute in the message matches the value of
> the PASSWORD-ALGORITHMS attribute sent in the response that sent the
> NONCE used with this message"?

I am not sure what to do there.

> 
> 
> Section 9.2.5
> 
>    If the response contains a PASSWORD-ALGORITHMS attribute, the
>    subsequent request MUST be authenticated using MESSAGE-INTEGRITY-
>    SHA256 only.
> 
> This means "all subsequent requests to this server"?  Maybe the
> wording could be more clear.  (It also means that if we get a new
> scheme when SHA256 is broken, the document introducing it may need
> to Update this document to remove the MUST.)

Fixed.

> 
> Also, I think the Security Feature bit is "Password algorithms"
> plural (in the next paragraph and maybe multiple locations in the
> document).

Fixed.

> 
> 
> Section 10
> 
>    [...] If the transaction uses TLS or DTLS and if the
>    transaction is authenticated by a MESSAGE-INTEGRITY-SHA256 attribute
>    and if the server wants to redirect to a server that uses a different
>    certificate, then it MUST include an ALTERNATE-DOMAIN attribute
>    containing the subjectAltName of that certificate.
> 
> The (definite article), implying the entire subjectAltName?  Or just
> one name that is contained as a SAN entry?

New text:

   [...]
   or practical.  If the transaction uses TLS or DTLS and if the
   transaction is authenticated by a MESSAGE-INTEGRITY-SHA256 attribute
   and if the server wants to redirect to a server that uses a different
   certificate, then it MUST include an ALTERNATE-DOMAIN attribute
   containing the name contained in the subjectAltName of that
   certificate.  The test on the MESSAGE-INTEGRITY-SHA256 attribute
   [...]

> 
> 
> Section 14.8
> 
> Need to not just say MESSAGE-INTEGRITY since the SHA256 variant
> should be fine as well.

Fixed.

> 
> 
> Section 14.10
> 
> Maybe the nonce cookie should get a shout-out here?

Not sure, as when using stunbis with RFC 5389, the nonce cookie will not be present.

> 
> 
> Section 14.12
> 
> Do we need to talk about the padding, since this is the the sole
> contents of the attribute and the attribute padding suffices?

Added similar text that was added to 14 about setting the padding bits to zero.

> 
> Also, "derive the long-term password" doesn't seem quite right;
> we're deriving the key from the long-term password, per Section
> 17.15.1.

New text:

   The PASSWORD-ALGORITHM attribute is present only in requests.  It
   contains the algorithms that the server must use to derive a key from
   the long-term password.

> 
> 
> Section 15.1.2
> 
>    There is no amplification of the number of packets with this attack
>    (the STUN server sends one packet for each packet sent by the
>    client), though there is a small increase in the amount of data,
>    since STUN responses are typically larger than requests.
> 
> I usually hear amplification in terms of bytes or bandwidth, not
> packets.  So maybe just "amplification is modest"?

That text is unchanged from RFC 5389.  Additionally the text says that "[...]there is a small increase in the amount of data,"

> 
> 
> Section 15.3
> 
>    This specification uses both HMAC-SHA1 and HMAC-SHA256 for
>    computation of the message integrity.
> 
> Could perhaps be reworded, since the number of situations when both
> are used on the same message is small, and we try to push SHA256
> when we can.

New text:

   This specification uses both HMAC-SHA256 for computation of the
   message integrity, sometimes in combination with HMAC-SHA1.  If, at a
   [...]

> 
>    o  STUN Client and Server using the Short Term Credential Mechanism
> 
> Clients and Servers, plural.

Fixed.

> 
> 
> Section 17.32.
> 
> I'm confused why we have MUST-level requirements about using
> PASSWORD-ALGORITHMS when it is in the comprehension-optional range
> (e.g., echoing from response to repeated request as in Section
> 9.2.5).  The server cannot actually enforce this MUST when talking
> to an old client if we want to retain the comprehension-optional
> semantics, IIUC.

It is comprehension-optional so old clients does not break.  But old client MUST process them (comprehension-optional doe snot mean that processing them is optional, but that an agent should not fail when seeing them and not understanding them).

> 
> 
> Section 17.6
> 
> If we're adding support for DTLS-over-udp (per the next section),
> should we be registering stuns/udp?

Adam explained why it is not needed.

> 
> "password encryption algorithm" is not quite the right statement to
> make, since we're hashing, not encrypting.
> 

I am not what this is about, as these words are not used in this section (or in the whole document).

-- 
Marc Petit-Huguenin
Email: marc@petit-huguenin.org
Blog: https://marc.petit-huguenin.org
Profile: https://www.linkedin.com/in/petithug