Subject: Some comments on draft-ietf-tsvwg-sctp-strrst-05.txt

From: Gorry Fairhurst <[email-masked]>
Date: Wed, 08 Sep 2010 10:06:01 +0100

I've read draft-ietf-tsvwg-sctp-strrst-05.txt, since this document has 
been suggested as nearing completion. I have some comments: In this 
version of the draft, I was unable to  check the details of the 
specification, because I didn't find the current language sufficeintly 
tight.

This is therefore a non-chair review, intended to help preparation of 
the next revision of this document. I'd expect the WG to comment on the 
protocol details. I also plan to review the next version.

Gorry (not wearing a chair hat this time!)

----

The document speaks of/maximum segment lifetime/ - is this the IP-level 
MSL or something more specific?
(MSL is a rather imprecise concept in the current IETF).

---

Introduction needs to be clearer - please check English and that this is 
clear about what the document describes and how this may be used.

---
Section 3 (NiT)
- This section should assume IANA makes the assignment and therefore 
simply say
"the chunk type assigned by IANA for a STREAM RESET"

or "the value XXTBDXX"
- with a note to IANA to replace the string with the assigned type.
---

sect 3, point 8 - is the error value unambiguous? can you say what the 
code is, or refer to the section number in an RFC that defines 
appropriate values?

---
Section 4.1

   Stream Reset Request Sequence Number: 4 bytes (unsigned integer)
/It is increased by 1./
- when? Is this once for each request? - please specify.

Stream Number N: 2 bytes (unsigned integer)
/     This optional field, if included, is used to indicates specific
        streams that are to be reset./
- Can this field be present several times in the same request?
- How does the receiver know this?
---
Section 4.2

- Same two questions as in section 4.1 above.
---
Section 4.2

   Stream Reset Request Sequence Number: 4 bytes (unsigned integer)
/It is increased by 1./
- when? Is this once for each request?
---
Section 4.5
/This field holds the length of the parameter, a fixed value of 12
        MUST be found in this field./
Suggest:
/This field holds the length of the parameter, this
   MUST be a fixed value of 12./

When is Stream Reset Request Sequence Number increased (as before).

/Number of new streams: 2 bytes (unsigned integer)  This value holds
        the number of additional outgoing streams the sender would like
        added to the association./
- /would like added/? or /requests to be added/?

/   This parameter can appear in a STREAM RESET chunk.  This parameter/
                     ^^^
- is this /MAY/?
---
Section 5.1.1 para 2

If the chunk type of the STREAM_RESET chunk
     does NOT appear in the
          ^^^
- This does not seem to be a correct use of a keyword, could you please
check?

/with an appropriate error indicating the peer SCTP stack does not
     support the stream reset extension./
- What is the appropriate error code?
---
Section 5.1.2

/The following steps MUST be followed:/
- This does not seem an appropriate use of MUST.

/it is unknown as to if/
- it is not known whether

/will accept or deny it/
- is the word /deny/ or /reject/ or /not accept/ ?

/A3:  If this Outgoing SSN Reset Request Parameter is sent in response
          to an Incoming SSN Request Parameter, the Stream Numbers between
          the Outgoing and the Incoming parameters MUST match.  /

- I don't understand the MUST.
- What happens if they do not match? I suspect the wording needs to be
changed to say, if this xxx is received, the receiver MUST , otherwise
it MUST do yyy.

A.5
/no Stream Numbers MUST be
          put into the Outgoing SSN Reset Request Parameter/
- This does not seem a requirement,please use lower case /must/.

/If the
          sender wants only some outgoing streams to be reset these Stream
          Numbers MUST be filled in the Outgoing SSN Reset Request
          Parameter./
- perhaps should be something like:
/If the
          sender requests only some outgoing streams to be reset the
          set of Stream are identified in the Outgoing SSN Reset Request
          Parameter./

/ It MAY be put together with an Incoming SSN Reset
          Request Parameter or an Stream Reset Response Parameter and MUST
          NOT be put together with any other parameter./
