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

[Benjamin Kaduk <[e-mail]>]

On Wed, May 02, 2018 at 03:20:05PM -0700, Marc Petit-Huguenin wrote:
> Hi Benjamin,
> 
> Thanks for the review, please see my responses inline.

Sorry for the slow reply.

> 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].

SGTM.

> > 
> > 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/

Thanks :)

> > 
> > 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.

"Nothing" may be the right answer, though I guess there was a touch
more follow-up on this point already.

> > 
> > 
> > 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.

WFM

> > 
> > 
> > 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.

Ah, agreed about the compatibility implications.

> > 
> > 
> > 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.

WFM.

> > 
> > 
> > 
> > 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
>    [...]

I think the copy/paste was from the wrong spot, but looking at the
diff, the new text works for me.

> > 
> > 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
>    [...]

WFM

> > 
> > 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.

Okay.

> > 
> > 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
>    [...]

Okay

> > 
> > 
> > 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).

Ah, this makes more sense.

> > 
> >       *  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.

I think I was trying to propose:

       *  Otherwise, unless (1) PASSWORD-ALGORITHM and PASSWORD-
	  ALGORITHMS are both present, (2) the value of the
	  PASSWORD-ALGORITHMS attribute in this message matches the
	  value of the PASSWORD-ALGORITHMS attribute sent in the
	  response that sent the NONCE used with this message, 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).

Though I suspect that "this message" has a more appropriate term
that could be used instead.

> > 
> > 
> > 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
>    [...]

I think "a name contained in the subjectAltName of that certificate"
is the expected meaning ("the" --> "a"), since SAN can be
multi-valued.

> > 
> > 
> > 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.

Even "in some cases, this value will include a 'nonce cookie' as
defined previously in the document" might be useful, but I'll defer
to your judgment.

> > 
> > 
> > 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.

Thanks

> > 
> > 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.

SGTM

> > 
> > 
> > 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,"

I don't think that faithfulness to RFC 5389 need apply here (this is
a full-fledged 'bis' version), but I also don't insist on any
change; it's a very minor issue.

> > 
> > 
> > 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
>    [...]

"both" seems out of place, now.

> > 
> >    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).

Probably I was just confused on my first read, sorry about that.
(A client *implementing this spec* MUST echo PASSWORD-ALGORITHMS
when generating a new request after an error response, but of course
older clients don't implement this spec.)

> > 
> > 
> > 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).

I see "password encryption algorithm" in the "Changes since RFC
5389" section, even in the -18.


Thanks for all the explanations and updates!

Benjamin