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

The QUIC-TLS draft should define anti-forgery limits for packet lengths up to 2^16 #3701

Closed
DavidSchinazi opened this issue May 26, 2020 · 25 comments · Fixed by #4175
Closed
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus. ietf-lc An issue that was raised during IETF Last Call. proposal-ready An issue which has a proposal that is believed to be ready for a consensus call.

Comments

@DavidSchinazi
Copy link
Contributor

This issue is in response to @chris-wood 's #3620 (comment) .

PR #3620 introduces anti-forgery limits for packet lengths up to 2^14 as those were inherited from the maximum TLS record size. Since @chris-wood mentioned a way to compute limits for those sizes, we should use that and put the limit in the document, instead of leaving that as an exercise to the reader.

@chris-wood
Copy link
Contributor

@larseggert larseggert added this to Triage in Late Stage Processing via automation May 28, 2020
@martinthomson martinthomson added the editorial An issue that does not affect the design of the protocol; does not require consensus. label May 28, 2020
@project-bot project-bot bot moved this from Triage to Editorial Issues in Late Stage Processing May 28, 2020
@larseggert
Copy link
Member

Anyone care to prepare a PR for this one?

@larseggert
Copy link
Member

Without a PR, this will stay as is...

@DavidSchinazi
Copy link
Contributor Author

Leaving things as-is is unsafe, as the current document allows sending QUIC packets of size 2^15 without providing bounds required for the security properties promised by the protocol. If we don't have anyone willing to the analysis for 2^16 (I don't consider myself competent enough to get this right), then we should disallow sending or receiving QUIC packets longer than 2^14. Otherwise we're shipping a protocol that is not safe.

@martinthomson
Copy link
Member

So I talked this over with @chris-wood and we both agree that this is not unsafe enough to block publication based on:

Where packets might be larger than 2^14 bytes in length, smaller limits might be needed.

Getting this right is hard enough that I'm concerned that we'll make another mistake. The number of mistakes we made already is enough that I don't want to put this in a spec without multiple rounds of checking.

@MikeBishop points out that a change in targets also changes some MUST-level requirements and so wouldn't be a purely editorial change, so I'm going to remove the "editorial" tag and suggest that we just not take this.

@martinthomson martinthomson removed the editorial An issue that does not affect the design of the protocol; does not require consensus. label Sep 24, 2020
@LPardue LPardue added the future-version An issue that is best addressed in a future version of QUIC, or an extension to it. label Sep 24, 2020
@LPardue
Copy link
Member

LPardue commented Sep 24, 2020

Sounds like a nice to have that would benefit from more time and experience. Marking as "future-version" for later consideration but otherwise closing this to clear our landing path.

@LPardue LPardue closed this as completed Sep 24, 2020
Late Stage Processing automation moved this from Editorial Issues to Issue Handled Sep 24, 2020
@DavidSchinazi
Copy link
Contributor Author

Hi @LPardue @larseggert @mnot, did I miss a recent change to our process? Since when are we closing non-editorial issues without a call for consensus?

@ianswett
Copy link
Contributor

I believe the process says that design issues cannot be closed without a call for consensus and this was never marked as a design issue.

@mnot created a flowchart at some point, but I'm failing to find it.

Given the difficulty of deploying even 4k packets, I don't foresee this being a practical problem in the near future.

@DavidSchinazi
Copy link
Contributor Author

The flow chart is here. Closing an issue that's marked neither editorial nor design isn't a scenario that's allowed according to that diagram.

@ekr
Copy link
Collaborator

ekr commented Sep 25, 2020

I agree with David that this doesn't seem like the right resolution. I also agree with MT that we should not attempt to adjust the limits for bigger packets now.

Given Ian's comment I think the correct resolution is to simply forbid packets > 2^14. If/when someone does the analysis for larger packet sizes, that document can simply lift this restriction and replace it with whatever the new rules are. This is explicitly contemplates in S 6.6. "Future analyses and specifications MAY relax confidentiality or integrity limits for an AEAD."

Note that this would actually make the spec slightly shorter because you could remove the Note about how this applies to 2^14 not 2^16.

@ekr
Copy link
Collaborator

ekr commented Sep 25, 2020

Hmm.. Thinking about this a little more
We can certainly prohibit sending > 2^14 packets, but we if we prohibit receiving it, then we'll need an extension to let you send them. And if we don't, then the receiver will not be able to properly enforce the limits on 2^16. So, maybe not so easy...

@DavidSchinazi
Copy link
Contributor Author

We could solve this by slightly tweaking the definition of the max_udp_payload_size transport parameter.

OLD TEXT:

The default for this parameter is the maximum permitted UDP payload of 65527. Values below 1200 are invalid.

NEW TEXT:

The default for this parameter is 16384. Values below 1200 are invalid, values above 16384 are invalid.

If/when someone ever figures out anti-forgery limits for packet sizes above 16384, they can define an extension that negotiates that feature.

@ekr
Copy link
Collaborator

ekr commented Sep 25, 2020

I had forgotten about this parameter. I don't think we even need an extension. We say:

You MUST NOT send max_udp_payload_size > 16384. If you receive max_udp_payload_size > 16384 you MUST treat it as 16384. You MUST discard packets greater than max_udp_payload_size without attempting to process them.

Then if we figure out new anti-forgery limits we remove these MUSTs.

