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.

Sign up for GitHub

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

Jump to bottom

Let server abort on post-Retry packet number reset #3990

Merged
merged 1 commit into from Sep 3, 2020
Merged

Let server abort on post-Retry packet number reset #3990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions draft-ietf-quic-transport.md
View file
Open in desktop
Expand Up @@ -4807,6 +4807,8 @@ responding to a Retry packet. However, the data sent in these packets could be
different than what was sent earlier. Sending these new packets with the same
packet number is likely to compromise the packet protection for those packets
because the same key and nonce could be used to protect different content.
A server MAY abort the connection if it detects that the client reset the
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're making a normative change, I'd argue for SHOULD

Copy link
Contributor

Choose a reason for hiding this comment

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

MAY or SHOULD abort if detects, but no requirement that it attempt to detect seems like a reasonable compromise.

Copy link
Contributor

Choose a reason for hiding this comment

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

MAY SGTM

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't object to this particular decision, but I would have assumed that you could always abort a connection whenever the peer violates a MUST. No?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is generally true, and this makes it more explicit without adding any requirement on the server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We certainly don't mention a general rule anywhere, but we can't stop an endpoint from enforcing MUSTs on its own.

Well, we can't stop them in the "no protocol police" sense, but we certainly can have a MUST-level rule that forbids it.

I think it's useful to explicitly call it out out here because this is the sort of place where we want to call explicit attention.

Why?

My general point is that text should do work. And if there is already a general rule permitting to you to error on MUST violations then why does this text need to be there? And working backward from that one might infer that no such rule exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

My argument for why here is because this PN reuse can be a security issue.

That said, you're right that it's not as principled as I would like it to be; there also isn't such a general rule, and you make a good point that it might be useful to have a general rule.

Given that we don't have such a rule at the moment, I would be happy to take this text in and evaluate this with other MAYs in the general sense if there's enough interest to do it. Would you mind filing a separate issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

Given that we don't have such a rule at the moment, I would be happy to take this text in and evaluate this with other MAYs in the general sense if there's enough interest to do it. Would you mind filing a separate issue for this?

I like Jana's suggestion. Can we take this PR and create another issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

My argument for why here is because this PN reuse can be a security issue.

I disagree with this statement. There's only a security issue if the client resets the 0-RTT packet number space, but the server wouldn't be able to detect that, since it wouldn't even bother to unprotect 0-RTT packets. So the only violation that can be detected here is the reuse of packet numbers in the Initial packet number space. Since the Initial keys are publicly known anyway, this can't be a security issue.

Copy link
Member

@kazuho kazuho Sep 2, 2020 •
edited

Choose a reason for hiding this comment

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

Generally speaking, detecting MUST violations for Handshake and ApplicationData packets, after decrypting them, is fine because it will always be about a misbehaving peer, and because we do not (yet) have existing implementations that do not follow the MUSTs.

However, using information that is not protected by AEAD is going to be dangerous, as it is a potential attack vector. For unprotected data (incl. Initial packets), a reader should not assume that a MUST on the sender side implies a "MAY detect" on the receiver side. As can be seen on the issue, we discussed if we might or might not want to have a MAY here, knowing that we have a MUST on the sender side.

To summarize, I tend to believe that having an explicit MAY here makes sense.

packet number.

A server acknowledges the use of a Retry packet for a connection using the
retry_source_connection_id transport parameter; see
Expand Down