- I am not sure exactly what this means.

- Please check section 5.1.3, and 5.1.4, for similar cases.

-----

/  There SHOULD NOT be sent more than one SSN/TSN Reset Request within a
     maximum segment lifetime./
- where is the maximum segment lifetime defined, if this is the IP MSL,
then I suggest an explicit time should be declared. This could be the
MSL value, is this seems appropriate.

----
5.1.5
/   D2:  The result of the processing of the incoming request is filled/
- I'm not sure what this means, is it the resulting value, or return 
code or what?
---
/ D3:  If the incoming request is a SSN/TSN reset requests, /
                                                          ^
- delete /s/
---
/is filled with the next TSN the sender
         of this Stream Reset Response Parameter will assign/
- perhaps clearer as /the next TSN value to be assigned by the sender of 
this Stream Reset Response Parameter/
---
/the Sender's next TSN field is not filled./
- SHOULD be zero? - or unchanged or what?
---
D4
- This procedure seems under-specified.
/for this is the highest TSN it has seen plus some delta. /
- I get the idea, but this is not much help to an implementor, can
we use some SHOULD and MUSTs to guide this?
- How big is delta - or how do you choose?
---
5.1.6
/it is able to send to/
suggest:
/to which it is able to send/
---
/ Note that new streams are added adjacent to
    the previous steams with no gaps/
- no gaps? - in what space, please clarify - does this mean streams are 
numbered consecutively?
---
/   Any new stream MUST number its first message to be stream sequence 0./
-/Any/A/
- I'm not sure what this sentence is about, is this the SCTP sender 
action for a new stream, what is being numbered?
---
5.2.1
/   Upon reception of a Stream Reset Chunk each parameter within it
    should be processed.  If some parameters have to be sent back, they
    MUST all be put into one STREAM RESET chunk./
- The language here needs to be a protocol spec, not a narrative. Please 
use some SHOULD and MUSTs to specify this.
---
/the same STREAM RESET chunk has to be sent back in response as
    earlier./
- The language here needs to be a protocol spec, not a narrative. Please 
use some SHOULD and MUSTs to specify this.
---
/   formed.  If for whatever reason the endpoint does NOT wish to process/
                                                       ^^^
- I think this should be lower case, it does not seem like a protocol 
action.
---
5.2.2.  Receiver Side Procedures for the Outgoing SSN Reset Request
         Parameter

    In the case that the endpoint is willing to perform a stream reset
    the following steps SHOULD be followed:
                        ^^^^^^^
- This SHOULD conflicts with /MUST/ later in the same section. Either 
the actions are enquired or recommended, please refine use of RFC 2119 
keywords in this section.

- This also applies 5.2.3, and 5.2.4!
- 5.2.6 has the opposite problem, it requires a recommended procedure.
- It may be best to omit the keyword in the introduction and ensure each 
protocol action is clearly called-out as recommended, optional, or 
required using MUST, SHOULD or MAY.
---
E1: /NOT/not/
---
E2:
/If the Stream Reset Timer is running for the Stream Reset
         Request Sequence Number indicated in the Stream Reset Response
         Sequence Number field, mark the Stream Reset Request Sequence
         Number as acknowledged.  If all Stream Reset Request Sequence
         Numbers the Stream Reset Timer is running for are acknowledged,
         stop the Stream Reset Timer./
- This and following sections need to be more concrete and use RFC 2119 
language.
---
5.2.5
- what is the action if not successful?
---
5.3
/Various Examples of the Stream Reset procedures/
- omit Various?
----
- please include one sentence explaining the diagram, saying what E-Z 
is, and saying that time progresses in a downwards direction.
---
6
- I'd have changed /SHOULD/should/ - it is not a specific protocol 
requirement. You may even prefer to say /needs to be extended/ ?
Overall:
---
- I have a question on section 6:
- What is the intended status of this API change?

/The suggested value of this field for IANA is/

