Re: [tram] Adam Roach's Discuss on draft-ietf-tram-stunbis-16: (with DISCUSS and COMMENT)

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

Hi Adam,

Thanks for the review, please see my responses inline.

This is also the last review I can process until Saturday 28.  The remaining of the work to do on stunbis is:

- process Alexey Melnikov review
- process Process Benjamin Kaduk review
- add text for Dale Worley point on IPv6 NAT
- add text explaining the bid-attack protection mechanism
- work on answer to §6.2.3 below
- answer additional concerns following my responses
- final review for language
- publish -17


On 04/17/2018 11:03 PM, Adam Roach wrote:
> Adam Roach has entered the following ballot position for
> draft-ietf-tram-stunbis-16: Discuss
> 

[...]

> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Thanks to everyone who worked on this revision of the STUN protocol. Thanks in
> particular to the ARTART reviewer and to the authors for actively engaging on
> the points he raised.
> 
> I have one concern about interoperability and another about the IANA changes
> that I believe require changes to the document prior to publication.
> 
> §14.2:
> 
>>  X-Port is computed by taking the mapped port in host byte order,
>>  XOR'ing it with the most significant 16 bits of the magic cookie, and
>>  then the converting the result to network byte order.  If the IP
>>  address family is IPv4, X-Address is computed by taking the mapped IP
>>  address in host byte order, XOR'ing it with the magic cookie, and
>>  converting the result to network byte order.  If the IP address
>>  family is IPv6, X-Address is computed by taking the mapped IP address
>>  in host byte order, XOR'ing it with the concatenation of the magic
>>  cookie and the 96-bit transaction ID, and converting the result to
>>  network byte order.
> 
> The discussion of performing operations "in host byte order" is very confusing,
> and seems likely to cause issues communicating between machines of different
> endianness. As an implementor, based on this description, I cannot tell
> whether, given a port of 0x1234 (and operating on a little-endian machine),
> I'm supposed to do:
> 
> Port (host order):   34 12
> Magic Cookie Prefix: 21 12
> Result (host order): 15 00
> X-Port (net order):  00 15
> 
> or:
> 
> Port (host order):   34 12
> Magic Cookie Prefix: 12 21
> Result (host order): 26 33
> X-Port (net order):  33 26
> 
> One of these is clearly wrong. I think it's the first one, but I *also* think
> that the first one is the most straightforward interpretation of the quoted
> paragraph.
> 
> The following would seem to be a complete description of the
> operation without introducing possible confusion about the difference between
> host and network order:
> 
>    X-Port is computed by XOR'ing the mapped port with the most significant 16
>    bits of the magic cookie.  If the IP address family is IPv4, X-Address is
>    computed XOR'ing the mapped IP with the magic cookie.  If the IP address
>    family is IPv6, X-Address is computed by XOR'ing the mapped IP address with
>    the concatenation of the magic cookie and the 96-bit transaction ID. In all
>    cases, the XOR operation works on its inputs in network byte order (that is,
>    the order they will be encoded in the message).
> 
> This makes it clear that the proper operation is:
> 
> Port:                12 34
> Magic Cookie Prefix: 21 12
> Result / X-Port:     33 26

It seems that you are advocating the opposite of what is traditionally done on that subject:  Always do all the calculations using the computer order and eventually convert to/from the network order when writing/reading a packet.  That basically force all little-endian computers (which is most computers nowadays) to do their work in big-endian format.

If there is some ambiguity in the text, I am all for fixing it, for example by saying that the 16 bits of the magic cookie, the magic cookie and the concatenation of the magic cookie and the 96-bit transaction ID should all be converted into host byte order before been used with the XOR operation - which is done automatically when loading them in integer variables.

> 
> ---------------------------------------------------------------------------
> 
> §17.3.1:
> 
>>  IANA is requested to update the names for attributes 0x0002, 0x0003,
>>  0x0004, 0x0005, 0x0007, and 0x000B, and the reference from RFC 5389
>>  to RFC-to-be for the following STUN methods:
> ...
>>  0x0003: (Reserved; prior to [RFC5389] this was CHANGE-REQUEST)
> 
> The attribute 0x0003 is registered by RFC 5780, and should not be removed by this document.

Fixed.

> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> §3:
> 
> This is almost, but not quite, the boilerplate defined by RFC 8174. Please
> update to match the text in RFC 8174.

Already fixed following a previous review.

> 
> ---------------------------------------------------------------------------
> 
> §5:
> 
>>  server to detect if the client will understand certain attributes
>>  that were added in this revised specification.
> 
> This is a *bit* misleading, as these attributes were actually added in RFC 5389.
> Consider: "...that were added to STUN by [RFC5389]."

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §6.1:
> 
>>  If the agent is sending a request, it SHOULD add a SOFTWARE attribute
>>  to the request.
> 
> I believe this would benefit from a pointer to the security section; e.g.:
> "Note that the inclusion of a SOFTWARE attribute may have security
> implications; see Section 15.1.2 for details."

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §6.2:
> 
>>  STUN may be used
>>  with anycast addresses, but only with UDP and in usages where
>>  authentication is not used.
> 
> This bit of operational advice seems out of place in the middle of an
> implementation discussion, and is quite likely to be missed by its intended
> audience. Consider relocating it to an "Operational Considerations" section.

Done.

> 
> ---------------------------------------------------------------------------
> 
> §6.2.1:
> 
>>  As with TCP, the usage of Karn's
>>  algorithm is RECOMMENDED [KARN87].
> 
> This normative language means that [KARN87] needs to be a normative reference.

Done.

> 
> ---------------------------------------------------------------------------
> 
> §6.2.3 says:
> 
>>  Alternatively, a
>>  client MAY be configured with a set of IP addresses that are trusted;
>>  if a certificate is received that identifies one of those IP
>>  addresses, the client considers the identity of the server to be
>>  verified.
> 
> Presumably, this means the server supplies a certificate with SubjectAltName
> containing an iPAddress? Please add text to clarify whether that's the
> intention.
> 
> If that *is* the intended meaning, then this behavior in §8.1 seems
> unnecessarily restrictive:
> 
>>  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.
> 
> Presumably, this is done because certs with iPAddress-form SubjectAltNames are
> pretty rare (although CAB Forum baseline requirements do explicitly allow
> their issuance) -- but if the text in §6.2.3 is based on allowing the use of
> such certs in a TURN deployment, then it seems that URI resolution should be
> also.
> 

I am not sure what was the intent there, so I'll work on that later.

> ---------------------------------------------------------------------------
> 
> §9.1.3:
> 
>>     If the MESSAGE-
>>     INTEGRITY-SHA256 attribute is not present, and using the same
>>     password, compute the value for the message integrity as described
>>     in Section 14.5.
> 
> I can't figure out what is meant by "and using the same password" here. The
> structure of the sentence would imply that the subject of "using" is "attribute"
> (the one that is not present), but that doesn't make sense. Is it supposed to
> be something like this?
> 
>       If the MESSAGE-INTEGRITY-SHA256 attribute is not present, compute the
>       value for the message integrity as described in Section 14.5, using the
>       same password as would have been used for MESSAGE-INTEGRITY-SHA256.

Already fixed following a previous review.

> 
> ---------------------------------------------------------------------------
> 
> §9.1.5:
> 
>>  A client sending subsequent requests to the same server MUST send
>>  only the MESSAGE-INTEGRITY-SHA256 or the MESSAGE-INTEGRITY attribute
>>  that matches the attribute that was received in the response to the
>>  initial request.
> 
> How long does it continue to do so? It seems sensible to do this for the
> length of validity of the credentials (and no longer) -- and probably even
> scoped to the credentials --  but the document really should call that out
> explicitly.

Doing that would mean that we expect that the server could revert from stunbis to RFC 5389 during a session or conversely can upgrade from RFC 5389 to stunbis during a session.  I am all for upgradability, but doing would require more analysis of the protocol to be sure that it is safe to do so (the analysis done where only assuming that the RFC level is fixed after the first round).

In other words, it looks like a new feature to me.

> 
> ---------------------------------------------------------------------------
> 
> §9.2.2:
> 
>>  The structure of the key when used with long-term credentials
>>  facilitates deployment in systems that also utilize SIP [RFC3261].
>>  Typically, SIP systems utilizing SIP's digest authentication
>>  mechanism do not actually store the password in the database.
>>  Rather, they store a value called H(A1), which is equal to the key
>>  defined above.
> 
> Suggest adding something like: "For example, this mechanism can be used with the
> authentication extensions defined in [RFC5090]."

Added.

> 
> ---------------------------------------------------------------------------
> 
> §9.2.3.2:
> 
>>  Once a request/response transaction has completed successfully, the
> 
> The use of "successfully" here is kind of confusing, since the transaction in
> question was completed with an Error Response. Suggest dropping "successfully".
> 
> ---------------------------------------------------------------------------

Applied.

> 
> §9.2.4:
> 
>>  o  If the NONCE is no longer valid, the server MUST generate an error
>>     response with an error code of 438 (Stale Nonce).  This response
>>     MUST include NONCE, REALM, and PASSWORD-ALGORITHMS attributes and
>>     SHOULD NOT include the USERNAME, USERHASH attribute, The NONCE
>>     attribute value MUST be valid.
> 
> Nit: The comma before the final "The" should be a period.

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §9.2.4:
> 
>>     USERHASH attribute.  The response MAY include a MESSAGE-INTEGRITY
>>     or MESSAGE-INTEGRITY-SHA256 attribute, using the previous password
>>     to calculate it.
> 
> I think this means to say "using the previous key," right?
> 
> ---------------------------------------------------------------------------

Applied.

> 
> §14:
> 
>>  The
>>  padding bits are ignored, and may be any value.
> 
> This seems to be encouraging implementors to use uninitialized memory for these
> bits, which could be a security issue. It would be far safer to do the more
> traditional "must be set to zero on sending, must be ignored by receiver" thing
> here.

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §14.5:
> 
>>  Petit-Huguenin, et al.  Expires September 6, 2018 [Page 40]
>>
>>  Internet-Draft Session Traversal Utilities for NAT (STUN) March 2018
>>  Similarly, when validating the MESSAGE-INTEGRITY, the length field in
> 
> This appears to be an errant header/footer pair. Note that the
> correctly-formatted footer for page 40 appears several paragraphs later.
> 

Already fixed following a previous review.

> ---------------------------------------------------------------------------
> 
> §14.7:
> 
>>  The 32-bit CRC is the one defined in ITU V.42 [ITU.V42.2002], which
>>  has a generator polynomial of
>>  x32+x26+x23+x22+x16+x12+x11+x10+x8+x7+x5+x4+x2+x+1.
> 
> Please correct the polynomial representation:
> 
> x^32 + x^26 + x^23 + x^22 + x^16 + x^12 + x^11 + x^10 + x^8 + x^7 + x^5 + x^4
> + x^2 + x + 1.

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §14.8:
> 
>>   The reason phrase is meant for user consumption
> 
> This is a very tricky assertion to maintain, unless you want to start working
> with language tags and reason phrase localization. I think you're far safer
> saying something like "The reason phrase is meant for diagnostic purposes."
> I would not encourage implementations to just display its contents to users
> without any localization discussion.

Applied.

> 
> ---------------------------------------------------------------------------
> 
> §15.2.4:
> 
> It's probably worth additionally noting that this attack can be trivially
> launched by the STUN server itself -- so users of STUN servers should have the
> same level of trust in them as any other node that can insert themselves into
> the communication flow.

Done.  I mostly reused the whole paragraph above.

> 
> ---------------------------------------------------------------------------
> 
> §15.3:
> 
> I'm surprised that there is no plan in here for cryptoagility of the USERHASH
> TLV. While a practical preimage attack on SHA-256 seems pretty implausible
> today, it would be nice to know that we have a remedy if we ever decide that we
> need to change things.
> 
> 

Good point.  I added the following paragraph at the end:

   Similarly if SHA256 is found to be compromised a new user-hash
   attribute and a new STUN Security Feature bit will be allocated in a
   Standard Track document.  The new user-hash attribute will have its
   value computed using a new hash.  The STUN Security Feature bit will
   be used to simultaneously signal to a STUN client using the Long Term
   Credential Mechanism that this server supports this new hash
   algorithm for the user-hash, and will prevent bid down attacks on the
   new user-hash attribute.

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