Subject: Re: [tsvwg] Benjamin Kaduk's Discuss on draft-ietf-tsvwg-rfc4960-bis-17: (with DISCUSS and COMMENT)

From: [email-masked]
Date: Sun, 16 Jan 2022 15:11:34 +0100

Hi Benjamin,

Martin suggested to submit a new version of the ID with suggested resolutions of
issues you raised. So the comments below describe what I changed when trying to
address the issues you raised.

Please let me know if you want additional changes, don't like my suggestions or
if you need more information regarding the questions you raised (in particular
the ones you raised on your DISCUSS section).

Just to be clear: I'm submitting this to try to progress work on this document,
not because I'm not willing to accept any further changes based on an upcoming
discussion.

Best regards
Michael

> On 26. Dec 2021, at 13:00, [email-masked] wrote:
> 
>> On 16. Dec 2021, at 15:48, Benjamin Kaduk via Datatracker <[email-masked]> wrote:
>> 
>> Benjamin Kaduk has entered the following ballot position for
>> draft-ietf-tsvwg-rfc4960-bis-17: Discuss
>> 
>> When responding, please keep the subject line intact and reply to all
>> email addresses included in the To and CC lines. (Feel free to cut this
>> introductory paragraph, however.)
>> 
>> 
>> Please refer to https://www.ietf.org/blog/handling-iesg-ballot-positions/
>> for more information about how to handle DISCUSS and COMMENT positions.
>> 
>> 
>> The document, along with other ballot positions, can be found here:
>> https://datatracker.ietf.org/doc/draft-ietf-tsvwg-rfc4960-bis/
>> 
>> 
>> 
>> ----------------------------------------------------------------------
>> DISCUSS:
>> ----------------------------------------------------------------------
>> 
>> My apologies for placing a Discuss so close to the telechat.  I believe
>> that both of these topics are comparatively minor and should be easy
>> to resolve, but that it's important for the document to have a clear
>> answer for them.
> No problem. Whatever improves the document is appreciated.
>> 
>> I ask this with nospecific answer in mind that I need to hear -- per my
>> comments on §5.1.3, what are the actual requirements on the
>> (cryptographic) protection of the State Cookie?  I feel like I got
>> different signals from different parts of the document, and it would be
>> good to have consistent messaging throughout.
> There are two things relevant here:
> (1) The sender of the state cookie considers the IP address, it send the
>    state cookie to, belonging to the peer.
> (2) The sender of the state cookie uses the information in it, when
>    receiving the state cookie, to establish the association and wants
>    to be sure that it has not been modified.
> There is no requirement for privacy, since the information in the cookie
> has been on the wire as part of the INIT and/or INIT ACK.
> 
> Does that make sense to you and is consistent with the signals you got?
Do you need more information?
>> 
>> Section 15.5 establishes a registry for payload protocol identifiers,
>> but I am not sure how this registry is supposed to be able to
>> effectively avoid collisions when we do not specify the endianness in
>> which the value is represented on the wire (per §3.3.1).
> Section 3 contains, just before Subsection 3.1:
> All integer fields in an SCTP packet MUST be transmitted in network byte order,
> unless otherwise stated.
> 
> So the PPID is transmitted always in network byte order, since its description
> does not make any other statement. The only statement that is made is
> that any byte order conversions must be done by the application, this is
> not automatically done by the SCTP implementation.
Do you need more information?
>> 
>> 
>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>> 
>> Peeking in the github repo for purposes of preparing an editorial PR, I
>> see that the original RFC 4960 was prepared in nroff -- my thanks to the
>> authors for the thankless work of converting it to XML!
>> 
>> I'm also glad to see the robustness changes made to the procedures for
>> handling INIT (and INIT ACK) chunks with invalid parameters.
>> 
>> I am also sympathetic to Alvaro's point about obsoleting additional
>> documents.
> This document now also obsoletes RFC 4460 and RFC 8540.
>> 
>> I only got a chance to review the diff from RFC 4960 and a small
>> selection of portions of this document, so this review is not as
>> comprehensive as I would typically perform.  That said, I trust that the
>> document has received sufficient review and therefore am comfortable
>> reballoting No Objection once my discuss points are resolved.
>> 
>> I put some editorial suggestions in a PR on github:
>> https://github.com/sctplab/rfc4960bis/pull/79
> Since I guess you want the discussion of these proposed changes in the
> GitHub infrastructure, please see my comments for the pull request
> at Github.
>> 
>> Abstract
>> 
>> I see that Warren already remarked about the length of the Abstract.
>> I have in my bookmarks
>> https://www.rfc-editor.org/policy.html#policy.abstract that indicates
>> "[a]n Abstract of more than 20 lines is generally not acceptable."
> The abstract is based on what is in RFC 4960 and then a first paragraph was
> added to discuss which documents are obsoleted.
> 
> The document you are linking states at the beginning:
> Note: This page has been replaced by the RFC Style Guide. Please see that page for current information.
> Looking at
> https://www.rfc-editor.org/rfc/rfc7322.html#section-4.3
> I don't see this section mentioning the limitation of number of lines.
> 
> I'm also not sure which format should be used for counting lines
> (isn't the .xml format the binding one), and if empty lines count.
> 
> I'm happy to remove the paragraph about the documents being obsoleted,
> tweak the .xml to remove the number of empty lines (when rendered as
> ASCII) or whatever you suggest. Please advise.
Please advise.
>> 
>> Section 3.3.4
>> 
>>  Cumulative TSN Ack: 32 bits (unsigned integer)
>>     The largest TSN, such that all TSNs smaller than or equal to it
>>     have been received and the next one has not been received.
>> 
>> (nit) The "and the next one has not been received" is implied by
>> "largest ... such that", since if the next one had been received, this
>> one would not be the largest such that the condition holds.
>> (This phrasing also seemse to appear in §3.3.8.)  I didn't remove this
>> in my editorial PR because it is plausible that the redundancy is
>> desired for emphasis.
> The text in 3.3.4 and 3.3.8 should be consistent.
> I agree, that the text you cite is redundant, but this redundancy is
> there on purpose as a result of feedback got via WG LC. If you prefer
> not to have the redundancy, please let me know and I will remove the
> "and the next one has not been received" part.
I kept the redundant text. I'll remove it on explicit request from you.
>> 
>> 
>> Section 5.1.3
>> 
>> When would it be permissible to not include a MAC on the State Cookie
>> data?  It seems like we refer to it as essentially mandatory from other
> You need some protection against changes by others. So encryption
> would also be a possibility with some checks. That it why we did not
> specify a MUST here. But we put in the spec how to handle the case
> when using a MAC.
>> parts of the spec, so I was surprised to see it only as a "SHOULD" here.
>> E.g., in §5.1.5 the first step in processing COOKIE ECHO is "compute a
>> MAC [and compare it to the one in the cookie]" -- there's no point in
>> doing that if there's no MAC in the cookie.  Similarly, in §14.1
>> "Parameters Necessary for the SCTP Instance" we list a secret key for
>> computing the MAC.
> Section 14 contains in the first paragraph:
> 
> This section is for illustrative purposes and is not considered to
> be requirements on an implementation...
> 
> So if you think we should make the MAC mandatory using MUST, please
> let me know and I do the change. All SCTP implementations I know of,
> use a MAC and don't encrypt the cookie. But there are implementation
> I don't know (but I'm aware that they exists).
>> 
>>  Since the State Cookie is not encrypted, it MUST NOT contain
>>  information which is not being envisioned to be shared.
>> 
>> What prevents the state cookie from being encrypted (other than
>> performance concerns)?  Perhaps we just need to require that *if* the
>> state cookie is not encrypted, it doesn't contain such information.
> OK. Changed to using "If the State Cookie..."
>> 
>> Section 6.2
>> 
>>  An SCTP receiver MUST NOT generate more than one SACK chunk for every
>>  incoming packet, other than to update the offered window as the
>>  receiving application consumes new data.  When the window opens up,
>>  an SCTP receiver SHOULD send additional SACK chunks to update the
>>  window even if no new data is received.  The receiver MUST avoid
>>  sending a large number of window updates -- in particular, large
>>  bursts of them.  [...]
>> 
>> "a large number" feels somewhat subjective, such that the power of the
>> "MUST"-level requirement is weakened.
> I understand. But I don't want to force any particular implementation
> here. That is why this text is generic. It is hard to specify a number
> or rate...
>> 
>>  If an endpoint receives a DATA chunk with no user data (i.e., the
>>  Length field is set to 16), it MUST send an ABORT chunk with a "No
>>  User Data" error cause.
>> 
>>  An endpoint SHOULD NOT send a DATA chunk with no user data part.
>>  This avoids the need to be able to return a zero-length user message
>>  in the API, especially in the socket API as specified in [RFC6458]
>>  for details.
>> 
>> This is just a COMMENT since the behavior was present in 4960, but why
>> allow sending an empty data chunk when the receiver is required to abort
>> because of it?  This seems like the reverse of the Postel principle.
> I agree.
> I think both should be the same level. If you API does allow handling of
> empty messages, the protocol could do it. So should we change the first
> text to:
> 
>  If an endpoint receives a DATA chunk with no user data (i.e., the
>  Length field is set to 16), it SHOULD send an ABORT chunk with a "No
>  User Data" error cause.
> 
> This improves consistency, still describes what is implemented and
> provides a way for implementations handling empty messages in the API
> (the RECEIVE primitive would allow this).
Revision 18 contains the above text. Please let me know if this is not
appropriate in your view.
>> 
>> Section 6.5
>> 
>>  Every DATA chunk MUST carry a valid stream identifier.  If an
>>  endpoint receives a DATA chunk with an invalid stream identifier, it
>>  SHOULD acknowledge the reception of the DATA chunk following the
>>  normal procedure, immediately send an ERROR chunk with cause set to
>>  "Invalid Stream Identifier" (see Section 3.3.10), and discard the
>>  DATA chunk.  [...]
>> 
>> What other behaviors are permitted (i.e., why is this SHOULD vs MUST)?
> An implementation might decide that sending a SACK, but not being able
> to deliver the user data to the application, is something which triggers
> an ABORT. 
>> 
>>  The Stream Sequence Number in all the outgoing streams MUST start
>>  from 0 when the association is established.  [...]
>> 
>> If we were starting from scratch this would probably be undesirable (a
>> la draft-gont-numeric-ids-sec-considerations), but I guess it's not
>> really changeable now, and we have to rely on the random initial TSN and
>> verifiers to prevent off-path guessing.
> You can't change this without breaking compatibility with existing
> implementations, which is what is not in scope for a -bis document in
> my mind.
> However, you would need to negotiate the initial values which would add
> up to 128 KB to the handshake.
> 
> Looking at
> https://www.rfc-editor.org/rfc/rfc9000.html#section-19.8
> it seems that QUIC does not randomise the initial Offset (i). It
> states "The first byte in the stream has an offset of 0.".
>> 
>> Section 11.2.5
>> 
>> Looking at the diff from RFC 4960, it seems that we no longer claim that
>> there is a "data retrieval id" passed with the communication-lost
>> notification.  I don't have a great sense for whether this makes sense,
>> e.g., due to an expectation that SEND-FAILURE events will be generated
>> separately for all affected messages.
> This was a mistake in the specification. The data retrieval id is used
> by RECEIVE_UNSENT and RECEIVE_UNACKED and is an identifier for a
> user message. If the communication is lost, there can be multiple messages
> to be retrieved. That is why you can't provide a single data retrieval id
> in a communication lost notification. If the communication is lost, the
> upper layer gets a single communication lost notification and a send failure
> notification for each message. That way the mechanism can work.
> 
> This mistake was not detected earlier, since this is a conceptual API.
> Most kernel implementations use the Socket API specified in RFC 6458,
> which does it a bit differently (due to Socket API constraints).
> 20 years ago we did a userland stack with an API based on the one given
> in RFC 2960, but we did not implement the data retrieval ID part.
> So we did not catch it...
>> 
>> Section 12
>> 
>> My apologies if I failed to find some relevant content in my
>> somewhat-piecemeal review, but I note that we require each HEARTBEAT
>> chunk to receive a corresponding HEARTBEAT ACK, so that (according to
>> the protocol rules) an endpoint can force the peer to do work.  Since
>> the heartbeat is expected to only occur once every RTO or so (or less
>> often), should we allow an endpoint to abort the association on receipt
>> of too many heartbeats?
> I would prefer not to specify anything. The sending of the HEARTBEAT
> can also be triggered by the application. And the network could also
> buffer several HEARTBEATs that were sent using an appropriate timing,
> but arrive (due to network buffering) in a bulk.
> 
> Any endpoint is free to abort an association anytime to protect itself.
> But I find it hard to specify appropriate thresholds and specifying
> is not needed for interoperability.
>> 
>> It also looks like it's allowed for a sender to leave gaps in the TSN
>> space when sending (non-fragmented) DATA chunks.  This is also the case
> No, that is not true. The sender needs to assign TSN consecutive.
> Section 3.3.1 contains:
> 
> The TSNs of DATA chunks sent SHOULD be strictly sequential.
> 
>> for QUIC (for the analogous numer space), and in QUIC's security
>> considerations there is a note that a sender who suspects the peer of
>> misbehaving (i.e., sending false/speculative ACKs) can deliberately
>> leave such a gap and abort the association if the non-existent TSN is
>> ACKed.  Do we want to make such a statement here as well?
> Section 12.4 contains:
> 
> An SCTP implementation SHOULD abort the association if it receives
> a SACK chunk acknowledging a TSN that has not been sent.
> 
> I think this is the best we can state...
>> 
>> Section 12.2.3
>> 
>>  Whenever ESP is in use, application-level encryption is not generally
>>  required.
>> 
>> I suggest removing this statement; we now have plenty of examples where
>> double-encryption is actually the right solution, and advances in
>> computing power have removed many reasons to desire avoiding it.
> I removed that statement.
>> 
>> Section 12.2.4.1
>> 
>>  The design of SCTP is resistant to flooding attacks, particularly in
>>  its use of a four-way startup handshake, its use of a cookie to defer
>>  commitment of resources at the responding SCTP node until the
>>  handshake is completed, and its use of a Verification Tag to prevent
>>  insertion of extraneous packets into the flow of an established
>>  association.
>> 
>> I suggest mentioning here that using distinct initial TSN and
>> initiate tag values improves robustness against such attacks, by making
>> attackers guess 32 more bits of values.  (§3.2.2 and 3.2.3 do allow for
>> the values to be the same, with concomitant reduced protection.)
> There are a couple of points here:
> 1. An endpoint can select the Initial TSN in whatever way it wants.
>   It will be able to interoperate.
> 2. Using a randomized Initial TSN makes it harder for an off-path
>   attacker to inject DATA chunks, but not other chunks. So the
>   impact of the selection of the Initial TSN is limited (to user data).
> 3. Selecting the Initial TSN to be the same value as the verification
>   tag is somewhat strange. It is random, but the same value as another
>   one. So it does not really give you increased security, possibly it
>   helps in debugging (since you see in a trace which does not contain
>   the initial hand shake the Initial TSN).
> 4. I am aware of an (old) implementation, which selected the Initial
>   TSN to be 0.
> 5. Major implementations select the Initial TSN as a random number.
> 
> So contemplating about it, I think we could remove the text that
> the verification tag and the initial TSN MAY be set tot the same value and
> add a text stating that the value SHOULD be a random number.
> 
> That reflects reality for a lot of implementations, provides a more
> consistent selection, but still allows other choices (for whatever
> reason).
Revision 18 now replaces