----
7

Please further consider the risks and countermeasures.

It appears to me there is no ambiguity at the application in the re-use 
of a stream, can you add text to explain why this does not result in 
applications problems by receiving the numbered data twice

I see the point on the abort… but if I understand, this method can also 
be used to create extra state for "rogue" flows, and therefore this is a 
vulnerability that may need to be countered, and is different to 
aborting a connection.

Reseting a stream "breaks" the connection, but can it leave state? How 
do the end points detect this?

etc.

- I'm not suggesting these are show-stoppers, but they could be points 
that may require a robust implementation to take precautions. Does the 
"maximum segment lifetime" mitigate this?

----

8.1,8.2 - /The value MUST be from/IANA shoal assign this value from/
---

References
- Normative reference needed to Chunk Types ID.

- The current text will require a NORMATIVE reference to the API, since 
this is a standards-track document that updates this. This could be 
relaxed if the API were explicitly stated as an "informative" section or 
an informative annexe. Either way though, this looks like it may need 
APPs area review, in the same way that the API will need this - so there 
is a logical dependency at least for getting the reviews completed.

---

The document, I suggest could benefit from a little explanation of WHY 
this is OK for SCTP and would be difficult for TCP. This understanding 
may be clear to the SCTP community, but I do not think it is necessarily 
clear to a TSV reviewer or the IETF in general. I discussed this with 
Randal informally - things like "how do you reset a stream sequence 
number during and avoid problems with older mis-sequenced in-flight 
segments?" (i.e. explain how this works in SCTP); "how do you avoid 
blind replay attacks (i.e. again explain how SCTP has a way to protect 
this);

---

---

From: Michael Tüxen <[email-masked]>
Date: Sun, 26 Sep 2010 00:26:28 +0200

Hi Gorry,

thank you very much for the comments. See my comments in-line.

I tried to address your points. To simplify the discussion
I have submitted a version which includes the changes based
on the comments from you and Tom.

Most likely you still have some comments (I might have not
addressed all of them appropriately), but I wanted to have
a 'known base' for future discussion. That is why I submitted
the new rev. It is not, because I do not want to do more changes.

Please go through my comments based on the new rev and let me
know what still need to be addressed.

Best regards
Michael

On Sep 8, 2010, at 11:06 AM, Gorry Fairhurst wrote:

> 
> I've read draft-ietf-tsvwg-sctp-strrst-05.txt, since this document has been suggested as nearing completion. I have some comments: In this version of the draft, I was unable to  check the details of the specification, because I didn't find the current language sufficeintly tight.
> 
> This is therefore a non-chair review, intended to help preparation of the next revision of this document. I'd expect the WG to comment on the protocol details. I also plan to review the next version.
> 
> The document speaks of/maximum segment lifetime/ - is this the IP-level MSL or something more specific?
Yes, it is the IP level MSL. That is why we used that term.
> 
> ---
> 
> Introduction needs to be clearer - please check English and that this is clear about what the document describes and how this may be used.
I've taken the suggested modifications from Tom. I hope this make the text clear enough.
If not, please let me know.
> 
> ---
> Section 3 (NiT)
> - This section should assume IANA makes the assignment and therefore simply say
Hmm. We really want IANA to use this value. It is used in FreeBSD, Wireshark, INET
and possibly in other implementations. All IANA related text is written such that
concrete numbers are suggested. We've done this earlier and had the same comments.
The problem is that we can;t get the numbers assigned before the RFC comes out
but we have (at least most of the time) released code before the RFC comes out.

