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

Marc Petit-Huguenin <petithug@acm.org> Mon, 05 March 2018 17:07 UTC

Return-Path: <petithug@acm.org>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 17D1C12EB8F; Mon, 5 Mar 2018 09:07:05 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.441
X-Spam-Level:
X-Spam-Status: No, score=-0.441 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RDNS_NONE=0.793, SPF_SOFTFAIL=0.665, URIBL_BLOCKED=0.001] autolearn=no autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oKFfSNYfOkQF; Mon, 5 Mar 2018 09:06:59 -0800 (PST)
Received: from implementers.org (unknown [92.243.22.217]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id A142312EB5F; Mon, 5 Mar 2018 09:06:44 -0800 (PST)
Received: from [IPv6:2601:648:8301:730f:b85e:a711:a0c4:7650] (unknown [IPv6:2601:648:8301:730f:b85e:a711:a0c4:7650]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "Marc Petit-Huguenin", Issuer "implementers.org" (verified OK)) by implementers.org (Postfix) with ESMTPS id 75CD6AE8D9; Mon, 5 Mar 2018 18:06:41 +0100 (CET)
From: Marc Petit-Huguenin <petithug@acm.org>
To: Dale Worley <worley@ariadne.com>, gen-art@ietf.org
Cc: draft-ietf-tram-stunbis.all@ietf.org, ietf@ietf.org, tram@ietf.org
References: <151906144357.18629.1445621426796368031@ietfa.amsl.com>
Message-ID: <99f8564f-bcff-0f8b-cf32-8ab3c7692bb8@acm.org>
Date: Mon, 05 Mar 2018 09:06:38 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <151906144357.18629.1445621426796368031@ietfa.amsl.com>
Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="EOaH01Yjg9GoCq4kpYwfGBWL8NF5lBcCe"
Archived-At: <https://mailarchive.ietf.org/arch/msg/gen-art/CkxgkyAj22vmF6mT9Mh2ZFvQcaw>
Subject: Re: [Gen-art] [tram] Genart last call review of draft-ietf-tram-stunbis-15
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 05 Mar 2018 17:07:12 -0000

Thanks for the thorough review, please see inline.

I'll publish -16 today before the cut-off.

On 02/19/2018 09:30 AM, Dale Worley 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 placeqs:  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.

I agree that some high-level text on this is needed, not just because of the usage of STUNbis as a Binding server (in section 12), but also to provide some guidelines to other STUN Usages (e.g. TURN).

Unfortunately I am running out of time before the cut-off so I am going to publish without additional text addressing that concern, and will prepare something for the next version.

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

Fixed.

> 
> 3.  Terminology
> 
> This section needs to be updated to reference RFC 8174.

Already reported and fixed.

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

Fixed.

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

Applied.

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

Fixed.

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

This section talks about the sending side, so I do not think we should mix server side stuff in there.

So I added the following paragraph at the end of section 6.3.1.2:

  The server is allowed to send responses in a different order than it
  received the requests.

> 
>    o   if multiplexing other application protocols over that port, has
>        finished using that other application, and
> 
> s/that other application/that other protocol/

Applied.

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

Applied.

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

The consequence of doing this is that indications (most likely TURN indications carrying data) may be lost because the nonce either expired or was invalidated on the server side ("Servers can invalidate nonces in order to provide additional security.").  A specific STUN Usage may find a way to mitigate the issue for the former reason (e.g. adding a way for the client to know the nonce lifetime, and so be sure that a transaction is executed before that time), but mitigation for the latter are more complex (sending an indication in the reverse direction to signal the invalidation?).  We can just say that future usages will have to figure that out, but that seems a bit perilous.

So I'd like to hear from the WG on that.  Meanwhile I am deferring action on that comment.

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

Applied.

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

Added.

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

That text was already in RFC 5389, but I agree that it needs to be fixed.

I removed the text in 9.2.3.1, and used it as replacement in 9.2.3:

 9.2.3.  Forming a Request

  There are two cases when forming a request.  In the first case, this
  is the first request from the client to the server (as identified by
  hostname, if the DNS procedures of Section 8 are used, else IP
  address if not).  

  [...]

 9.2.3.1.  First Request

  If the client has not completed a successful request/response
  transaction with the server, it MUST omit the USERNAME, USERHASH,

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

Removed.

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

That's far better.

Applied.

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

Also far better that the original.

Applied.

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

It is an action by the operator of the server that immediately shorten the lifetime of the nonce and will trigger the allocation of a new nonce.  To align the terminology with RFC 7009, I changed the word "invalidate" to "revoke".

> 
>       See Section 4.3 of [RFC7616] for guidelines.
> 
> There is no section 4.3 in RFC 7616.

Already reported and fixed.

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

It looks like a bad case of copy and paste.  I removed the sentence altogether.

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

Also removed.

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

USERHASH is a replacement for USERNAME so it should never appear in a response.

The copy and paste mistake is probably because I forgot at the time that the "Username anonymity" and the "Password algorithms" are not exactly symmetrical. The latter requires in a response to be accompanied with a PASSWORD-ALGORITHMS attribute, whereas the former does not require any additional attribute.

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

When the transaction times out.  RFC 5389 does something similar.

See my email in the TRAM mailing-list on 12/20/2016 10:26 AM PT "More issues with RFC 5389".

But it took me a few hours to remember why that was done like this, so I added a note:

 Note:  In that case the 400 will never reach the application,
    resulting in a timeout.

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

Added.

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

Fixed.

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

Applied.

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

Applied

> 
> Also, what error code should be used?

I chose 500, the text now look like this:

 In addition to the backward compatibility already described in
 Section 12 of [RFC5389], DTLS MUST NOT be used with [RFC3489] (also
 referred to as "classic STUN").  Any STUN request or indication
 without the magic cookie (see Section 6 of [RFC5389]) over DTLS MUST
 be considered invalid: all requests MUST generate a "500 Server
 Error" error response and indications MUST be ignored.

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

I used "A usage also defines:"

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

The situation in RFC 5389:

1. Paragraph 3 of section 15 states that only the first occurence of an attribute needs to be processed and any duplicate MAY be ignored.  Thus opening the possibility of multiple occurrence of the same attribute type (which is now used in TURN to receive multiple XOR-RELAYED-ADDRESS attributes).

Notice that there is nothing that says the multiple occurrences must be consecutive.

2. Paragraph 1 of section 15.4 states that all attributes following MESSAGE-INTEGRITY must be ignored, at the exception of the FINGERPRINT attribute.

3. Paragraph 1 of section 15.5 states that the FINGERPRINT attribute MUST be the last attribute.

Now STUNbis uses (2) to its advantage by redefining that meaning, saying that a compliant implementation of STUNbis looks for a MESSAGE-INTEGRITY-265 after the MESSAGE-INTEGGRITY.  That's what makes both versions work with each other.

We need to keep the "must be ignored" rule because when MESSAGE-INTEGRITY-265 will be close to be broken, we will release STUNter that will add another MESSAGE-INTEGRITY-XXX attribute (that's how we do HMAC agility).  If we invalidate the message then a future upgrade will not be possible.

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

Applied.

> 
> 14.5.  MESSAGE-INTEGRITY
> 
>    Since it uses the SHA1 hash, the HMAC will be
>    at 20 bytes.
> 
> I think "at" should be omitted.

Removed.

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

I think that there is two issues here.  First is that the fact that agents MUST ignore everything but the MESSAGE-INTEGRITY-SHA256 and FINGERPRINT is irrelevant to the MESSAGE-INTEGRITY attribute.  I moved that sentence to the end of section 9 as this is common behavior for all authentication schemes:

  Note that agents MUST ignore all attributes that follow MESSAGE-
  INTEGRITY, with the exception of the MESSAGE-INTEGRITY-SHA256 and
  FINGERPRINT attributes.  Similarly agents MUST ignore all attributes
  that follow the MESSAGE-INTEGRITY-SHA256 attribute if the MESSAGE-
  INTEGRITY attribute is not present, with the exception of the
  FINGERPRINT attribute.

The second issue is that the fact that the input text stops at the end of the preceding attribute is not in the following paragraph.  See next answer for that.

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

No, the input is up to and including the attribute *preceding* the MESSAGE-INTEGRITY attribute.  The message attribute is only counted when calculating the length in the header (yeah, that's really confusing, but we had RFC 5769 to help).

So I reused your text, modified accordingly.  I also completely removed the second paragraph and added a reference to RFC 5769:

  The text used as input to HMAC is the STUN message, up to and
  including the attribute preceding 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 a 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 in the STUN header must be adjusted to point to the end
  of the MESSAGE-INTEGRITY attribute prior to calculating the HMAC over
  the STUN message, up to and including the attribute preceding the
  MESSAGE-INTEGRITY attribute.  Such adjustment is necessary when
  attributes, such as FINGERPRINT and MESSAGE-INTEGRITY-SHA256, appear
  after MESSAGE-INTEGRITY.  See also [RFC5769] for examples of such
  calculations.

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

As explained above the content of the MESSAGE-INTEGRITY is not used when computing the hash, so the content itself is irrelevant.

> 
> 14.6.  MESSAGE-INTEGRITY-SHA256
> 
> This section has the same problems regarding the HMAC calculation as
> section 14.5.

I removed the second paragraph altogether and rewrote the last paragraph to mirror the new text in MESSAGE-INTEGRITY:

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

  Once the computation is performed, the value of the MESSAGE-
  INTEGRITY-SHA256 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-
  SHA256, the length field in the STUN header must be adjusted to point
  to the end of the MESSAGE-INTEGRITY-SHA256 attribute prior to
  calculating the HMAC over the STUN message, up to and including the
  attribute preceding the MESSAGE-INTEGRITY-SHA256 attribute.  Such
  adjustment is necessary when attributes, such as FINGERPRINT, appear
  after MESSAGE-INTEGRITY-SHA256.  See also Appendix B.1 for examples
  of such calculations.

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

The text now reads:

  The MESSAGE-INTEGRITY-SHA256 attribute contains an HMAC-SHA256
  [RFC2104] of the STUN message.  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 HMAC MUST NOT
  be truncated below a minimum size of 16 bytes.  The value must be the
  full 32 bytes unless the STUN Usage explicitly specifies that
  truncation is allowed.  STUN 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.

That is how it works in RFC 5389, so we should not change it.  Note that the fact that the field is not updated is implicit with the fact that FINGERPRINT MUST be the last one (second paragraph)
and that the fourth paragraph states that "[the length field in the STUN header] must be correct and include the CRC attribute as part of the message length, prior to computation of the CRC."

I updated the whole section to take in account MESSAGE-INTEGRITY-SHA256.

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

Applied.

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

Right.  New text:

  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 binary encoding of the error code
  modulo 100, and its value MUST be between 0 and 99.

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

Applied.

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

Applied.

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

Applied.

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

Applied.

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

Applied.

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

This text did not change since RFC 5389 so I am reluctant to update it.

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

See above.

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

Applied.

> 
> 17.3.2.  New Attributes
> 
>    0xXXXX: PASSSORD-ALGORITHMS
> 
> s/PASSSORD/PASSWORD/

Applied.

> 
> 17.5.  Password Algorithm Registry
> 
> I think you want to title this registry "Stun password algorithm registry".

Applied, with "STUN" instead of "Stun".

> 
> 17.6.  STUN UDP and TCP Port Numbers
> 
> This section should state that it is updating "Service Name and
> Transport Protocol Port Number Registry".

Applied.

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

Done.

> 
> [END]
> 
> 

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