Defines the initial TSN that the sender will use. The valid range is
from 0 to 4294967295. This field MAY be set to the value of the
Initiate Tag field.

by

<t>Defines the initial TSN that the sender of the INIT chunk will use.
The valid range is from 0 to 4294967295 and the Initial TSN SHOULD be set to a
random value in that range.
The methods described in <xref target='RFC4086'/> can be used for the
Initiate TSN randomization.</t>

Similar text for the INIT ACK chunk.
>> 
>>  The IP Authentication Header and Encapsulating Security Payload might
>>  be useful in reducing the risk of certain kinds of denial-of-service
>>  attacks.
>> 
>> Current IPSECME WG guidance is to prefer ESP with NULL encryption over
>> AH, so I'd recommend not mentioning AH here.
> Text is changed to:
> 
> <t>ESP might be useful in reducing the risk of certain kinds of
> denial-of-service attacks.</t>
>> 
>> Section 12.4
>> 
>>  INIT ACK chunk will be larger than the original INIT chunk.  An SCTP
>>  implementation SHOULD attempt to make the INIT ACK chunk as small as
>>  possible to reduce the possibility of byte amplification attacks.
>> 
>> The emerging consensus in QUIC and (D)TLS seems to be to limit the
>> amplification factor to at most three times the received data; do we
>> want to mention that heuristic here?
> I'm not sure...
> 
> From a high level, a factor of 3 is OK. Since you can use the concatenation
> of the INIT chunk and the INIT ACK chunk (without the state cookie parameter)
> as the state cookie and you can assume that size of the INIT ACK chunk
> without the state cookie is about the size of the INIT chunk.
> 
> However, the state cookie also has some overhead, like the MAC, some
> address information, a timestamp, tie tags, some other implementation
> specific stuff.
> 
> Testing with the shortest possible INIT chunk of 20 bytes, FreeBSD responds
> with an INIT ACK chunk of 196 bytes and Linux responds with an INIT ACK chunk
> of 252 bytes. This can be optimised a bit (at least on FreeBSD), but already
> the HMAC-SHA1 gives 20 bytes overhead, which is as large as the input...
> 
> For larger INIT chunks (more supported extensions), the ratio would go against 3...
> 
> So I would keep the text as is.
>> 
>> Section 15.6
>> 
>>  SCTP services can use contact port numbers to provide service to
>>  unknown callers, as in TCP and UDP.  IANA is requested to open the
>>  existing "Service Name and Transport Protocol Port Number Registry"
>>  for SCTP using the following rules, which we intend to mesh well with
>>  existing port-number registration procedures.  [...]
>> 
>> I don't really see anything following that looks like rules for IANA to
>> follow.  Is that phrase stale text from 4960?  (Deferring the actual
>> procedures to RFC 6335 seems like the right approach.)
> What is following is the deferral to RFC 6335. But let us make this clearer.
> What about just using:
> 
> <t>SCTP services can use contact port numbers to provide service to unknown
> callers, as in TCP and UDP.
> An IESG-appointed expert reviewer supports IANA in evaluating SCTP port
> allocation requests, according to the procedure defined in
> <xref target='RFC8126'/>.
> The details of this process are defined in <xref target='RFC6335'/>.</t>
Revision 18 now contains the text suggested by me.
>> 
>> Section 18
>> 
>> Some of these references probably don't need to be classified as
>> normative.  e.g., RFC 3873 is the MIB for SCTP, but we don't require its
>> implementation, and RFC 4301 is only referenced for "operators might
>> consult [RFC4301]".
> Done.
>> 
>> Appendix A
>> 
>> I'd consider updating the note about the modifications made to the
>> sample code; noteworthy changes appear to include the use of fixed-width
>> types and some general cleanup for modern C best practices.
> I changed the sentence to:
> 
> The code is somewhat modified from <xref target='WILLIAMS93'/>, to ensure
> portability between big-endian and little-endian architectures, use fixed
> sized types to allow portability between 32-bit and 64-bit platforms, and
> general C code improvements.
>> 
>> Also, it might be worth checking for undefined behavior -- "1 << (31 -
>> i)" will be UB for i==1 on systems with 32-bit int, since 2**31 is not
>> representable in (signed) int, which is the default type in which the
>> expression will be evaluated, even if the result is going to be assigned
>> to an unsigned type.
> I guess you mean for i == 0. I changed the line to
>      rw |= 1UL << (31 - i);
>> 
>> 
>> 
>

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 19 Jan 2022 22:29:18 -0800

