Subject: Re: [bfcpbis] TSVDIR LC review: <draft-ietf-bfcpbis-rfc4582bis-13.txt> (The Binary Floor Control Protocol (BFCP)) to Proposed Standard>

From: "Paul E. Jones" <[email-masked]>
Date: Wed, 23 Sep 2015 06:26:15 +0000

Gorry, et al,

Following up on this email, please see my comments inline regarding 
Gorry Fairhurst's comments.  I don't think these were addressed 
elsewhere; I could not find a reply, in any case.

[ Note: I removed [email-masked].  I don't think we need dialog with the 
entire IETF, but feel free to add the alias back if you disagree. ]

------ Original Message ------
From: "Allison Mankin" <[email-masked]>
To: "IETF Discussion" <[email-masked]>; 
[email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]; "Transport 
Directorate" <[email-masked]>
Sent: 3/2/2015 12:06:13 PM
Subject: [bfcpbis] TSVDIR LC review: 
<draft-ietf-bfcpbis-rfc4582bis-13.txt> (The Binary Floor Control 
Protocol (BFCP)) to Proposed Standard>

>[Put into template form and forwarded for the TSVDIR reviewer, Gorry 
>Fairhurst, to expedite while he is traveling]
>
>Please resolve these comments along with any other Last Call comments 
>you may receive.
>
>Document:  draft-ietf-6man-resilient-rs-04.txt
>Reviewer: Gorry Fairhurst
>Review Date: 2015-03-01
>IETF LC End Date: 2015-03-05
>IESG Telechat date: ---
>
>Summary: This draft is not ready for publication as a standards track 
>RFC.
>
>
>This document defines a control protocol that may be used over TCP or 
>UDP.
>The major change between this document and its predecessor RFC4528 is 
>stated to be:
>
>Main purpose of this work was to revise the specification to support 
>BFCP over an unreliable transport (Section 16.1)
>
>
>The use of UDP raises a number of issues that need to be considered 
>when
>used in environments that may experience congestion, long delay, or
>limited capacity. Further work appears to be needed to address these
>concerns. Citing RFC 5405 as normative (rather than informational) may 
>be
>good, and some of the guidance from this BCP suggests protocol issues 
>that
>should be examined and justified or amended. Notable concerns are the 
>lack of
>support for RTT estimation, and the way in which fragmentation is 
>handled.

There is a fair bit of treatment in the draft on congestion control, 
noting that the protocol uses exponential back-off to avoid sending 
messages too frequently and that there is never more than one 
transaction on the wire at a time.  RFC 5405 is listed as an informative 
reference and, arguably, even that should be removed, since it's never 
actually referenced; but I agree with you that it should be for the RTT 
issues you raise below.

In general, protocol exceeds the recommendation in 3.1.2/RFC5405 by not 
having more than one transaction ongoing at any time.  Is there specific 
wording you think should be added to the draft?  I think section 6.2.1 
would be the right place for any addition.

>There is mention of potential support for SCTP, however the statement 
>on
>SCTP appears to need updating.
Use of SCTP is outside the scope of this work.  What updating do you 
believe needs to be made?  If B.1.1.6, I'll note that was a statement of 
fact at the time the protocol work was undertaken.

>The intro states:
>"BFCP has been designed so that it can be used in low-
>bandwidth environments" - there are specific low-bandwidth
>concerns when UDP is used. See below.
>
>Section 3.4: Congestion Control
>I do not understand how congestion-control is performed on the media 
>flow.
>Is it the responsibility of each audio stream to independently react to
>congestion? If it is, it would be useful to state this here.
BFCP does not handle media.  It merely sends floor control requests and 
related messages between floor participants, floor chairs, and the floor 
control server.  These are relatively small binary-encoded messages.  
Media transmission, media congestion issues, etc. are outside the scope 
of this text.

>Section 5: Protocol:
>"If a floor control server receives a
>message with an unsupported version field value, and the extensions
>in this document is supported, the receiving server MUST send an
>Error message with parameter value 12 (Unsupported Version) to
>indicate this. "
>- What happens in the case where the receiving server does not support 
>the
>extensions in this document? Does it silently discard?

Note to editor: "extensions ... is supported" should be "extensions ... 
are supported"

Any server compliant with this draft will support these extensions, so 
perhaps we do not even need to have this bit in the text.  I would 
suggest we say, "If a floor control server receives a message with an 
unsupported version field value, the server MUST indicate it does not 
support the protocol version by sending an Error message with parameter 
value 12 (Unsupported Version)."

>Section 5:
>
>What is the security model when TLS/DTLS is not used? - has the 
>protocol
>protection from off-path attacks, and how is this provided?

There is no security model, except TLS or DTLS.

So long as we use DTLS, off-path attacks are not an issue (I think), 
except for ICMP attacks.  I've noted that as something we need to 
address.

>What is the privacy model when TLS/DTLS is not used?  - Does the 
>protocol
>expose the identity of participants to others on the network?

There is no privacy if plain TCP is used and, yes, user identifiable 
information can be exposed if a secure transport is not employed.   For 
this reason, the security considerations section notes that TLS or DTLS 
are used to provide confidentiality.

>"Res: At this point, the 3 bits"
>- This may be better written as perhaps: Res: In version 1 and 2 of the
>protocol, the 3 bits".

Agreed, this could be improved.  In version 1, it was actually 5 bits 
reserved.  So, I would suggest we simply remove "at this point".   If a 
reliable transport is used, the two new bits will be zero and, 
therefore, align with version 1 of the protocol.

>Primitive
>- Please specify here what a receiver should do with an undefined 
>primitive value!

Section 13 states that an Error message would be sent with code == 13 
(Unknown Primitive).

>Payload Length:
>- What happens when using a datagram format if the datagram length 
>(e.g.
>UDP-Length) is less or more than the value specified within the BFCP?

This will result in an Error message with code == 13 (Incorrect Message 
Length)

>Conference ID:
>- Is the Conference ID randomised to provide protection to mitigate the
>chance of off-path data insertion attacks? Note that in section 6.2.4,
>symmetric port numbers (src,dst) are recommended, and hence the a 
>random
>ephemeral port does not offer protection from off-path attacks in the 
>case
>where TLS/DTLS is not used.
There is nothing stated about how the conference IDs are generated.  My 
opinion is that it would be good if these were randomized, but I can 
appreciate that some implementers might sequentially number conferences. 
  I'll take note and confer with co-authors and invite input from other 
on this.

I think use of DTLS mitigates this form of attack.  These issues are 
covered in the security considerations section.

>Fragment Length:
>- What happens if the datagram length (e.g. UDP-Length) is less or more
>than the value specified within the BFCP?
Arguably, we don't even need the fragment length and could just make all 
of those bits reserved.  The UDP packet itself tells us how long the 
fragment is.  And having this length and the "Payload Length" opens up 
the door to have another Heatbleed-type bug.  We'll either remove it or 
add error-handling comments along the lines of what exists for payload 
length.

>Section 6.1: CC:
>"if a TCP
>connection cannot deliver a BFCP message and times out or receives an
>ICMP port unreachable message mid-connection, the TCP connection
>SHOULD be reestablished."
>- I think this needs more specification. How many times will the TCP
>connection be retried in succession? While it seems OK for a session to 
>be
>restarted because of a delivery failure, this seems like a
>congestion-control problem if it is unconditionally resumed with no
>control of the number of attempts, and no requirement to back-off after
>repeated failures.

I noted that this language existed in the original RFC.  Nonetheless, 
your point is valid.  I'll take a note to address this.

>Section 6.2.1: Retransmission interval:
>"The default initial interval MUST be set to 500ms "
>- The protocol appears to fail systematically  when the actual path RTT 
>is
>greater than 500ms. The sender needs not only to expand the timer each
>retransmission, but needs to avoid the possibility that every message 
>is
>duplicated when the path RTT is greater than 500mS. A simple solution 
>is
>for the sender learn the actual RTT and increase the default timer when
>this is greater than 500 mS? This seems like a protocol flaw. See RFC 
>5405
>on the need to back-off timers, based on RTT.

