[Gen-art] Genart last call review of draft-ietf-tram-stunbis-15

[Dale Worley <[e-mail]>]

Reviewer: Dale Worley
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft.  The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

Document:  draft-ietf-tram-stunbis-15
Reviewer:  Dale R. Worley
Review Date:  2018-02-19
IETF LC End Date:  2018-02-20
IESG Telechat date:  [unknown]

Summary:

       This draft is on the right track but has open issues, described
       in the review.

Overall, the draft is quite well organized and well written.  But
there are a number of open issues that have substantial technical
content.  I suspect that the intended design does not have these
issues, and the issues that I see are just where the text hasn't been
fully updated to match the intended design.

Dale

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

There is an issue that shows up in several places:  The NAT may
forward the request using an IP family that is different from the IP
family that it received the request using.  This means that the
"source IP family of the request" may depend on whether one is
speaking of the client or the server.  The draft is cognizant of this,
and mentions its consequences in sections 6.3.3 and 12.  But this also
has consequences for ALTERNATE-SERVER:  Section 14.15 says "The IP
address family MUST be identical to that of the source IP address of
the request." even though that family might not be usable by the
client.  The draft doesn't seem to explicitly say that this comes from
address-switching by the NAT.  It would help if there was a
higher-level discussion of this matter, pointing to the various
consequences.

The text contains these references to SHA algorithms (that it does not
itself define).  Some should be corrected:

    HMAC-SHA1
    HMAC-SHA-1
should be HMAC-SHA1 per RFC 2104
    HMAC-SHA256
    HMAC-SHA-256
should be HMAC-SHA256 per RFC 6151, but HMAC-SHA-256 per RFC 6151!
    SHA1
    SHA-1
It appears that the proper name of this function is SHA-1.
    SHA256
    SHA-256
NIST calls this algorithm SHA-256.

3.  Terminology

This section needs to be updated to reference RFC 8174.

4.  Definitions

Although the definitions of STUN Client and STUN Server mention that
they receive responses and requests (respectively), they don't mention
that they also receive indications.

5.  STUN Message Structure

   All STUN messages MUST start with a 20-byte header followed by zero
   or more Attributes.

It would be clearer, I think, to say "All STUN messages comprise a
20-byte header followed by zero or more Attributes."

6.2.2.  Sending over TCP or TLS-over-TCP

   The client MAY send multiple transactions over a single TCP (or TLS-
   over-TCP) connection, and it MAY send another request before
   receiving a response to the previous.

s/the previous./the previous request./

This section should also state whether or not a server is allowed to
provide responses in a different order than it received the requests,
if it receives further requests before sending a response.

   o   if multiplexing other application protocols over that port, has
       finished using that other application, and

s/that other application/that other protocol/

6.3.4.  Processing an Error Response

    o  If the error code is 300 through 399, the client SHOULD consider
       the transaction as failed unless the ALTERNATE-SERVER extension is
       being used.  See Section 10.

This syntax binds "section 10" to the entire preceding sentence, whose
topic is error codes 300-399, whereas "section 10" only applies to
ALTERNATE-SERVER.  A better syntax would be

    o  If the error code is 300 through 399, the client SHOULD consider
       the transaction as failed unless the ALTERNATE-SERVER extension
       (Section 10) is being used.

9.2.  Long-Term Credential Mechanism

   Note that the long-term credential mechanism cannot be used to
   protect indications, since indications cannot be challenged.  Usages
   utilizing indications must either use a short-term credential or omit
   authentication and message integrity for them.

Alternatively, if there has been a recent request transaction between
the client and the server, then a nonce have been established, and an
indication can be sent securely using the long-term mechanism.
(Although there is no mechanism for signaling if the nonce has become
stale.)  It seems to me plausible that some usage may wish to exploit
this possibility.

9.2.1.  Bid Down Attack Prevention

   To indicate that it supports this specification, a server MUST
   prepend the NONCE attribute value with the character string composed
   of "obMatJos2" concatenated with the Base64 [RFC4648] encoding of the
   24 bit STUN Security Features as defined in Section 17.1.

It might be informative to note that the encoding of the security
features is always four characters long:
s/the Base64 [RFC4648] encoding/the (4 character) Base64 [RFC4648] encoding/

9.2.2.  HMAC Key

   The structure of the key when used with long-term credentials
   facilitates deployment in systems that also utilize SIP.  Typically,
   SIP systems utilizing SIP's digest authentication mechanism do not
   actually store the password in the database.

Presumably there should be an explicit reference [RFC3261] here.

9.2.3.  Forming a Request

This section defines "first request from the client to the server" as
being "identified by its IP address and port".  However in section
9.2.3.1, "the server" is "identified by hostname, if the DNS
procedures of Section 8 are used, else IP address if not".  These are
not the same, and presumably need to be aligned on the correct
definition.  (And perhaps one section can simply refer to the
definition in the other section.)  Alternatively, they may be intended
to be different, in which case the text needs to be clarified and also
warn the reader.

9.2.3.1.  First Request

   In other words, the very first request is sent [...]

It seems better style to omit "very", given that "first request" has a
specific meaning.

9.2.4.  Receiving a Request

      The
      server MUST ensure that the same NONCE cannot be selected for
      clients that use different source IP addresses, different source
      ports, or both different source IP addresses and source ports.

It seems clearer to phrase this condition "The server MUST NOT choose
the same NONCE for two requests unless they have the same source IP
address and port."

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1 but
      PASSWORD-ALGORITHMS does not match the value sent in the response
      that sent this NONCE, then the server MUST generate an error
      response with an error code of 400 (Bad Request).

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1 but 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).

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1 but only
      one of PASSWORD-ALGORITHM or PASSWORD-ALGORITHMS is present, then
      the server MUST generate an error response with an error code of
      400 (Bad Request).

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1 but
      PASSWORD-ALGORITHM does not match one of the entries in PASSWORD-
      ALGORITHMS, then the server MUST generate an error response with
      an error code of 400 (Bad Request).

Given these cases all include one long condition, it seems like it
would be clearer if they were grouped and the condition factored out:

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1, the
      server performs these checks in the order specified:
							      
      *  If PASSWORD-ALGORITHMS does not match the value sent in the response
	 that sent this NONCE, then the server MUST generate an error
	 response with an error code of 400 (Bad Request).

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

      *  If 
	 only one of PASSWORD-ALGORITHM or PASSWORD-ALGORITHMS is present, then
	 the server MUST generate an error response with an error code of
	 400 (Bad Request).

      *  If PASSWORD-ALGORITHM does not match one of the entries in PASSWORD-
	 ALGORITHMS, then the server MUST generate an error response with
	 an error code of 400 (Bad Request).

This could be further shortened and clarified by using the negation of
the condition we desire:

   o  If the NONCE attribute starts with the "nonce cookie" with the
      STUN Security Feature "Password algorithm" bit set to 1, the
      server performs these checks in the order specified:
							      
      *  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).

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

--

      Servers can
      invalidate nonces in order to provide additional security.

It's not clear what "invalidate" means here.  The text has been
talking about nonces expiring, which suggests that it is not a process
that the server actively causes at the time the nonce becomes invalid,
but "invalidate" suggests that the server causes it at the moment of
invalidation.

      See Section 4.3 of [RFC7616] for guidelines.

There is no section 4.3 in RFC 7616.

9.2.5.  Receiving a Response

   If the test succeeds and the "nonce cookie" has
   the STUN Security Feature "Username anonymity" bit set to 1 but no
   USERHASH attribute is present, then the client MUST NOT retry the
   request with a new transaction.

I can find no reference to the server sending USERHASH in a response,
so I don't understand what this means.  I think what was intended is
"... is set to 1, then the client MUST NOT retry the request using the
USERHASH attribute."  But that requirement seems to be stated in the
next paragraph:

   If the response is an error response with an error code of 401
   (Unauthenticated), the client SHOULD retry the request with a new
   transaction.  [...]
   If the "nonce cookie" was present and had
   the STUN Security Feature "Username anonymity" bit set to 1 then the
   USERHASH attribute MUST be used, else the USERNAME attribute MUST be
   used.

--

   For all other responses, if the NONCE attribute
   starts with the "nonce cookie" with the STUN Security Feature "User
   anonymity" bit set to 1 but USERHASH is not present, the response
   MUST be ignored.

As above.

The above texts suggest that there is, or was, an intention that the
server would reflect back the USERHASH value in responses.  But
checking all the mentions of "USERHASH", I can find no text saying
that USERHASH will ever appear in responses.  This design decision
needs to be verified and the text updated correspondingly.

   If the response is an error response with an error code of 400, and
   does not contains either MESSAGE-INTEGRITY or MESSAGE-INTEGRITY-
   SHA256 attribute then the response MUST be discarded, as if it was
   never received.  This means that retransmits, if applicable, will
   continue.

Some responses generated according to this specification will not pass
the above check.  E.g., from section 9.2.4 item 2:

   o  If the message contains a MESSAGE-INTEGRITY or a MESSAGE-
      INTEGRITY-SHA256 attribute, but is missing either the USERNAME or
      USERHASH, REALM, or NONCE attribute, the server MUST generate an
      error response with an error code of 400 (Bad Request).  This
      response SHOULD NOT include a USERNAME, USERHASH, NONCE, or REALM.
      The response cannot contain a MESSAGE-INTEGRITY or MESSAGE-
      INTEGRITY-SHA256 attribute, as the attributes required to generate
      them are missing.