So I would suggest to leave it as it is. If the WG thinks this is not acceptable,
we can take out the concrete numbers, wait for IANA making its choices and will
have interoperability problems...
> ---
> 
> sect 3, point 8 - is the error value unambiguous? can you say what the code is, or refer to the section number in an RFC that defines appropriate values?
I make the text more explicit:
<t>If a sender transmits an unsupported combination, the receiver SHOULD
send an ERROR chunk with a Protocol Violation cause as defined in
section 3.3.10.13 of <xref target="RFC4960"/>).</t>
> - when? Is this once for each request? - please specify.
I changed it to
"It is increased by 1 whenever sending a new Stream Reset Request parameter."
This is explained in detail in the procedures section.
> - Can this field be present several times in the same request?
I have replace "Stream Number N" by "Stream Number 1..N" to make
clear (a in the figure) that you can have N Stream Numbers.
> - How does the receiver know this?
From the Parameter Length... I changed that text to
This field holds the length in bytes of the parameter;
the value MUST be 16 + 2 * N.
> - Same two questions as in section 4.1 above.
Same answer. Only the length is 8 + 2 * N.
> - when? Is this once for each request?
Same as above.
>  MUST be a fixed value of 12./
Based on suggested text by Tom it reads now:
This field holds the length in bytes of the parameter; the value MUST be 12.
OK?
> When is Stream Reset Request Sequence Number increased (as before).
Same answer as before...
> - /would like added/? or /requests to be added/?
I took "requests to be added"...
> /   This parameter can appear in a STREAM RESET chunk.  This parameter/
Applied.
> - This does not seem to be a correct use of a keyword, could you please
It is just a 'not'...
> - What is the appropriate error code?
The text says:
... any request by the application for such service SHOULD
be responded to with an appropriate error...
Therefore this is an API thing and the "appropriate error code"
is API dependent. So I think we can not be more specific.
> - This does not seem an appropriate use of MUST.
Why not? What are you suggesting to use instead.
We use similar wording for example in RFC 4960, page 76,
first sentence on that page. This does not mean that
the wording is used appropriately...
> - it is not known whether
Already reported by Tom.
> - is the word /deny/ or /reject/ or /not accept/ ?
Reconsidering this: I think we use deny almost everywhere. I'll replace some
occurrences of reject by deny, also undoing a change suggested by Tom.

If you think deny is wrong, we can change that (in all places), but this
is exposed in the API. So we would need these constants, too.
> changed to say, if this xxx is received, the receiver MUST , otherwise
A3 and A5 are somewhat duplications of each other. I have removed A3.
> - This does not seem a requirement,please use lower case /must/.
No, it is the way to say All streams. 
>         set of Stream are identified in the Outgoing SSN Reset Request
The text reads after Toms suggestions:
If the sender requests only some outgoing streams to be reset these Stream 
Numbers MUST be placed in the Outgoing SSN Reset Request Parameter.
OK?
> - I am not sure exactly what this means.
These are the limitations of putting two parameters in the chunk.