Good point.  This is a good place to insert that reference and I would 
suggest we change the language, also, to say that the default initial 
timer shall not be less than 500ms.  We can then add language to adjust 
it based on learned RTT.

>Section 6.2.2:
>How does the receiving endpoint verify that the ICMP message was from 
>the
>corresponding endpoint, is there any field in the protocol header that
>could be used to reduce the chance of off-path attacks?
I think this is an issue.  My opinion is that we remove this language 
relating to ICMP for UDP.  I'll take a note and confer with the 
coauthors.

>Section 6.2.2:
>This section could usefully also refer to RFC 5405.

Noted.  We need to talk about ICMP handling.

>Section 6.2.3:
>"BFCP entities should consider the MTU size"
>- This isn't capitalised as "SHOULD", was that intended? I'm unclear 
>what
>the intention is here, especially when discussing PMTU-D.

It probably should be SHOULD.

>"and MAY run MTU discovery, such as [20][21][22], for this purpose."
>- Presumably this is "Path" MTU Discovery.
yes

>- The ID does not explain how this is done. Could it be done using 
>dummy
>messages (e.g. padded out with unused options?) or some other method to
>probe the path? The current text simply does not explain how PMTUD  or
>PLPMTUD is implemented, It needs to specify how to generate a "big" 
>packet
>and how to handle retransmission.

Valid point.  I'll take note to see how we should address this.  Would 
you prefer an active PMTU-D method, or use a passive PMTU-D where the 
endpoint adjusts its assumed PMTU value based on received ICMP error 
messages?  Personally, I prefer the latter.  It avoids introducing new 
protocol machinery.

>Congestion Control:
>The protocol appears to say it sends a single datagram per RTT, but
>actually fragmentation may result in multiple fragments per RTT. This
>amplifies the congestion control risk. There appears to be no limit set 
>to
>the number of fragments that may be created, and each are presumably 
>sent
>as a burst - and all retransmitted if there is a single loss. This 
>could
>introduce a significant risk of congestion collapse, if this happens 
>for
>multiple consecutive messages.

While your point about re-sending two or three packets back-to-back is 
correct, it is my opinion that this is insignificant in the face of of a 
few of things:
1) There is only one transaction outstanding at a time
2) If we adjust T1/T2 per your suggestions to consider RTT, then this 
will mitigate the risk
3) Generally, this protocol is used in environments where there are tons 
of audio of video packets, completely dwarfing this sparse traffic. :)  
(This is only half-hearted; I appreciate that this protocol should work 
in environments where there is limited bandwidth or congestion.)

>Timers T1 and T2 are set to a static value by configuration. While the
>values suggested may be reasonable default minimum values, these two
>values should be increased when a path has a larger RTT to avoid the
>protocol replicating all data (see RFC 5405). The current specification
>can increase congestion for long RTT paths, and needs to be updated to
>reflect paths discovered to have a larger RTT.

Agreed.

>B.1.1.6.  SCTP
>"It would be quite straight forward to specify a BFCP binding for SCTP
>[28], and then tunnel SCTP over UDP in the use case described in
>Appendix B.1.  SCTP is gaining some momentum currently.  There is 
>ongoing
>discussion in the RTCWeb WG regarding this approach. However, this
>approach for tunneling over UDP was not mature enough  when considered 
>and
>not even fully specified."
>- it seems this dismisses SCTP support, yet this text appears out of 
>date
>and needs  to be updated to reflect RFC 6951, and possibly
>https://tools.ietf.org/html/draft-ietf-tsvwg-dtls-for-sctp?
This is likely a good change to make, though this Appendix is background 
for the motivation of this work originally.  Nonetheless, I'll take note 
to confer with the co-authors about what changes should be made here.

Paul