How does the client effectively receive the error response in this
situation?

10.  ALTERNATE-SERVER Mechanism

To be clearer, the first paragraph should mention that the
ALTERNATE-SERVER attribute carries an IP address, not a domain name
(see section 14.15).  In particular, that disambiguates the test for
"same server" in the last paragraph.

   The error response
   message MAY be authenticated; however, there are uses cases for
   ALTERNATE-SERVER where authentication of the response is not possible
   or practical.

s/uses cases/use cases/

   If the client has been redirected to a
   server on which it has already tried this request within the last
   five minutes, [...]

It is a little more formal to say "to a server to which it has already
sent this request ...".

11.  Backwards Compatibility with RFC 3489

   Any STUN request or indication
   without the magic cookie (see Section 6 of [RFC5389]) over DTLS MUST
   always result in an error.

The meaning is clear, but in this document, "error" seems to be
limited to error responses.  Perhaps better is:

   Any STUN request or indication
   without the magic cookie (see Section 6 of [RFC5389]) over DTLS MUST
   be considered invalid: requests MUST generate error responses and
   indications MUST be ignored.

Also, what error code should be used?

13.  STUN Usages

   A usage would also define:

I'm sure you don't want to use the subjunctive mood here, so perhaps
s/would/will/.  OTOH, you might want to s/would/must/, or "A usage
also defines:" to parallel the first sentence of the paragraph.

14.  STUN Attributes

Note that there is an important ordering restriction on attributes:
the attributes MESSAGE-INTEGRITY, MESSAGE-INTEGRITY-SHA256, and
FINGERPRINT must, if present, appear finally and in that order.  There
also seems to be a rule that any attributes which follow one of these
attributes in violation of that rule MUST be ignored.

This is described in those attribute definitions, but it's a global
constraint in the use of attributes, and I think should be pointed out
at this level of the specification.

I'm nervous about the "must be ignored" rule, as it becomes a little
complicated to do the processing right in all cases.  Perhaps it would
be better to declare that any such violation invalidates the message
instead?

14.3.  USERNAME

   The value of USERNAME is a variable-length value.  

This doesn't actually say what the value is.  Better:

   The value of USERNAME is a variable-length value containing the
   authentication username.

14.5.  MESSAGE-INTEGRITY

   Since it uses the SHA1 hash, the HMAC will be
   at 20 bytes.

I think "at" should be omitted.

   The text used as input to HMAC is the STUN message, including the
   header, up to and including the attribute preceding the MESSAGE-
   INTEGRITY attribute.  With the exception of the MESSAGE-INTEGRITY-
   SHA256 and FINGERPRINT attributes, which appear after MESSAGE-
   INTEGRITY, agents MUST ignore all other attributes that follow
   MESSAGE-INTEGRITY.

This paragraph is troublesome.  The matter of attribute ordering is
discussed above under section 14.  But the description of calculating
the MAC disagrees with the description in the fourth paragraph.  My
suspicion is that the fourth paragraph is correct.  My choice would be
to omit this paragraph and leave its topics to be dealt with
elsewhere.

   Based on the rules above, the hash used to construct MESSAGE-
   INTEGRITY includes the length field from the STUN message header.
   Prior to performing the hash, the MESSAGE-INTEGRITY attribute MUST be
   inserted into the message (with dummy content).  The length MUST then
   be set to point to the length of the message up to, and including,
   the MESSAGE-INTEGRITY attribute itself, but excluding any attributes
   after it.  Once the computation is performed, the value of the
   MESSAGE-INTEGRITY attribute can be filled in, and the value of the
   length in the STUN header can be set to its correct value -- the
   length of the entire message.  Similarly, when validating the
   MESSAGE-INTEGRITY, the length field should be adjusted to point to
   the end of the MESSAGE-INTEGRITY attribute prior to calculating the
   HMAC.  Such adjustment is necessary when attributes, such as
   FINGERPRINT, appear after MESSAGE-INTEGRITY.

I would rephrase this slightly, borrowing from the second paragraph:

   The text used as input to HMAC is the STUN message, up to and
   including the MESSAGE-INTEGRITY attribute.  The length field of the
   STUN message header is adjusted to point to the end of the
   MESSAGE-INTEGRITY attribute.  The value of the MESSAGE-INTEGRITY
   attribute is set to the dummy value ***.

   Once the computation is performed, the value of the
   MESSAGE-INTEGRITY attribute is filled in, and the value of the
   length in the STUN header is set to its correct value -- the
   length of the entire message.  Similarly, when validating the
   MESSAGE-INTEGRITY, the length field must be adjusted to point to
   the end of the MESSAGE-INTEGRITY attribute and the value of the
   attribute set to ***  prior to calculating the
   HMAC.  Such adjustment is necessary when attributes, such as
   FINGERPRINT, appear after MESSAGE-INTEGRITY.