Hi Michael,

My apologies for the slow response time -- as I mentioned on github, I was
sick last week and am still catching up from the holiday backlog.

> address the issues you raised.

That's a great idea; thank you both.

> not because I'm not willing to accept any further changes based on an upcoming

Understood, and thanks again.

> Do you need more information?

I think I basically have enough information now, but I think I may have
asked my question poorly -- I should have put more text here instead of
just referring forward to §5.1.3.  In particular, I was mostly only
concerned about the MAC, or to be more precise, the cryptographic integrity
protection that a MAC provides (but which could be provided in another way
as well).  So while it's good to have confirmation that there is no
requirement for privacy (or "confidentiality protection" as we might call
it), I was pretty sure that there was no *requirement* for privacy at all.

My takeaway here is that the sender of the state cookie needs to be able to
make sure that the information in the state cookie is not modified between
when it was generated and when it is received back (e.g., because otherwise
the return-routability check can be forged).  This brings me back to
§5.1.3's "Inside this State Cookie, the sender SHOULD include a MAC".  Why
is this not "MUST include a MAC"?  If it's just a reluctance to
over-specify mechanism (since the method is, after all "strictly a private
matter for the receiver of the INIT chunk"), then I would propose that we
change the requirements language to cover the required property and not a
specific mechanism.  That might be, for example, "the sender MUST provide
integrity protection on the state cookie (e.g., via a MAC such as HMAC
[RFC2104]) so that any modification of it will be detected and the modified
cookie rejected as invalid.  The State Cookie SHOULD include a timestamp on
when the State Cookie was created and the lifespan of the State Cookie,
along with [...]".

There might be some second-order corrections in the following paragraph as
well, such as "method used to generate the MAC (or otherwise provide
integrity protection)" and "Integrity protection is mandatory to prevent
denial-of-service attacks".

