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

[Adam Roach <[e-mail]>]

Adam Roach has entered the following ballot position for
draft-ietf-tram-stunbis-16: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-tram-stunbis/



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

---------------------------------------------------------------------------

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


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

§3:

This is almost, but not quite, the boilerplate defined by RFC 8174. Please
update to match the text in RFC 8174.

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

§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?

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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

---------------------------------------------------------------------------

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