Subject: [secdir] Secdir review of draft-ietf-tls-record-limit

From: Alan DeKok <[email-masked]>
Date: Thu, 22 Feb 2018 17:21:05 -0500

I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other last call comments.

  The summary of the review is ready with nits.

4.  The "record_size_limit" Extension
	... an endpoint
   MUST NOT send a value higher than the protocol-defined maximum record
   size ...

Comment: That's good, so later we have:

   The record size limit can interact with the maximum transmission unit
   (MTU) in DTLS, but it is a separate and independent constraint on
   record size.

Nit:  perhaps say that this is an *additional* constraint.  Which is (to me) a clearer indication that both constraints must be matched.  "independent" constraints sound like not only they vary independently, but that they can be applied independently (i.e. separately).  Saying "additional" constraint is a clearer indication that both constraints must be applied at the same time.

   In particular, it is not appropriate to use the record
   size limit in place of path MTU detection. 

Q: How would that be done?  I don't mean that the document needs to explain how to do something wrong.  I mean that it would be good to explain the misunderstanding which would lead to using record size limit in place of path MTU detection.

  e.g. "the reception of an illegal_parameter error on a session gives no information about the allowed MTU size"

   The record size limit is
   a fixed property of an endpoint that is set during the handshake and
   fixed thereafter.  In comparison, the MTU is determined by the
   network path and can change dynamically over time.

Comment:  it would be good to give guidance on what to do here, and what happens in error cases.

  e.g. should the record size limit to be set at the start of a session to the *smallest expected MTU*?  If so, what are the side effects?

  Or what about this - the MTU is large at the start of a session, and a record size limit is negotiated which matches that MTU.  At some later time, the MTU changes to a lower value than the negotiated record size limit.

  What happens then?  The application may receive an ICMP message indicating destination unreachable, with a code indicating fragmentation needed.  If Don't Fragment (DF) is not set.. the packet cannot be sent.

  Should the session be closed?  If not, why not?  If so, should the application keep track of minimum MTU across multiple sessions, and negotiate a value of record size limit which is more likely to work?

  It would be good to have guidance for edge / error conditions.  They tend to be a source of network problems and interoperability issues.

5.  Deprecating "max_fragment_length"
	...A server that supports the "record_size_limit"
   extension MUST ignore and "max_fragment_length"

Nit: this is probably "any" instead of "and".

  7.  IANA Considerations
	...
   This document registers the "record_size_limit" extension in the TLS
   "ExtensionType Values" registry ...

   In the same registry, the "max_fragment_length" [[has been|will be]]
   changed to a status of not recommended.

Comment: the registry has no "status" column.

  It may be useful to update that registry to add a "deprecated" note, perhaps as is done with the ICMP parameters registry:

https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml

  In which case the "max_fragment_length" entry should be changed to:

Value				1
Extension name		max_fragment_length (deprecated)
Reference			RFC6066, draft-ietf-tls-record-limit

---

From: Martin Thomson <[email-masked]>
Date: Fri, 23 Feb 2018 12:19:19 +1100

Thanks Alan,

A few changes based on your feedback here:
https://github.com/tlswg/tls-record-limit/pull/17

> Q: How would that be done?  I don't mean that the document needs to explain how to do something wrong.  I mean that it would be good to explain the misunderstanding which would lead to using record…

How would you mess this up?  Set record_size_limit to your MTU with
the expectation that this is sufficient.  Worse, the expectation that
peer has also done this.

The remainder of the paragraph is intended to make that distinction
clear.  i.e., this limit is inherently fixed by negotiation, the other
is dynamic.

> Comment:  it would be good to give guidance on what to do here, and what happens in error cases.

DTLS (RFC 6347) already has some fairly extensive guidance on PMTUD,
which I didn't want to replicate here.  You hit a lot of the issues in
your questions.  With some better citations, this is what I came up
with:

+The path maximum transmission unit (PMTU) in DTLS also limits the size of
+records.  The record size limit SHOULD be set independently of PMTU.  The
+record size limit is fixed during the handshake and so is best set based on
+constraints at the endpoint and not the current network environment.  In
+comparison, the PMTU is determined by the network path and can change
+dynamically over time.  See {{?PMTU=RFC8201}} and Section 4.1.1.1 of {{?DTLS}}
+for more detail on PMTU discovery.

> Comment: the registry has no "status" column.