> Do you need more information?

I think that helps me understand better, but I would like to probe the
topic further: if the PPID is to be treated by SCTP as just an integer
parameter like any other, then why is it necessary to call out that "this
field is not touched by an SCTP implementation" and "therefore, its byte
order is not necessarily big endian" and "[t]he upper layer is responsible
for any byte order conversions to this field"?  Would it not suffice to say
"this value is passed to SCTP as an integer by its upper layer and sent to
its peer"?

In particular, I don't understand which frame of reference the statement
about "byte order is not necessarily big endian" is intended to apply to.
What you said above seems to be saying that the PPID as sent on the wire
actually is always a big-endian integer, and I'm not sure what other
context the *protocol* specification should be concerned about such that we
need to call out the potential difference.  The only thing I can come up
with is that the interface from SCTP implementation to application is not
actually passing a (platform-native) integer but rather provides a 4-byte
buffer whose contents are the network-byte-order encoding of an unsigned
integer.  But is that really a matter for the protocol specification?

> > GitHub infrastructure, please see my comments for the pull request

Thanks for those as well.  I'm sorry that it ended up being so much work
for you to review, with several bad/flawed suggestions included, and I
appreciate that you took the time to give it a careful review.

> > Note: This page has been replaced by the RFC Style Guide. Please see that page for current information.