Only new implementations will send max_udp_payload_size > 16384 and only if both implementations are new will they send packets > 16384. That means that (1) sending implementations will enforce the right AEAD limits (2) only implementations which know about the new receive limits will offer max_udp_payload_size > 16384 and older implementations can just reject longer packets out of hand which means that they don't contribute to the forgery limit.

@DavidSchinazi
Copy link
Contributor Author

That sounds good to me.

@larseggert
Copy link
Member

I wold like more people to take a look at this. To me personally, this seems like a simple enough change, but it is a design change...

Since we really want to start IETF LC today (because the longer we delay, the more editorial issues are being opened again, perpetuating the cycle), I propose we convert this issue to an IETF LC issue and resolve it together with anything else that gets raised during that.

@ianswett
Copy link
Contributor

Reducing the max packet size in QUIC v1 is fine by me, given I don't expect to exceed 16384 bytes.

@DavidSchinazi
Copy link
Contributor Author

@larseggert making this an IETF LC issue is fine if you prefer it that way. Can you please reopen this issue and replace the "future-version" GitHub tag with "ietf-lc"?

@larseggert larseggert reopened this Sep 25, 2020
Late Stage Processing automation moved this from Issue Handled to Triage Sep 25, 2020
@larseggert larseggert added ietf-lc An issue that was raised during IETF Last Call. and removed future-version An issue that is best addressed in a future version of QUIC, or an extension to it. labels Sep 25, 2020
@larseggert
Copy link
Member

Done. I hope we can resolve this quickly.

@DavidSchinazi
Copy link
Contributor Author

Thanks. I hope so as well. Would you like me to send a PR or should I wait for IETF LC to start?

@larseggert
Copy link
Member

Let's wait for some more comments on the issue. The editors were swamped.

@martinthomson
Copy link
Member

After thinking about this a lot, I prefer not to do anything. For several reasons, but mostly because I think that a mechanical protection is an overreaction to this situation.

The target limits are arbitrary. The degradation here is at worst an increase in attacker advantage from 2^-57 to 2^-53. That's for CCM. The GCM and Poly1305 numbers scale linearly, so the change is to 2^-55.

I could see a way to sharpen the text, but mandating use of the UDP size limit to do this seems unnecessary. Suggesting that usage - for the paranoid - would be OK.

@DavidSchinazi
Copy link
Contributor Author

If you're fairly confident in your analysis that 2^16 byte packets are safe given certain bounds, can we instead put those bounds in the spec? If you're not confident in said bounds, then I'm uncomfortable shipping a protocol whose security bounds we are not confident in. At the end of the day, either we know the bounds or we don't, I'm not understanding what a middle-ground looks like here.

@ekr
Copy link
Collaborator

ekr commented Sep 29, 2020

I think I am with David here.

Specifically: I think the document should provide a traffic limit and an analysis that those limits provide security at least at a given level. It seems like we have three choices (1) restrict the packet size (2) lower the traffic limits (3) provide an analysis with a weaker guarantee, given that as MT points out, the guarantee is somewhat arbitrary

martinthomson added a commit that referenced this issue Oct 2, 2020
The analysis describes conditions where higher limits might be allowed,
and provides a formula in each case that can be used to determine other
limits more readily.

The number of changes I've made here are somewhat frightening.  This is
why I resisted doing this.  This will need careful double-checking.

The actual numbers don't concern me much, it's the changes to the
structure that need to be looked at.  Some of the changes are editorial,
but they still need extra eyes.

Closes #3701.
martinthomson added a commit that referenced this issue Oct 2, 2020
The analysis describes conditions where higher limits might be allowed,
and provides a formula in each case that can be used to determine other
limits more readily.

The number of changes I've made here are somewhat frightening.  This is
why I resisted doing this.  This will need careful double-checking.

The actual numbers don't concern me much, it's the changes to the
structure that need to be looked at.  Some of the changes are editorial,
but they still need extra eyes.

Closes #3701.
martinthomson added a commit that referenced this issue Oct 2, 2020
The analysis describes conditions where higher limits might be allowed,
and provides a formula in each case that can be used to determine other
limits more readily.

The number of changes I've made here are somewhat frightening.  This is
why I resisted doing this.  This will need careful double-checking.

The actual numbers don't concern me much, it's the changes to the
structure that need to be looked at.  Some of the changes are editorial,
but they still need extra eyes.

Closes #3701.
@larseggert
Copy link
Member

Labeling this as "design" based on #4175

@larseggert larseggert added the design An issue that affects the design of the protocol; resolution requires consensus. label Oct 15, 2020
@project-bot project-bot bot moved this from Triage to Design Issues in Late Stage Processing Oct 15, 2020
@larseggert larseggert moved this from Design Issues to Consensus Emerging in Late Stage Processing Oct 15, 2020
@larseggert larseggert added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Oct 15, 2020
Late Stage Processing automation moved this from Consensus Emerging to Issue Handled Oct 15, 2020
@larseggert larseggert moved this from Issue Handled to Consensus Emerging in Late Stage Processing Oct 16, 2020
@larseggert
Copy link
Member

I am moving this back to "Consensus Emerging" so we don't forget to call it out during the IETF LC.

@larseggert larseggert moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Oct 21, 2020
@larseggert larseggert moved this from Consensus Call issued to Issue Handled in Late Stage Processing Nov 20, 2020
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. ietf-lc An issue that was raised during IETF Last Call. proposal-ready An issue which has a proposal that is believed to be ready for a consensus call.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

7 participants