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]