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 <wor...@ariadne.com> 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] > > >
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art