> MSL value, is this seems appropriate.
What is the explicit MSL value? We actually mean the IP MSL. I have
added some clarification.
> - I'm not sure what this means, is it the resulting value, or return code or what?
It is one of the values from table 3.
> / D3:  If the incoming request is a SSN/TSN reset requests, /
Done.
> - perhaps clearer as /the next TSN value to be assigned by the sender of this Stream Reset Response Parameter/
Done.
> - SHOULD be zero? - or unchanged or what?
It is an optional field. So you do not fill it, it is not there. That is why it is optional.
> - How big is delta - or how do you choose?
Thanks for catching this. I added:
The value should be the smallest TSN not acknowledged by the receiver
of the request plus 2^31.
> /to which it is able to send/
Done.
> - no gaps? - in what space, please clarify - does this mean streams are numbered consecutively?
The next sentence gives an example. Streams are identified by stream identifiers starting from
0 to N-1, where N is the number of streams. There cannot be gaps.
> - I'm not sure what this sentence is about, is this the SCTP sender action for a new stream, what is being numbered?
Each ordered message in a stream has a stream sequence number (SSN). The meaning of the sentence
is that the SSN of the first ordered message is 0.
I changed it to:
A new stream MUST use for its first ordered message the stream sequence number 0.
> - The language here needs to be a protocol spec, not a narrative. Please use some SHOULD and MUSTs to specify this.
Done.
> - The language here needs to be a protocol spec, not a narrative. Please use some SHOULD and MUSTs to specify this.
Done.
> - I think this should be lower case, it does not seem like a protocol action.
Correct.
>   the following steps SHOULD be followed:
changed to MUST.
> - This SHOULD conflicts with /MUST/ later in the same section. Either the actions are enquired or recommended, please refine use of RFC 2119 keywords in this section.
> 
Solve similar.
> - 5.2.6 has the opposite problem, it requires a recommended procedure.
I changed the API stuff to lowercase wording.
> - It may be best to omit the keyword in the introduction and ensure each protocol action is clearly called-out as recommended, optional, or required using MUST, SHOULD or MAY.
I hope I made it at least consistent.
> E1: /NOT/not/
Done.
> - This and following sections need to be more concrete and use RFC 2119 language.
Fixed.
> - what is the action if not successful?
... then he does not change anything.
> /Various Examples of the Stream Reset procedures/
Done.
> ----
> - please include one sentence explaining the diagram, saying what E-Z is, and saying that time progresses in a downwards direction.
Added:
<t>The following message flows between an Endpoint A and an
Endpoint Z illustrate the described procedures. The time
progresses in downward direction.</t>
> ---
> 6
> - I'd have changed /SHOULD/should/ - it is not a specific protocol requirement. You may even prefer to say /needs to be extended/ ?
Yes, "needs to be extended" is what we want.
> - What is the intended status of this API change?
I've added the sentence:
<t>Please note that this section is informational only.</t>
This is what we want, since the Socket API is also informational. Therefore the
reference to the Socket API ID can continue to be an informational one, right?
> /The suggested value of this field for IANA is/
I do not understand this comment. Section 6 requires no action from IANA and does not
contain this sentence.
> It appears to me there is no ambiguity at the application in the re-use of a stream, can you add text to explain why this does not result in applications problems by receiving the numbered data twice
The only problem I see is that an application is written under the assumption that the SSNs are monotonically
increasing. When they run on top of a stack supporting stream reset and the peer resets a stream they
might not be able to handle it. That is why we have specified in the API that the application must explicitly
allow the processing the stream reset requests. So for an old applications all requests would be denied.
> 
> I see the point on the abort… but if I understand, this method can also be used to create extra state for "rogue" flows, and therefore this is a vulnerability that may need to be countered, and is different to aborting a connection.
The point is that an on path attacker can always do anything. An off-path attacker has to guess
the verification tag. This does not depend on the use of the extension described in this ID.
> 
> Reseting a stream "breaks" the connection, but can it leave state? How do the end points detect this?
It does not leave state, applications can subscribe to corresponding notifications. But this is an API issue.
> 
> etc.
> 
> - I'm not suggesting these are show-stoppers, but they could be points that may require a robust implementation to take precautions. Does the "maximum segment lifetime" mitigate this?
The MSL is to avoid multiple TSNs wrap arounds within an MSL...
> 8.1,8.2 - /The value MUST be from/IANA shoal assign this value from/
Applied. Do you really mean 'shoal' or is it a typo?
> - Normative reference needed to Chunk Types ID.
Good point. Done.
> 
> - The current text will require a NORMATIVE reference to the API, since this is a standards-track document that updates this. This could be relaxed if the API were explicitly stated as an "informative" section or an informative annexe. Either way though, this looks like it may need APPs area review, in the same way that the API will need this - so there is a logical dependency at least for getting the reviews completed.
Hopefully addressed above. 
> 
> ---
> 
> The document, I suggest could benefit from a little explanation of WHY this is OK for SCTP and would be difficult for TCP. This understanding may be clear to the SCTP community, but I do not think it is necessarily clear to a TSV reviewer or the IETF in general. I discussed this with Randal informally - things like "how do you reset a stream sequence number during and avoid problems with older mis-sequenced in-flight segments?" (i.e. explain how this works in SCTP); "how do you avoid blind replay attacks (i.e. again explain how SCTP has a way to protect this);
Hmm, not sure what you want here. TCP has no streams, so it does not have stream sequence numbers exposed
to the application. TCP numbers bytes, but the sequence numbers are not exposed. So I'm not sure how 
procedures defined in this ID could be 'translated to TCP'.