Ahh, a problem of concurrent updates.
draft-ietf-tls-iana-registry-updates (which is going out just ahead of
this) adds that column.  I don't think that there was any intention of
creating an explicit dependency, which won't make sense in 5 years
time, but I guess we can add an informational reference.

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 22 Feb 2018 19:49:57 -0600

> Thanks Alan,

Yes, thanks!

> time, but I guess we can add an informational reference.

Unfortunately draft-ietf-tls-iana-registry-updates is on the 5 April
telechat and will need to undergo another IETF Last Call to call out
downrefs for a handful of documents that it needs to Update.  So,
probably we should shuffle things around so the depndency goes the
other way.

-Benjamin

---

From: Martin Thomson <[email-masked]>
Date: Fri, 23 Feb 2018 15:33:14 +1100

> probably we should shuffle things around so the depndency goes the

I don't think that this is so urgent that it can't wait for the
registry updates to happen.

---

From: Alan DeKok <[email-masked]>
Date: Fri, 23 Feb 2018 09:49:11 -0500

> https://github.com/tlswg/tls-record-limit/pull/17

  Thanks. That helps.

> clear.  i.e., this limit is inherently fixed by negotiation, the other

  My point is that those two limits may agree or disagree dynamically.  So it would be good to give guidance on what to do when a previous agreement dynamically changes to disagreement.

> your questions.  With some better citations, this is what I came up

  That still doesn't give guidance.  What happens if the record size limit is fine at the start of a session, and then MTU changes, and the packets no longer make it through?   What does the application do?

  RFC 6347 Section 4.1.1.1 gives some guidance, but I think not enough.  Exposing the MTU to the application is good, but what does the application *do* with this information?

  e.g. PMTU should be exposed to the application as per Section 4.1.1.1 of {{?DTLS}}.  If the PTMU changes, the application may discover that the new MTU is smaller than the record size limit.  In that situation, the only recourse available may be to close the session, and to open a newer one with a smaller record size limit that is compatible with the new MTU.

  Alan DeKok.

---

From: Kathleen Moriarty <[email-masked]>
Date: Fri, 23 Feb 2018 11:45:09 -0500

> registry updates to happen.

OK, I can move it to the next telechat and Ben can take over the AD
role for the draft.

Thanks.

-- 

Best regards,
Kathleen

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 23 Feb 2018 13:17:14 -0600

>   e.g. PMTU should be exposed to the application as per Section 4.1.1.1 of {{?DTLS}}.  If the PTMU changes, the application may discover that the new MTU is smaller than the record size limit.  In…

The record size limit is a cap, not a mandatory record size.  So,
the application could just send smaller records that fit within the
new PMTU, provided both sides have the proper logic around PMTU
discovery and record sizing.

-Benjamin

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Feb 2018 12:31:45 +1100

On Sat, Feb 24, 2018 at 6:17 AM, Benjamin Kaduk <[email-masked]> wrote:
> The record size limit is a cap, not a mandatory record size.

That's right.  Alan, does that address your primary concern?

>>   RFC 6347 Section 4.1.1.1 gives some guidance, but I think not enough.  Exposing the MTU to the application is good, but what does the application *do* with this information?

In many cases, nothing.

My understanding of DTLS implementations is that the size of a write
determines the size of the packet that is ultimately sent.  Say I
write 100 octets to the socket, so DTLS writes out a record of 128
octets.

What stacks do when a write exceeds PMTU differs.  The same applies to
this new limit:

* The stack I work on splits records based on its understanding of the
MTU, but it only learns that during the handshake and so relies mostly
on the application making smaller writes.  (There is currently no way
to set the MTU, which is an open bug.)  It also sends a single record
in each packet for application data, so the net effect of a large
write is multiple UDP datagrams with one record each.

If your stack operates like this, then the information is pretty much
just advisory.  The only thing you might contemplate is changing any
coalescing rules you might have to compensate.

* If a stack did automatic splits, but then put multiple records in
the same datagram, that's the same for a using application.

* If a stack rejects large writes and forces the application to write
within its constraints, then that stack needs to be clear about what
limit was in force, such as through the use of specific error codes.
Such stack that adds this feature would need to be careful about
introducing this feature.

---

From: Hannes Tschofenig <[email-masked]>
Date: Tue, 27 Feb 2018 10:27:07 +0000

My 5 cents. We currently only have a very basic implementation of the Maximum Fragment Length extension and have been waiting for the RSL extension to complete before we add the functionality to the Mbed TLS stack.

