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

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 4 Feb 2022 20:09:51 -0800

Hi Michael,

Trimming somewhat...

On Fri, Jan 28, 2022 at 12:36:17AM +0100, [email-masked] wrote:
> 
> > On 20. Jan 2022, at 07:29, Benjamin Kaduk <[email-masked]> wrote:
> > 
> > On Sun, Jan 16, 2022 at 03:11:34PM +0100, [email-masked] wrote:
> >> 
> >>> On 26. Dec 2021, at 13:00, [email-masked] wrote:
> >>> 
> >>>> On 16. Dec 2021, at 15:48, Benjamin Kaduk via Datatracker <[email-masked]> wrote:
> >>>> 
> >>>> 
> >>>> ----------------------------------------------------------------------
> >>>> 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?
> > 
> > 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
> Correct.
> > §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
> 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?

A thing that you add to some data to protect its integrity is called a MAC,
yes.  There is some slight subtlety relating to the other conversation we
had about encrypting the cookie, though, since if you (for example) use an
AEAD cipher to encrypt the cookit, that provides both the integrity
protection as well as the encryption, but in a certain sense it's something
of a stretch to say that it "includes a MAC".  (In other senses it's still
accurate -- AES-GCM includes the Galois MAC, chacha20/poly1305 includes the
poly1305 MAC, CCM is Counter with CBC MAC, etc.  This pattern maybe breaks
down for the OCB AEAD mode, though)

> 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>

So in light of the above, I think this looks fine.  I'm willing to help
draft more text if you want to handle the AEAD-encrypted cookie case, but
given that we don't see a reason to encrypt the cookie, I'm happy to say
that this is good enough and leave it at that.
 
> 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?

Yes, those two changes look good.

I spot-checked everywhere that talks about "MAC" and didn't see any other
changes needed.  There are maybe ~200 instances of "Cookie", so I didn't
try to look through them.  I think we can consider these two changes as
sufficient.

> > 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".
> > 
> >>>> 
> >>>> 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?
> > 
> > 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?
> 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.

That is much more clear about what we're intending to say, yes.
However ... in some ways it leaves me even less comfortable than I was
previously.  (The nature of this discomfort is probably not something that
merits a DISCUSS ballot or even further changes to the document, though.)

To be more concrete, in my understanding, normal IETF protocol documents
specify the "bits on the wire", that is, how to interpret the fields as
they are sent and received, so that communications peers using different
implementations will interoperate.  So typically we would talk about a
protocol field, say that the bits on the wire are a 4-byte unsigned
integer, and that would be enough.  We do, of course, also have RFCs that
talk about APIs, from the Generic Security Service API of RFC 2743 to the
Sockets API (many RFCs, e.g., 3493).  GSS-API even has both the API and the
wire protocol in the same document.  So in that sense I should not be
bothered by having an API specification and protocol specification in the
same document, and yet I am.  Perhaps it is just that the API boundary is
drawn in an almost "gerrymandered" form, that juts out to have a special
behavior for just this one field, when we otherwise leave the concrete API
details to implementations and RFC 6458.  My instincts are telling me that
this detail should be "out of scope" for this document and would be better
covered elsewhere.

That said, as you describe below, the historical reality makes a strong
case for saying *something* here to alert implementations to the oddity.
We can't go back and change history, even if it would seem prettier to me
to leave the API details to RFC 6458.

> 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?

The above change is probably enough, yes.

I might consider an additional change in §11.1.5,

OLD:
      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.

NEW:
      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.  Note that the application is responsible
          for the host to network byte order conversion of this field,
          which is passed by SCTP as 4 bytes of opaque data.

since "passed as opaque data" might require some work for the reader to
untangle.

> >>>> 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.
> > 
> > I agree.
> > 
> >>> However, you would need to negotiate the initial values which would add
> >>> 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.
> 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.

Ah, that explains it -- thanks.

> 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.

(I can only assume I was very confused, and that's how I came up with 128
bytes.)

> >>>> 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.
> > 
> > Okay, let us leave this alone and not specify anything.
> > 
> >>>> 
> >>>> 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.
> > 
> > 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?
> 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.

I think I had just missed the "MUST then assign, in sequence" entirely when
I was reading -- that text is unchanged in the diff from RFC 4960 (which is
what I read most carefully) and I only did targeted reading of the
unchanged text.  So, that's my mistake entirely, and sorry for the mixup.
I agree that the text I was referring to is just about the order in which
to put things on the wire, and not the actual allocation of TSN values.

> 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?

I think no changes are needed.

> >>>> 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...
> > 
> > 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.
> 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. 

A separate specification sounds good.

> What we can do to improve things is replace 
> 
> An SCTP implementation SHOULD abort the association if it receives
> a SACK chunk acknowledging a TSN that has not been sent.
> 
> 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?

Thanks for the suggestion.  I would be happy to see it applied, though I do
not insist upon it.

Many thanks for the updates and for enlightening me on SCTP operation,

Ben

---

From: [email-masked]
Date: Sat, 5 Feb 2022 16:42:39 +0100

> On 5. Feb 2022, at 05:09, Benjamin Kaduk <[email-masked]> wrote:
> 
> Hi Michael,
Hi Benjamin,
> Trimming somewhat...
Doing the same...
> that this is good enough and leave it at that.
OK, then we leave the text as it is now. Encrypting the cookie was never
the intention of the specification, but still is allowed.
> Yes, those two changes look good.
OK. Done.
> integer, and that would be enough.  We do, of course, also have RFCs that
I think we have a small difference in our understanding what needs to
be in a protocol specification, at least for a transport protocol.

I think it needs to specify how it interacts with its lower and upper
layer and how it handles timers. The description of interactions with
its lower layer includes packet formats and so one. But also needs to
describe its interaction with its upper layers. This is why the RFCs
for TCP and SCTP include an (informational) API for the upper layer.
This is a generic API, since it is not tied to a specific implementation
of it. RFC 6458 is much more specific and provides an instance of the
abstract API in the context of the socket API.

Therefore it is important to state that the PPID is not touched by
the SCTP stack.
> this detail should be "out of scope" for this document and would be better
But we wanted this to be a feature available in all SCTP implementation.
It should not be a feature of a particular implementation.
Maybe a use case helps. Assume you have an application level protocol you
want to run over SCTP and most (if not all) of your implementation use
little endian as its host byte order. Then you could register two PPIDs,
the same number, but one instance in Litte Endian, one instance in Big Endian.
Then you could send the application data in host byte order and indicate
the byte ordering by the PPID. So if you are running most of your applications
on machines with host byte order being litte endian, you could save the
translation.
> to leave the API details to RFC 6458.
As said above, this is not intended to be socket API specific. That is why
we have it in the base specification, at least in the abstract API section.
> since "passed as opaque data" might require some work for the reader to
Done. I used "upper layer" instead of "application" and applied the same
change to §11.1.7 Receive.
> Thanks for the suggestion.  I would be happy to see it applied, though I do
Changed to a MUST.
> Many thanks for the updates and for enlightening me on SCTP operation,
Thanks a lot for helping to improve the specification!

Since I think all issues are resolved, I uploaded revision 19, so you
can see all changes. If more changes are needed, please let me know and
I'll integrate them in a revision 20, if needed.

Best regards
Michael