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

[Spencer Dawkins at IETF <[e-mail]>]

Dear Authors,

Now that Last Call for this draft has ended, could you folks take a look at
Dale's comments and respond?

Thanks!

Spencer, as responsible AD

On Mon, Feb 19, 2018 at 11:30 AM, Dale Worley <[e-mail]> wrote:

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