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 <worley@xxxxxxxxxxx> 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]