Also, the text doesn't specify what the "dummy content" is!

14.6.  MESSAGE-INTEGRITY-SHA256

This section has the same problems regarding the HMAC calculation as
section 14.5.

The discussion of truncation seems to assume that the reader already
fully understands the concept.  Better would be to explain it
explicitly, since that doesn't take more words:

   The MESSAGE-INTEGRITY-SHA256 attribute can be present in any STUN
   message type.  The MESSAGE-INTEGRITY-SHA256 attribute contains an
   initial portion of the HMAC-SHA-256 [RFC2104] of the STUN message.
   The value will be at most 32 bytes and MUST be a positive multiple
   of 4 bytes.  The value must be the full 32 bytes unless the STUN
   usage explicitly specifies that truncation is allowed.  Usages may
   specify a minimum length longer than 4 bytes.

14.7.  FINGERPRINT

   The FINGERPRINT attribute MAY be present in all STUN messages.  The
   value of the attribute is computed as the CRC-32 of the STUN message
   up to (but excluding) the FINGERPRINT attribute itself, [...]

Note that unlike the MESSAGE-INTEGRITY attributes, the FINGERPRINT
calculation does not prepare the text by adjusting the Length field of
the header.  Verify that this is what is intended and perhaps mention
it explicitly.

   [...] up to (but excluding) the FINGERPRINT attribute itself, XOR'ed with
   the 32-bit value 0x5354554e (the XOR helps in cases where an
   application packet is also using CRC-32 in it).

This is awkward.  Perhaps unpack it into:

   The value of the attribute is computed as the CRC-32 of the STUN
   message up to (but excluding) the FINGERPRINT attribute itself,
   XOR'ed with the 32-bit value 0x5354554e.  (The XOR operation
   ensures that the FINGERPRINT test will not report a false positive
   on a packet containing a CRC-32 generated by an application
   protocol.)

14.8.  ERROR-CODE

   The Reserved bits SHOULD be 0, and are for alignment on 32-bit
   boundaries.  Receivers MUST ignore these bits.  The Class represents
   the hundreds digit of the error code.  The value MUST be between 3
   and 6.  The Number represents the error code modulo 100, and its
   value MUST be between 0 and 99.

How is Number encoded?  I suspect that binary is intended, but it is
an 8-bit field holding a two-digit decimal number, and so might
plausibly be encoded as two nibbles containing the two digits.

14.9.  REALM

   Presence in certain
   error responses indicates that the server wishes the client to use a
   long-term credential for authentication.

I think you mean s/a long-term credential/a long-term credential in
that realm/.

14.10.  NONCE

   Note that this means that the NONCE attribute will not contain
   actual quote characters.

More exactly, "will not contain the surrounding quote characters".

But some thought should be given to using the same wording in 14.9 and
14.10.

14.11.  PASSWORD-ALGORITHMS

   The parameters start with the actual length of the
   parameters as a 16-bit value, followed by the parameters that are
   specific to each algorithm.

"actual length" should be "length (prior to padding)".

14.12.  PASSWORD-ALGORITHM

   The parameters starts with the actual length of the
   parameters as a 16-bit value, followed by the parameters that are
   specific to the algorithm.

"actual length" should be "length (prior to padding)".

15.1.2.  Inside Attacks

   Fortunately, STUN
   requests can be processed statelessly by a server, making such
   attacks hard to launch.

Actually, such attacks are easy to launch, but "hard to launch
effectively".

   This attack is mitigated by ingress source address filtering.

I would help to explain who is filtering and on what basis to
implement this mitigation.

15.2.4.  Attack IV: Eavesdropping

   In this attack, the attacker forces the client to use a reflexive
   address that routes to itself.

"itself" usually refers to the subject of the verb in its clause,
which is "address".  But I think "attacker" is intended, in which case
you can phrase it "that routes to the attacker itself".

17.1.  STUN Security Features Registry

   New Security Features are assigned by a Standard Action [RFC8126].

s/Standard Action/Standards Action/ per RFC 8u126 section 4.9.

17.3.2.  New Attributes

   0xXXXX: PASSSORD-ALGORITHMS

s/PASSSORD/PASSWORD/

17.5.  Password Algorithm Registry

I think you want to title this registry "Stun password algorithm registry".

17.6.  STUN UDP and TCP Port Numbers

This section should state that it is updating "Service Name and
Transport Protocol Port Number Registry".

18.  Changes since RFC 5389

The following items should contain proper references to the mentioned RFCs:

   o  Added support for DTLS-over-UDP (RFC 6347).

   o  Aligned the RTO calculation with RFC 6298.

   o  Added support for STUN URI (RFC 7064).

   o  Updated the PRECIS support to RFC 8265.

[END]