The problem we have with IoT devices is that they are quite limited in terms of available RAM*. Additionally, public key crypto requires a lot of RAM when performance optimizations are enabled. Hence, there is not much left for the message buffers.

Hence, we want to let the other communication party know what limits there are. As pointed out in this discussion thread there is an interworking between the MTU/PMTU and the MFL/RSL mechanisms when a stack decides about fragmenting (or combining) messages into records.

My thinking was that the relationship between MTU/PMTU and MFL/RSL is actually fairly easy, namely something like max_fragment_size=min(MTU/PMTU, MFL/RSL)

Of course, implementations are going to be complex since they cannot just look at the fragment size alone. Instead you also have to consider what the stack or the application should do with the received data.

For example, imagine a certificate message that may contain multiple certificates that then need to be processed one by one by the recipient. Here the stack needs to be "smart" enough to send enough data that the recipient can actually do something with it (like one complete certificate) but also not too much so that it still fits into the buffer. For application data it may be less complicated since the application developer who had set the RSL limit is most likely aware of what to do when the stack cannot send all the data.

In my earlier review I pointed out that setting reasonable values by the developer may actually be complicated since the size of transmitted messages in DTLS/TLS depends very much on the selected ciphersuite and various other parameters (key size, padding of the algorithm, padding for avoiding traffic analysis, authentication tag size, use of certain extensions). Additionally, there are higher layer protocols (such as CoAP) that also add header information. A developer therefore has to know quite a bit to pick the values appropriately when they are using a constrained device.

FWIW: The draft currently says that "unprotected messages - handshake messages in particular - are not subject to this limit.". I am not sure whether I fully understand that sentence. Does it mean that the limit does not apply to handshake messages at all or only to handshake messages that are unprotected? In either case I am not sure whether that scoping makes sense since handshake messages can become fairly large.

Ciao
Hannes

PS: The RAM limitation is obviously independent of whether you use TCP or UDP. Hence, I expect this extension to be applicable to both. Of course, with TCP you additionally have a layer in there that offers fragmentation...

-----Original Message-----
From: Martin Thomson [mailto:[email-masked]]
Sent: 26 February 2018 02:32
To: Benjamin Kaduk; Hannes Tschofenig
Cc: Alan DeKok; [email-masked]; IESG; [email-masked]
Subject: Re: Secdir review of draft-ietf-tls-record-limit

In many cases, nothing.

My understanding of DTLS implementations is that the size of a write determines the size of the packet that is ultimately sent.  Say I write 100 octets to the socket, so DTLS writes out a record of 128 octets.

What stacks do when a write exceeds PMTU differs.  The same applies to this new limit:

* The stack I work on splits records based on its understanding of the MTU, but it only learns that during the handshake and so relies mostly on the application making smaller writes.  (There is currently no way to set the MTU, which is an open bug.)  It also sends a single record in each packet for application data, so the net effect of a large write is multiple UDP datagrams with one record each.

If your stack operates like this, then the information is pretty much just advisory.  The only thing you might contemplate is changing any coalescing rules you might have to compensate.

* If a stack did automatic splits, but then put multiple records in the same datagram, that's the same for a using application.

* If a stack rejects large writes and forces the application to write within its constraints, then that stack needs to be clear about what limit was in force, such as through the use of specific error codes.
Such stack that adds this feature would need to be careful about introducing this feature.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

---

From: Martin Thomson <[email-masked]>
Date: Wed, 28 Feb 2018 12:00:43 +1100

Thanks Hannes,

> For example, imagine a certificate message that may contain multiple certificates that then need to be processed one by one by the recipient. Here the stack needs to be "smart" enough to send enoug…

Yeah, if you are so constrained that a single datagram is all the
scratch space you have, then big certificates can be nigh on
impossible to manage.  My understanding is that raw public keys are
more common in this context.

> FWIW: The draft currently says that "unprotected messages - handshake messages in particular - are not subject to this limit.". I am not sure whether I fully understand that sentence. Does it mean…

This was written with two things in mind.  TCP makes it easy to apply
back pressure and thereby enable progressive reads for larger
cleartext messages, assuming that you can read it all without holding
everything in memory at once.  More critically, a ClientHello will be
sent in ignorance of any preferences on a server, so a server has to
cope somehow.

If you remain concerned about constrained *clients*, maybe we can
change the requirement and have it apply immediately for the server's
handshake messages.  In TLS 1.2, it would then also apply to the
client Certificate, which I suppose is something.  It's definitely
possible to make this change. (Now that I think on it, my
implementation may already do this.  I should check.)