(I don't think it was a fully comprehensive "replacement" process when
converting that page to the RFC Style Guide, but you are correct that there
does not seem to be a document that claims to be an in-force document and
contains a line count limit for the document Abstract.  Accordingly, there
is no process requirement that I can point to and insist on substantive
edits/cuts to the Abstract, even if I continue to think that doing so would
improve the document.)

> > ASCII) or whatever you suggest. Please advise.

Of those options, "leave it unchanged" is the least-bad to me.

> I kept the redundant text. I'll remove it on explicit request from you.

Your confirmation that there is desire for the emphasis is enough for me.

> > I don't know (but I'm aware that they exists).

[Just noting here that I put more detailed discussion up in the DISCUSS
section; the summary is that I think strong integrity protection should be
a MUST but use of a MAC specifically should not be a MUST.]

> appropriate in your view.

The text in -18 seems fine to me.  This is probably the "safer" way to make
them be at the same level, since trying to make both into "MUST" would make
any (probably hypothetical, but we can't be sure) implementation that does
send such empty chunks immediately noncompliant.

I agree.

> > up to 128 KB to the handshake.

No need to answer, since we agree that we should not change anything here,
but I don't understahd how this negotiation would add kilobytes to the
handshake, as opposed to just 128 bytes.

> > states "The first byte in the stream has an offset of 0.".

That is true, and reconciling the strong consensus for QUIC as specified in
RFC 9000  with the text of draft-gont-numeric-ids-sec-considerations is why
the latter has not been advanced to the IESG Evaluation state.  QUIC has
some mitigating factors, such as all packets bearing integrity protection
(and packets carrying STREAM frames bearing strong integrity protection
using cryptographic key material known only to the two protocol endpoints)
and the contents of STREAM frames always being encrypted using keys known
only to the two protocol endpoints.  There is no scope for an attacker to
inject traffic with guessed stream offsets without first breaking the QUIC
cryptographic handshake.  SCTP (and TCP as well) does not have the benefit
of this strong cryptographic protection, and as such have to do with more
modest protections against off-path attacks.

> > So we did not catch it...

Thanks for the detailed explanation; it makes a great deal of sense now.  I
will leave it to you as to whether this merits mention in the list of
changes since RFC 4960 -- in some sense it's not a change per se, just
correcting the documentation to match the way that it has to work.

> > is not needed for interoperability.

Okay, let us leave this alone and not specify anything.

> > The TSNs of DATA chunks sent SHOULD be strictly sequential.

That is a SHOULD, not a MUST, though.  I think when I originally read that
text I saw it as guidance to both assign sequentially and make the order of
the chunks sent on the wire that same sequential order, but since it's only
a SHOULD it seems to allow for both sending (non-fragmented) chunks "out of
order" and for leaving gaps in the TSN space.  Do we want to split this
into separate statements about allocating TSNs and about ordering
(non-fragmented) chunks on the wire?

> > I think this is the best we can state...

Ah, I missed that; my apologies!

I think that (noting the previous question of SHOULD vs MUST assign TSN
sequentially) in some sense we *could* "do better", in that we could have a
specific statement that "if a sender suspects the peer of misbehaving, it
could leave a gap in assigned TSNs so as to reveal whether the peer is
acknowledging packets it has not received".  But of course, if there is a
requirement to assign TSNs in strictly sequential order, then you are
correct and the current text is the best we can do.

> Similar text for the INIT ACK chunk.

Thank you!

> > So I would keep the text as is.

Okay.  Thanks for investigating!

> Revision 18 now contains the text suggested by me.

Excellent; thanks.

> > general C code improvements.

Thanks.

> > I guess you mean for i == 0. I changed the line to

Yes, I meant for i==0, and that looks to be a good fix.

I happened to look a bit more after I balloted, and I think that this bit of
generate_crc32c() can also invoke unsigned behavior:

     uint8_t byte0, byte1, byte2, byte3;
  [...]
     byte0 = result & 0xff;
     byte1 = (result>>8) & 0xff;
     byte2 = (result>>16) & 0xff;
     byte3 = (result>>24) & 0xff;
     crc32 = ((byte0 << 24) |
              (byte1 << 16) |
              (byte2 << 8)  |
              byte3);

since byte0 is a uint8_t, with integer conversion rank smaller than that of
'int', the value contained in byte0 is promoted to type int prior to
evaluating the leftwise bit shift, and for 32-bit int, the 24-bit shift can
cause the sign bit to be set and invoke undefined behavior.
Since all of the byte[0-3] are explicitly clamped to 0xff prior to use, it
seems like it would be safe to declare them as uint32_t rather than
uint8_t, avoiding the promotion to 'int'.

Thanks again for the careful explanations and updates,

Ben

---

From: [email-masked]
Date: Fri, 28 Jan 2022 00:36:17 +0100

> On 20. Jan 2022, at 07:29, Benjamin Kaduk <[email-masked]> wrote:
> 
> Hi Michael,
> 
Correct.
> over-specify mechanism (since the method is, after all "strictly a private
I think the intention was to be flexible in the method being used for
integrity protection. But if I understand the notion of a MAC correctly,
these methods are called MAC. Is that correct?
If yes, I would suggest to use the following text:

Inside this State Cookie, the sender MUST include a MAC
(see <xref target='RFC2104'/> for an example) to provide integrity protection
on the State Cookie.
The State Cookie SHOULD also contain a timestamp on when the
State Cookie is created, and the lifespan of the State Cookie, along with all
the information necessary for it to establish the association including
the port numbers and the verification tags.</t>

I think the text in 5.1.5:

Authenticate the State Cookie as one that it previously generated by
comparing the computed MAC against the one carried in the State Cookie.
If this comparison fails, the SCTP packet, including the COOKIE ECHO chunk
and any DATA chunks, SHOULD be silently discarded,

should also be changed to "MUST be silently discarded".

Does that improve things for you? What else is needed?
> integer.  But is that really a matter for the protocol specification?
I have been thinking about this and I agree, the description is not very
good. The text regarding the endianness was introduced in
https://datatracker.ietf.org/doc/html/rfc4460#section-2.50

The important point is that the SCTP stack does NOT touch this field and
that the application is responsible for the conversion from host to network
byte order.

So I suggest to replace:

Note that this field is not touched by an SCTP implementation;
therefore, its byte order is not necessarily big endian.
The upper layer is responsible for any byte order conversions to this field.</t>

by

Note that this field is not touched by an SCTP implementation;
The upper layer is responsible for the host to network byte order conversion of this field.</t>

I guess this describes it more clearly.

This should also make clear, why this description is needed in a specification,
since it describes how an implementation processes this field. A transport
protocol has an interface the network and to its upper layer. Both need to
be specified.

Please note that initially there were SCTP stacks which converted the PPID
from host to network byte order. This made it hard to write portable
applications. In RFC 2960, only the description of the SEND primitive in
https://datatracker.ietf.org/doc/html/rfc2960#section-10.1
contained:
   o  payload protocol-id - A 32 bit unsigned integer that is to be
      passed to the peer indicating the type of payload protocol data
      being transmitted.  This value is passed as opaque data by SCTP.

This was not clear enough and also the API is only informational. That is
why we wanted to stress the point in the main text.

Does the above text change resolve the issue?
> appreciate that you took the time to give it a careful review.
You are welcome. Addressing the issues improves the text.
> Of those options, "leave it unchanged" is the least-bad to me.
OK. The RFC Editor still can come up with suggestions to shorten it...
> handshake, as opposed to just 128 bytes.
The stream sequence numbers of the streams are independent. So you would
need to negotiate a 2 byte initial stream sequence number for each stream.
There are up to 2^16 stream resulting in 128KB.

Not sure how you come up with 128 bytes. If you would use the same initial
stream sequence for all streams, then you would only need to negotiate
2 bytes.

If the protection against blind attackers of the base specification is
not good enough, SCTP authentication can be used. Several implementations
(including the FreeBSD and Linux stack) support it.
> modest protections against off-path attacks.
That is true, you are right. Using SCTP authentication or TCP AO would give
more protection, more in the spirit of QUIC, I guess. SCTP authentication
is supported pretty well, not sure about TCP AO.
> correcting the documentation to match the way that it has to work.
I added:

<li><t>Fix the API for retrieving messages in case of association failures.</t></li>
> (non-fragmented) chunks on the wire?
The requirement of sequential TSN assignments for fragments is already covered
in Section 6.9:

2) The transmitter MUST then assign, in sequence, a separate TSN to each of the
   DATA chunks in the series. The transmitter assigns the same Stream Sequence
   Number to each of the DATA chunks. If the user indicates that the user message
   is to be delivered using unordered delivery, then the U flag of each DATA chunk
   of the user message MUST be set to 1.

It is using a MUST. Please note that the sentence you are referring to does not
speak about assigning, but about sending.

What you really want is that the DATA chunks arrive at the receiver in strictly
increasing by 1 order. This way to loss recovery is triggered. You can't directly
control packet loss or reordering on one network path, but if you are using
loadsharing and you are sending on multiple paths at same time and the delay
on the multiple paths is not the same, you might use an intelligent way
of sending packets in a way they *receive* in sequence.
However, in the case of not using loadsharing, sending them in sequence might
be a good way to achieve the goal of reception in the correct order (not
taking reordering / loss on the path into account).

That is the reason for the SHOULD in sending the TSNs.

Does that make things clearer? Do you want additional changes?
> correct and the current text is the best we can do.
If the sender decides to leave a gap in the TSN sequence space, the
receiver has to report it over and over. It can't move the cumTSN.
This specification will then also require
* the sender to fast retransmit it after three gap reports and enter fast recovery
* the sender can't send more than 2^31-1 DATA chunks after the gap

So this behaviour would require some more magic. This could be done
on a temporary base with specific code do not trigger fast retransmissions
and not doing fast recovery, since there was no packet loss.
I think this can be implemented, since we are not using MUST level for
fast recovery, fast retransmission, and the 2^31-1 limit.
But I really would leave it implementation specific and not describe this
in this document. I'm also not aware of any other feature in SCTP were
one end-point actively tries to figure out if the peer is violating the
specification by acting like a conformance tester. The method is more to
protect yourself in case you observe the peer is violating the specification.

I would suggest to leave adding a conformance testing like features to
a separate specification. Let me know if that is acceptable to you. 

What we can do to improve things is replace 

by

An SCTP implementation MUST abort the association if it receives
a SACK chunk acknowledging a TSN that has not been sent.

Would you prefer that?

<--- SNIP ---->
Removing content which we agreed on...
> uint8_t, avoiding the promotion to 'int'.
You are right, I changed the type of byte[0123] to uint32_t.