Ballot for draft-ietf-tram-stunbis
Yes
No Objection
No Record
Note: This ballot was opened for revision 16 and is now closed.
Balloting NoObj, trusting AD.
1) Maybe it would make sense to refer informationally to draft-ietf-tram-stun-pmtud in section 6.1? 2) sec 6.2.2: This sentence seems to assume that only one request is sent per TCP connection/each request sends out after a new SYN: "However, for a request/response transaction, if the client has not received a response by Ti seconds after it sent the SYN to establish the connection, it considers the transaction to have timed out.“ i don’t think that assumption would be correct. Maybe rephrase this sentence…? 3) Also section 6.2.2: Should the client send keep-alives if a connection is hold open but currently not used? If not, I guess further recommendation is needed to address the case where a transmission fails because an existing idle TCP connection was used that doesn’t work anymore. In this case, I'd say the recommendation would be to send a RST and try to open a new TCP connection. 4) Should section 9.2 maybe provide further guidance on the lifetime of the (server-selected) nonce? 5) I'm sure, I just missed something but how does a server know if a first request intends to use the Long-Term Credential Mechanism or not (see 9.2.3.1. and 9.2.4 I guess). Or is this pre-configured? 6) Section 6.3 says that this doc only specifies the procedures for the new spec in this document and subsequently says: "It checks [...] that the magic cookie field has the correct value..." However, given this spec obsoletes RFC5389, I think that section 11 should provide more guidance on how to handle "old" STUN messages. Or is the intention that an upgraded STUN server does not handle old requests anymore? If so that should be spelled out as well. 7) sec 14.5: "The value of the MESSAGE-INTEGRITY attribute is set to a dummy value." Should the dummy value be further specified? Also it looks like there was a compile problem on page 39. Is there text missing? 8) sec 17.6: Isn't "stuns 5349/upd" used for DTLS? If so, it should be registered!
[Thanks for addressing my DISCUSS -- the original text of my DISCUSS follows] Thanks to everyone who worked on this revision of the STUN protocol. Thanks in particular to the ARTART reviewer and to the authors for actively engaging on the points he raised. I have one concern about interoperability and another about the IANA changes that I believe require changes to the document prior to publication. §14.2: > X-Port is computed by taking the mapped port in host byte order, > XOR'ing it with the most significant 16 bits of the magic cookie, and > then the converting the result to network byte order. If the IP > address family is IPv4, X-Address is computed by taking the mapped IP > address in host byte order, XOR'ing it with the magic cookie, and > converting the result to network byte order. If the IP address > family is IPv6, X-Address is computed by taking the mapped IP address > in host byte order, XOR'ing it with the concatenation of the magic > cookie and the 96-bit transaction ID, and converting the result to > network byte order. The discussion of performing operations "in host byte order" is very confusing, and seems likely to cause issues communicating between machines of different endianness. As an implementor, based on this description, I cannot tell whether, given a port of 0x1234 (and operating on a little-endian machine), I'm supposed to do: Port (host order): 34 12 Magic Cookie Prefix: 21 12 Result (host order): 15 00 X-Port (net order): 00 15 or: Port (host order): 34 12 Magic Cookie Prefix: 12 21 Result (host order): 26 33 X-Port (net order): 33 26 One of these is clearly wrong. I think it's the first one, but I *also* think that the first one is the most straightforward interpretation of the quoted paragraph. The following would seem to be a complete description of the operation without introducing possible confusion about the difference between host and network order: X-Port is computed by XOR'ing the mapped port with the most significant 16 bits of the magic cookie. If the IP address family is IPv4, X-Address is computed XOR'ing the mapped IP with the magic cookie. If the IP address family is IPv6, X-Address is computed by XOR'ing the mapped IP address with the concatenation of the magic cookie and the 96-bit transaction ID. In all cases, the XOR operation works on its inputs in network byte order (that is, the order they will be encoded in the message). This makes it clear that the proper operation is: Port: 12 34 Magic Cookie Prefix: 21 12 Result / X-Port: 33 26 --------------------------------------------------------------------------- §17.3.1: > IANA is requested to update the names for attributes 0x0002, 0x0003, > 0x0004, 0x0005, 0x0007, and 0x000B, and the reference from RFC 5389 > to RFC-to-be for the following STUN methods: ... > 0x0003: (Reserved; prior to [RFC5389] this was CHANGE-REQUEST) The attribute 0x0003 is registered by RFC 5780, and should not be removed by this document. --------------------------------------------------------------------------- §3: This is almost, but not quite, the boilerplate defined by RFC 8174. Please update to match the text in RFC 8174. --------------------------------------------------------------------------- §5: > server to detect if the client will understand certain attributes > that were added in this revised specification. This is a *bit* misleading, as these attributes were actually added in RFC 5389. Consider: "...that were added to STUN by [RFC5389]." --------------------------------------------------------------------------- §6.1: > If the agent is sending a request, it SHOULD add a SOFTWARE attribute > to the request. I believe this would benefit from a pointer to the security section; e.g.: "Note that the inclusion of a SOFTWARE attribute may have security implications; see Section 15.1.2 for details." --------------------------------------------------------------------------- §6.2: > STUN may be used > with anycast addresses, but only with UDP and in usages where > authentication is not used. This bit of operational advice seems out of place in the middle of an implementation discussion, and is quite likely to be missed by its intended audience. Consider relocating it to an "Operational Considerations" section. --------------------------------------------------------------------------- §6.2.1: > As with TCP, the usage of Karn's > algorithm is RECOMMENDED [KARN87]. This normative language means that [KARN87] needs to be a normative reference. --------------------------------------------------------------------------- §6.2.3 says: > Alternatively, a > client MAY be configured with a set of IP addresses that are trusted; > if a certificate is received that identifies one of those IP > addresses, the client considers the identity of the server to be > verified. Presumably, this means the server supplies a certificate with SubjectAltName containing an iPAddress? Please add text to clarify whether that's the intention. If that *is* the intended meaning, then this behavior in §8.1 seems unnecessarily restrictive: > A "stuns" URI > containing an IP address MUST be rejected, unless the domain name is > provided by the same mechanism that provided the STUN URI, and that > domain name can be passed to the verification code. Presumably, this is done because certs with iPAddress-form SubjectAltNames are pretty rare (although CAB Forum baseline requirements do explicitly allow their issuance) -- but if the text in §6.2.3 is based on allowing the use of such certs in a TURN deployment, then it seems that URI resolution should be also. --------------------------------------------------------------------------- §9.1.3: > If the MESSAGE- > INTEGRITY-SHA256 attribute is not present, and using the same > password, compute the value for the message integrity as described > in Section 14.5. I can't figure out what is meant by "and using the same password" here. The structure of the sentence would imply that the subject of "using" is "attribute" (the one that is not present), but that doesn't make sense. Is it supposed to be something like this? If the MESSAGE-INTEGRITY-SHA256 attribute is not present, compute the value for the message integrity as described in Section 14.5, using the same password as would have been used for MESSAGE-INTEGRITY-SHA256. --------------------------------------------------------------------------- §9.1.5: > A client sending subsequent requests to the same server MUST send > only the MESSAGE-INTEGRITY-SHA256 or the MESSAGE-INTEGRITY attribute > that matches the attribute that was received in the response to the > initial request. How long does it continue to do so? It seems sensible to do this for the length of validity of the credentials (and no longer) -- and probably even scoped to the credentials -- but the document really should call that out explicitly. --------------------------------------------------------------------------- §9.2.2: > The structure of the key when used with long-term credentials > facilitates deployment in systems that also utilize SIP [RFC3261]. > Typically, SIP systems utilizing SIP's digest authentication > mechanism do not actually store the password in the database. > Rather, they store a value called H(A1), which is equal to the key > defined above. Suggest adding something like: "For example, this mechanism can be used with the authentication extensions defined in [RFC5090]." --------------------------------------------------------------------------- §9.2.3.2: > Once a request/response transaction has completed successfully, the The use of "successfully" here is kind of confusing, since the transaction in question was completed with an Error Response. Suggest dropping "successfully". --------------------------------------------------------------------------- §9.2.4: > o If the NONCE is no longer valid, the server MUST generate an error > response with an error code of 438 (Stale Nonce). This response > MUST include NONCE, REALM, and PASSWORD-ALGORITHMS attributes and > SHOULD NOT include the USERNAME, USERHASH attribute, The NONCE > attribute value MUST be valid. Nit: The comma before the final "The" should be a period. --------------------------------------------------------------------------- §9.2.4: > USERHASH attribute. The response MAY include a MESSAGE-INTEGRITY > or MESSAGE-INTEGRITY-SHA256 attribute, using the previous password > to calculate it. I think this means to say "using the previous key," right? --------------------------------------------------------------------------- §14: > The > padding bits are ignored, and may be any value. This seems to be encouraging implementors to use uninitialized memory for these bits, which could be a security issue. It would be far safer to do the more traditional "must be set to zero on sending, must be ignored by receiver" thing here. --------------------------------------------------------------------------- §14.5: > Petit-Huguenin, et al. Expires September 6, 2018 [Page 40] > > Internet-Draft Session Traversal Utilities for NAT (STUN) March 2018 > Similarly, when validating the MESSAGE-INTEGRITY, the length field in This appears to be an errant header/footer pair. Note that the correctly-formatted footer for page 40 appears several paragraphs later. --------------------------------------------------------------------------- §14.7: > The 32-bit CRC is the one defined in ITU V.42 [ITU.V42.2002], which > has a generator polynomial of > x32+x26+x23+x22+x16+x12+x11+x10+x8+x7+x5+x4+x2+x+1. Please correct the polynomial representation: x^32 + x^26 + x^23 + x^22 + x^16 + x^12 + x^11 + x^10 + x^8 + x^7 + x^5 + x^4 + x^2 + x + 1. --------------------------------------------------------------------------- §14.8: > The reason phrase is meant for user consumption This is a very tricky assertion to maintain, unless you want to start working with language tags and reason phrase localization. I think you're far safer saying something like "The reason phrase is meant for diagnostic purposes." I would not encourage implementations to just display its contents to users without any localization discussion. --------------------------------------------------------------------------- §15.2.4: It's probably worth additionally noting that this attack can be trivially launched by the STUN server itself -- so users of STUN servers should have the same level of trust in them as any other node that can insert themselves into the communication flow. --------------------------------------------------------------------------- §15.3: I'm surprised that there is no plan in here for cryptoagility of the USERHASH TLV. While a practical preimage attack on SHA-256 seems pretty implausible today, it would be nice to know that we have a remedy if we ever decide that we need to change things.
Thank you for addressing my DISCUSS points and comments.
Hi, thanks for this work. For the record, I have mainly reviewed the diff. General: I think Peter's ART-ART review has some points worth addressing. §1, last paragraph: This seems out of place. Also, I wonder if it should be stated more strongly, perhaps with a SHOULD or even MUST. §3: RFC 8174 offers it's own recommended boilerplate; is there a reason not to use it? §6.2.1, third paragraph: " The value SHOULD be considered stale and discarded after 10 minutes without any transactions to the same server. " This is a bit ambiguous. I think it means the value is stale if no transactions have occurred in the last 10 minutes, But I think it could be read to say that there should be no transactions to the same server once one considers the value as stale. §9.1.4: Does it make sense to signal that "an attack took place" rather than signaling "integrity protection was violated" and let the human operators decide if that implies an attack? §14.3: "A compliant implementation MUST be able to parse UTF-8 encoded sequence of less than 763." 763 whats? §15.3, 2nd bullet: I'm not sure I understand what is meant by "get an updated external mechanism"
I'm balloting No Objection and not DISCUSS because no individual item rises quite to that level of criticality, but there still are quite a number of things that ought to be addressed prior to publication. In addition to the excellent points raised in the secdir review (which are mostly addressed but only in the editor's copy?), I would call out a few more key observations: If I understand correctly, the server's PASSWORD-ALGORITHMS array from the response are echoed back by the client in the subsequent request in order to provide downgrade protection -- the initial response is at least sometimes not authenticated, and by having the client repeat it back under an authenticated scheme, the server can detect if the list was tampered to remove any new, stronger, algorithms. This is probably an important enough concept to be called out explicitly, either where the behavior is described or in Section 9.2.1 ("Bid Down Attack Prevention"). Section 8.1 has some text that could be read as requiring a STUN client to implement dual-stack IPv4/IPv6, which presumably is not the intention: In addition, instead of querying either the A or the AAAA resource records for a domain name, the client MUST query both and try the requests with all the IP addresses received, as specified in [RFC8305]. There are still some places where the text talks about MESSAGE-INTEGRITY when it really means "message integrity protection" (i.e., either the SHA1 or SHA-256 variants, for now); I've attempted to note them in the notes below. There is text in both Sections 9.1 and 9.2 that talks of replay protection being provided, by the time-limited nature of the short-term credentials or the nonce for long-term credentials. This seems to only, strictly speaking, be true if any given password/nonce can only be used once, but for the short-term credentials the password is used for the duration of the "session-equivalent", and for the long-term credentials a nonce can be reused for some amount of time as well. So while these are both replay countermeasures that can limit replay attacks, it seems a stretch to claim that they prevent replay attacks (entirely). The Stale Nonce behavior seems potentially worrisome, in that it opens up a side channel for a distinguishing attack, between a 401 and 438 response. (That is, "password correct" vs. "password incorrect".) The impact seems rather muted, though, since the gain to the attacker is to be able to precompute a bunch of requests using a nonce of the attacker's choosing and blindly replay the precomputed object against (possibly multiple) servers looking for a guess. The realm and username are still in play, so the scope for the attacker to gain from the precomputation seems limited. (The same level of brute-force guessing can be obtained "live" just by computing the trial responses against a live server, using a valid nonce.) So, while it might be nice to give guidance that 438 should only be used when the server can validate that it did generate the nonce and the nonce was valid "recently", and to treat other cases as authentication failures, it's not clear to me that there's enough of a benefit from the change to make it worth doing. Section 6.2.3 Maybe just cite BCP 195/RFC 7525 instead of inlining requirements? Also, the "client SHOULD verify ..." and "client MUST verify ..." are similar enough that some clarification of certificate vs. identity verification would be in order, if both statements are believed accurate. Section 6.3 [...] Known-but-unexpected attributes SHOULD be ignored by the agent. [...] Why is this not an error? Section 6.3.1, 6.3.2, etc. Should the second paragraph start with "Otherwise, "? Presumably once an error response is sent or processing ceases, processing stops... Section 8.1 [...] A "stuns" URI containing an IP address MUST be rejected, unless the domain name is provided by the same mechanism that provided the STUN URI, and that domain name can be passed to the verification code. "the verifcation code" is probably unnecessarily vague; we're just talking about the (D)TLS SNI and certifciate verification code, right? Section 9.2 [...[ The nonce provides the replay protection. It is a cookie, selected by the server, and encoded in such a way as to indicate a duration of validity or client identity from which it is valid. Is this like a TLS cookie, where only the server needs to know about the internal structure (viz. "encoded" in the above)? If so, the text could probably be more clear. When mentioning just "a message-integrity", maybe forward-ref both versions of it to be more clear that we don't mean just the old one? It's unclear why indications can't use a nonce, if one is known from a previous request/response on the same connection. When encoding the value of 0 for the security feature, it may be worth clarifying that it is still encoded as a 24-bit integer and base64-encoded, though the "13 character string" text ought to make this clear to the reader. Section 9.2.4 * 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). This Note seems a bit odd, since we're predicated on PASSWORD-ALGORITHMS being absent. * 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). Maybe we could be more clear about "the value of the PASSWORD-ALGORITHMS attribute in the message matches the value of the PASSWORD-ALGORITHMS attribute sent in the response that sent the NONCE used with this message"? Section 9.2.5 If the response contains a PASSWORD-ALGORITHMS attribute, the subsequent request MUST be authenticated using MESSAGE-INTEGRITY- SHA256 only. This means "all subsequent requests to this server"? Maybe the wording could be more clear. (It also means that if we get a new scheme when SHA256 is broken, the document introducing it may need to Update this document to remove the MUST.) Also, I think the Security Feature bit is "Password algorithms" plural (in the next paragraph and maybe multiple locations in the document). Section 10 [...] If the transaction uses TLS or DTLS and if the transaction is authenticated by a MESSAGE-INTEGRITY-SHA256 attribute and if the server wants to redirect to a server that uses a different certificate, then it MUST include an ALTERNATE-DOMAIN attribute containing the subjectAltName of that certificate. The (definite article), implying the entire subjectAltName? Or just one name that is contained as a SAN entry? Section 14.8 Need to not just say MESSAGE-INTEGRITY since the SHA256 variant should be fine as well. Section 14.10 Maybe the nonce cookie should get a shout-out here? Section 14.12 Do we need to talk about the padding, since this is the the sole contents of the attribute and the attribute padding suffices? Also, "derive the long-term password" doesn't seem quite right; we're deriving the key from the long-term password, per Section 17.15.1. Section 15.1.2 There is no amplification of the number of packets with this attack (the STUN server sends one packet for each packet sent by the client), though there is a small increase in the amount of data, since STUN responses are typically larger than requests. I usually hear amplification in terms of bytes or bandwidth, not packets. So maybe just "amplification is modest"? Section 15.3 This specification uses both HMAC-SHA1 and HMAC-SHA256 for computation of the message integrity. Could perhaps be reworded, since the number of situations when both are used on the same message is small, and we try to push SHA256 when we can. o STUN Client and Server using the Short Term Credential Mechanism Clients and Servers, plural. Section 17.32. I'm confused why we have MUST-level requirements about using PASSWORD-ALGORITHMS when it is in the comprehension-optional range (e.g., echoing from response to repeated request as in Section 9.2.5). The server cannot actually enforce this MUST when talking to an old client if we want to retain the comprehension-optional semantics, IIUC. Section 17.6 If we're adding support for DTLS-over-udp (per the next section), should we be registering stuns/udp? "password encryption algorithm" is not quite the right statement to make, since we're hashing, not encrypting.
Thank you for addressing my DISCUSS
NO RECORD - ran out of time to review this document in detail.