Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define an anti-forgery limit #3620

Merged
merged 19 commits into from
Jun 9, 2020
Merged

Define an anti-forgery limit #3620

merged 19 commits into from
Jun 9, 2020

Conversation

martinthomson
Copy link
Member

This defines a limit on the number of packets that can fail
authentication before you have to use new keys.

There is a big hole here in that AES-CCM (that is, the AEAD based on
CBC-MAC) is currently permitted, but we have no analysis to support
either the confidentiality limits in TLS 1.3 or the integrity limits in
this document. It is probably OK, but that is not the standard we apply
here.

So this might have to remain open until we get some sort of resolution
on that issue. My initial opinion is to cut CCM from the draft
until/unless an analysis is produced.

Closes #3619.

This defines a limit on the number of packets that can fail
authentication before you have to use new keys.

There is a big hole here in that AES-CCM (that is, the AEAD based on
CBC-MAC) is currently permitted, but we have no analysis to support
either the confidentiality limits in TLS 1.3 or the integrity limits in
this document.  It is probably OK, but that is not the standard we apply
here.

So this might have to remain open until we get some sort of resolution
on that issue.  My initial opinion is to cut CCM from the draft
until/unless an analysis is produced.

Closes #3619.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -tls labels May 1, 2020
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good overall, but a few comments


Endpoints MUST count the number of packets that are received but cannot be
authenticated. Packet protection keys MUST NOT be used for removing packet
protection after authentication fails on more than a per-AEAD limit. Endpoints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a "per-AEAD limit"?

{{AEBounds}} and {{ROBUST}}.

For AEAD_AES_128_GCM, AEAD_AES_256_GCM, and AEAD_CHACHA20_POLY1305 the
number of packets that fail authentication MUST NOT exceed 2^36. Note that the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This MUST NOT is a bit of a circuitous requirement. Perhaps "if the number of packets that fail authentication exceeds 2^36, the endpoint MUST immediately close the connection"

analysis in {{AEBounds}} supports a higher limit for the AEAD_AES_128_GCM and
AEAD_AES_256_GCM, but this specification recommends a lower limit. For
AEAD_AES_128_CCM the number of packets that fail authentication MUST NOT exceed
2^24.5; see {{ccm-bounds}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to above

the use of the associated AEAD function that preserves margins for
confidentiality and integrity. That is, limits MUST be specified for the number
of packets that can be authenticated and for the number packets that can fail
authentication. Any limits SHOULD reference any analysis upon which values are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a strange SHOULD. It's a recommendation about how the spec is to be written in the future. I would remove the normative here.

martinthomson and others added 2 commits May 8, 2020 09:47
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all of this. LGTM modulo the point below.


For AEAD_AES_128_GCM, AEAD_AES_256_GCM, and AEAD_CHACHA20_POLY1305, if the
number of packets that fail authentication exceeds 2^36, the endpoint MUST
immediately close the connection. Note that the analysis in {{AEBounds}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if "MUST immediately close the connection" is correct. Our recommendation, stated above, is to do a key update.

I think it would be fine to simply say "MUST NOT exceed X."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that this is the necessary reaction if the key update was not successful before this limit was reached. That we require a key update to be initiated before this is reached isn't any guarantee that the key update will succeed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thank you for the clarification.

Then, I think that the text could become less confusing if we move "MUST close the connection" to the paragraph above, and change this paragraph to just listing the numbers.

For example, the paragraph above can be changed to something like:
Endpoints MUST count the number of packets that are received but cannot be authenticated. If the number of packets that fail authentication exceeds a limit that is specific to the AEAD in use, the endpoint MUST immediately close the connection. Endpoints MUST initiate a key update before reaching this limit. Applying a limit reduces the probability that an attacker is able to successfully forge a packet; see {{AEBounds}} and {{ROBUST}}.

For AEAD_AES_128_GCM, AEAD_AES_256_GCM, and AEAD_CHACHA20_POLY1305, the limit on the number of packets that fail authentication is 2^36...

Based on input from @chris-wood, it appears as though the length
calculation was off.

Of course, the length calculation is off anyway, because 2^10 is
arbitrary and doesn't match the expected packet size. But as long as
we're being arbitrary, we can at least be *consistently* arbitrary.
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
martinthomson and others added 2 commits May 8, 2020 17:38
Co-authored-by: Felix Günther <mail@felixguenther.info>
Copy link
Contributor

@chris-wood chris-wood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @martinthomson! I derived the same bounds. This looks good to me.

draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
draft-ietf-quic-tls.md Show resolved Hide resolved
Copy link
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CCM part of this feels like playing with fire to me. I think I'd almost prefer to remove CCM from the spec entirely, and add it as an extension document once there exists a peer-reviewed analysis that doesn't have the 2^14 record size caveat.

Endpoints MUST count the number of packets that are received but cannot be
authenticated. If the number of packets that fail authentication exceeds a
limit that is specific to the AEAD in use, the endpoint MUST immediately close
the connection. Endpoints MUST initiate a key update before reaching this
Copy link
Contributor

@DavidSchinazi DavidSchinazi May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text feels unclear to me. It could be interpreted as "you MUST close the connection and initiate key rotation" which is silly. I think what's missing is the fact that the counter of invalid packets received is reset on key rotation. But if we have a "MUST initiate rotation", what's the point of the "MUST close the connection"?

limits on TLS record size. The maximum size of a TLS record is 2^14 bytes.
In comparison, QUIC packets can be up to 2^16 bytes. However, it is
expected that QUIC packets will generally be smaller than TLS records.
Where packets might be larger than 2^14 bytes in length, smaller limits might
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds scary to me. It feels like the spec is saying Computing the limits for larger packet sizes is left as an exercise to the reader. As a reader of this specification, I personally do not feel competent to get that right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what it says. And it should be scary. That is, if you want 64k packets, be prepared to read the papers. (However, if someone ignores this, the damage isn't that bad, as the worst effect from this set of AEADs is in reducing the margin by a factor of 16 to 2^-56/2^-53, which is still small.)

It also says that the margins are usually better in practice than the analysis permits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really nice to do the math for 2^16 byte packets in this document, but I'm not volunteering so I guess I can live with this as is

@@ -2029,6 +2079,104 @@ ffff00001b0008f067a5502a4262b574 6f6b656ea523cb5ba524695f6569f293
a1359d8e
~~~

# Analysis of Limits on AEAD_AES_128_CCM Usage {#ccm-bounds}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move this to an appendix? Ideally I would prefer for this to not be in the draft at all, and instead have a reference to a peer-reviewed paper, but I suspect that doesn't exist today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an appendix :)

And yeah, I'm not all that happy that this is not in a peer-reviewed paper, but TLS didn't have that either. The mitigating factors are that this is being carefully reviewed (my two screwups so far have been caught), and the math is fairly simple once you get past the layers of obfuscation.

martinthomson and others added 2 commits May 11, 2020 10:56
Co-authored-by: Christopher Wood <caw@heapingbits.net>
Co-authored-by: Felix Günther <mail@felixguenther.info>
Only require closing the connection if you can't update.

If you hit this limit, you can send a key update.  You won't be able to
read any packets until your peer reads that key update though.  This
manifests as a bunch of packet loss, because you threw out keys.  So you
do end up sending a bunch of packets into the dark in the hopes that one
will get through.

Of course, you can't always update, so you have to close then.
@martinthomson
Copy link
Member Author

In the interest of transparency, I have added one more commit here.

When I did the same for DTLS, ekr observed that if you have updated, you might safely stop trying to accept packets with the old keys rather than killing the connection. That results in loss for any packets that genuinely did want to use the exhausted keys, but that is probably less disruptive than having the connection drop. Of course, you can't always update, so closure is still likely necessary in some cases.

draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
Copy link
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the edits!

limits on TLS record size. The maximum size of a TLS record is 2^14 bytes.
In comparison, QUIC packets can be up to 2^16 bytes. However, it is
expected that QUIC packets will generally be smaller than TLS records.
Where packets might be larger than 2^14 bytes in length, smaller limits might
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really nice to do the math for 2^16 byte packets in this document, but I'm not volunteering so I guess I can live with this as is

@chris-wood
Copy link
Contributor

It would be really nice to do the math for 2^16 byte packets in this document, but I'm not volunteering so I guess I can live with this as is

For what it's worth, Martin, Felix, and I put together a document which might help users do this themselves:

https://github.com/chris-wood/draft-wood-cfrg-aead-limits

@DavidSchinazi
Copy link
Contributor

@chris-wood Could you use that to come up with 2^16 limits and put them in the doc? I’m really wary of letting implementors roll their own

@chris-wood
Copy link
Contributor

chris-wood commented May 23, 2020

@chris-wood Could you use that to come up with 2^16 limits and put them in the doc? I’m really wary of letting implementors roll their own

Sure! Can you please file an issue with that request? Noting anything else you'd like spelled out in detail would be helpful, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forgery limits on packet protection
8 participants