SCTP uses verification tags to protect against bind attacks. This has nothing to do with using
the extension defined in this ID or not.

So if you could describe more detailed what kind of explanation you want (and where), I can write up
something.

---

From: Gorry Fairhurst <[email-masked]>
Date: Sun, 26 Sep 2010 08:28:29 +0100

Thanks for the new revision.

I have comments in-line on the responses where you asked
for a few clarifications, which I have now tried to provide below.

Best wishes,

Gorry

> On Sep 8, 2010, at 11:06 AM, Gorry Fairhurst wrote:
Good plan.

<snip>
> The problem is that we can;t get the numbers assigned before the RFC comes out
I think we can get numbers as soon as we have consensus to publish, that
could during the WG process,  i.e. I'd especially support getting 
numbers as
soon as we are ready to do a WGLC.
> have interoperability problems...
I think the draft could simply say these are tentative assignments to be 
confirmed
by IANA when published, in a RFC-ed note:-)
> section 3.3.10.13 of<xref target="RFC4960"/>).</t>
> <snip>
>> ---
>> Section 4.5
OK
<snip>
> the wording is used appropriately...
I'd need to read, in general I prefer to use MUST when it
refers to some action, or some format that must be used. So,

I'd have preferred something like:

The receiver must do the following;
* It MUST do action A.
* If p or q it MUST do action B.
* A z format response MUST be sent and all option fields MUST be zero.

However, this is style.  I'd be happy to re-read the new version,
and I know you have made many changes. The problem is when
the list of things after the "following:" do not all have an
RFC2119 requirement in them... e.g. E6 - which says something
/is/ put into a field.

<snip>

> is exposed in the API. So we would need these constants, too.
I expect it will be fine, if consistently used.
> No, it is the way to say All streams.
Then is the requirement that the Stream Number MUST be zero (?), 
indicating all streams?
> Numbers MUST be placed in the Outgoing SSN Reset Request Parameter.
OK, the MUST is clear.
> These are the limitations of putting two parameters in the chunk.

- My problem here was a sentence with combinational logic /or/ /and/
that also includes a MAY and a MUST NOT, and refers back to "it".
- Perhaps two sentences would be clearer instead of the /and/.
- Could you also refer to the requirement?
> added some clarification.
IP MSL is an interesting topic in itself... but let's not be diverted 
down this
conversation. My suggestion was state the time you required, and justify
this as being the same as IP MSL.
> It is one of the values from table 3.
- I didn't understand "filled"

<snip>
> It is an optional field. So you do not fill it, it is not there. That is why it is optional.
- Say this rather than "filled".
> I do not understand this comment. Section 6 requires no action from IANA and does not
See above IANA note. Documents should contain something to say this 
number is
to be allocated or *confirmed* by IANA - at least as an RFC -Ed note :-)
> allow the processing the stream reset requests. So for an old applications all requests would be denied.
Sounds good to mention.
> the verification tag. This does not depend on the use of the extension described in this ID.
I agree, let's state this!
> It does not leave state, applications can subscribe to corresponding notifications. But this is an API issue.
Again, good note.
> The MSL is to avoid multiple TSNs wrap arounds within an MSL...
Again, good note.
> Applied. Do you really mean 'shoal' or is it a typo?
It is indeed a mistake by me! /shoal/should/
> So if you could describe more detailed what kind of explanation you want (and where), I can write up
I don't think the SCTP world has a problem with this - but if we get a 
TCP reviewer, I'd like to just see a statement
that SCTP Streams are uniquely defined, and that the TSNs give 
protection. The above discussions also help to
identify what is already a part of SCTP and where there could be issues.