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

Consider simplifying Packet Number Encryption #1575

Closed
DavidSchinazi opened this Issue Jul 17, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@DavidSchinazi
Contributor

DavidSchinazi commented Jul 17, 2018

Currently PNE encrypts the variable-length packet number. There are at least two ways to implement decryption:

  1. decrypt the first byte in-place, compute the PN length, decrypt the remaining bytes in-place
  2. decrypt the max PN length (4 bytes) into a separate buffer, compute the PN length, copy that amount of bytes back

This has the downside of worse performance, or can cause timing attacks.

It would be simpler to always encrypt the max length - that way we can always decrypt 4 bytes in place.

Apologies if this has been discussed before, I wasn't able to find it.

@mikkelfj

This comment has been minimized.

Contributor

mikkelfj commented Jul 17, 2018

If you send very short messages such as PING you risk overlapping with the AEAD tag and then it starts to get interesting figuring out how to decrypt the packet number.

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Jul 17, 2018

I'm not sure I understand, can you elaborate? You just decrypt four bytes - having that flow into the tag is not a problem

@martinthomson

This comment has been minimized.

Member

martinthomson commented Jul 17, 2018

I think that this is purely editorial: the draft doesn't actually describe a process for decryption, just points at considerations. Are you suggesting that we recommend a specific method? I think that any of these options are fine.

@martinthomson martinthomson added the -tls label Jul 17, 2018

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Jul 17, 2018

Sorry, I meant that we could change the amount we encrypt - always encrypt four bytes instead of just the PN.

But thinking about this some more I understand @mikkelfj 's comment. If we encrypt the sample we'll have a hard time decrypting... Would it be reasonable to reduce the size of the sample for very short packets?

@martinthomson

This comment has been minimized.

Member

martinthomson commented Jul 17, 2018

@DavidSchinazi , I see. We shouldn't really reduce the sample size. That is, if you value the strength of the encryption. Either way, it shouldn't be necessary.

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Jul 17, 2018

I agree that this isn't necessary - I'm just wondering if this proposal could make implementations simpler and safer.

Proposal: always encrypt four bytes [pn_offset, pn_offset + 3] and always have the sample start at pn_offset + 4. If the sample were to overflow the payload + tag (which only happens if you use 128-bit AES and have pn_len + payload_len < 4), you just pad the sample with zeroes. In the worst case scenario the sample contains two bytes of zero but you still have a 128bit secret key. Is that unacceptable?

@martinthomson

This comment has been minimized.

Member

martinthomson commented Jul 17, 2018

I wouldn't say unacceptable, but it does lower the birthday bound.

@mnot

This comment has been minimized.

Member

mnot commented Sep 19, 2018

Discussed in NYC; @ekr to follow up with @DavidSchinazi within two weeks.

@kazuho

This comment has been minimized.

Member

kazuho commented Sep 19, 2018

I prefer the current approach.

IIUC, the proposal is to have a conditional branch that tests if the sum of PN length and payload is below 20 octets, then create a zero-padded copy for sampling.

I do not think that is simpler than the current approach that uses a conditional branch to determine the sampling offset.

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Sep 24, 2018

I had a conversation with @ekr about this yesterday, and I'm wondering if the following proposal might get us the best of both worlds:

  1. Require that QUIC packets MUST verify length(packet number) + length(payload) >= 4, by padding the payload if needed.
  2. Always have PNE encrypt 4 bytes from the start of the packet number, and use the following 16 bytes as nonce.

In practice the padding overhead here is very low as it only impacts a small number of frame types when sent without an ACK frame. (Worse case scenario is a lonely PING frame with packet number below 63, and that case suffers 2 bytes of padding out of a 20+8+1+1+1=31 byte IPv4 packet)

It also has the nice property of obfuscating the packet-number length, as an observer can no longer tell when the packet number is below 63.

In terms of implementation, this is simplest for decryption, and the padding can be added right before encrypting the payload as that code needs to have access to the packet number as nonce anyway.

What are people's thoughts on this?

@martinthomson

This comment has been minimized.

Member

martinthomson commented Sep 25, 2018

That sounds good. We can stipulate that a packet MUST always contain at least 3 octets of frames. Adding a couple of padding octets is probably a good idea.

Note that we're probably changing the encoding of packet numbers (again, yeah, sorry), so the precise set of

Not sure whether you mean that you always XOR 4 octets (which means XOR over the encrypted payload if the packet number is <4), or whether you only XOR the octets of the packet number. With the proposed move of the packet number length to the first octet, it's probably better to only XOR the octets of the packet number. That way you don't touch the payload octets other than to sample them.

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Sep 29, 2018

I think requiring 3 octets of frames is more restrictive that we need to be. I would prefer saying that QUIC packets MUST verify length(packet number) + length(payload) >= 4, and if implementors want to do that by always ensuring 3 bytes of frames that's fine.

I meant that we XOR over the payload if the packet number length is < 4. When we're encrypting the PN, I'm not sure the distinction between PN and payload is relevant - there's nothing wrong with changing the start of the payload in my mind. And it simplifies the implementation.

But we can put this on hold until we've landed on what the PN encoding looks like.

@kazuho

This comment has been minimized.

Member

kazuho commented Oct 4, 2018

I think requiring 3 octets of frames is more restrictive that we need to be. I would prefer saying that QUIC packets MUST verify length(packet number) + length(payload) >= 4, and if implementors want to do that by always ensuring 3 bytes of frames that's fine.

This sounds like a neat approach.

I meant that we XOR over the payload if the packet number length is < 4.

I am not sure if I like the idea. Because it might be an additional complexity to the hardware decoders. In the current design, payload is decrypted only once by an AEAD cipher. This change would mean that it can be decrypted twice, first by a CTR cipher and then by an AEAD cipher.

As @martinthomson points out, we discussed in New York about moving the PN length bits to the first octet. Assuming that we adopt that change, I think there would be no reason to prefer always CTR-decrypting 4 octets. The different is just having a different order between picking the specified length and applying CTR.

@DavidSchinazi

This comment has been minimized.

Contributor

DavidSchinazi commented Oct 4, 2018

Could someone please point me at the issue or PR for the PN length bits change proposed in New York, if it exists?

@kazuho

This comment has been minimized.

Member

kazuho commented Oct 4, 2018

martinthomson added a commit that referenced this issue Nov 21, 2018

Pad rather than shift the header protection offset
This simplifies a little, so it's a net win in my view.

Closes #1575.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment