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

First byte changes #2006

Merged
merged 13 commits into from
Nov 20, 2018
Merged

First byte changes #2006

merged 13 commits into from
Nov 20, 2018

Conversation

martinthomson
Copy link
Member

This is a first draft of the changes that we discussed, first in NYC, then confirmed in Bangkok.

I had to recast "packet number protection" as header protection, but I took the opportunity to expand the text and organize it some more. I'm not especially happy with the picture, and could probably drop that, but I think that the changes clarify the process more.

The changes are all exactly as discussed, except that I ended up moving the ODCIL field from the Retry packet, which conveniently fit into four bits that I was struggling to describe what to do with. The low four bits in Retry can't be protected because Retry packets aren't protected. This seemed like a neat way of handling that.

I'm not certain that these are the only changes needed, but I think that this is essentially complete. It's good to get everything nailed down finally.

This doesn't include the -tls draft pieces yet, which cover how the
first octet is (partially) protected.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Nov 15, 2018
Fixed Bit:

: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we say that those packets are supposed to be dropped?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann I believe there a general section on dropping invalid handshake packets. But the following paragraphs says to raise a protocol violation - which only makes sense if the packet is authenticated. Either way, I think this paragraph needs to be consistent with the other fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I would say that receipt of zero here should be treated as a connection error of type PROTOCOL_VIOLATION.

Copy link
Contributor

Choose a reason for hiding this comment

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

@janaiyengar: I was thinking of a fast-path processing here: I can check this bit without decrypting the packet, so if I receive a packet with 0x40 == 0, I know it's some UDP garbage, and I can drop it on the floor immediately.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we have two choices:

  • drop the apparently broken packet without even decrypting it
  • decrypt and verify the AEAD tag of the apparently broken packet, then close the connection with PROTOCOL_VIOLATION

I tend to agree what @marten-seemann that we do not need to decrypt in this case. Let's handle it the same way as handling a packet with a broken AEAD tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite the same way. This can be detected using public information, so you don't need to run a constant-time decryption on that packet.

@@ -3353,8 +3354,8 @@ following sections.
The end of the packet is determined by the Length field. The Length field
covers both the Packet Number and Payload fields, both of which are
confidentiality protected and initially of unknown length. The size of the
Payload field is learned once the packet number protection is removed. The
Length field enables packet coalescing ({{packet-coalesce}}).
Payload field is learned once the header protection is removed. The Length
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that? The Payload Length is not encrypted, is it?

Copy link
Member

Choose a reason for hiding this comment

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

What is not encrypted is the "Length" field that represents the sum of the packet number size and payload size.

The length of the payload is revealed by obtaining the size of the packet number field by removing the header protection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Apparently I completely misunderstand this sentence, and the source of the confusion is that the payload is call "Payload field".

Suggested change:
"The length of the payload is learned once the header protection is removed."


Third Bit:
: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

... same error here.

draft-ietf-quic-tls.md Show resolved Hide resolved
\[\[Editor's Note: this section should be removed and the bit definitions
changed before this draft goes to the IESG.]]
: The sixth bit (0x20) of byte 0 is the Latency Spin Bit, set as described in
{{!SPIN=I-D.ietf-quic-spin-exp}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to say something about peers that are not spinning?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave that to the other doc.

@@ -3677,7 +3666,7 @@ wishes to perform a stateless retry (see {{validate-handshake}}).
0 1 2 3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+
|1| 0x7e |
|1|1| 3 |ODCIL(4|
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of weird to have the ODCIL length field so far before the ODCIL. On the other hand, it saves a byte, and fills unused bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Saving a byte is neither here nor there, but the two blocks of four unused bits was a problem. I agree, it's odd :)

{{QUIC-TLS}}). The value included prior to protection MUST be set to 0. An
endpoint MUST treat receipt of a packet that has a non-zero value for these
bits after removing protection as a connection error of type
PROTOCOL_VIOLATION.
Copy link
Contributor

Choose a reason for hiding this comment

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

This only applies to packets that are successfully encrypted. Should we mention that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann You mean decrypted/authenticated?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann : did you mean "after successfully removing header protection"? I don't think you need to wait to remove packet / payload protection before doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion. I meant "removing packet protection" (not header protection). The reason is that header protection is not authenticated, so an attacker can send a packet, and we'll happily remove header protection (and receive some garbage values). Only when we try to remove packet protection will we discover that this packet was not sent by the peer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. @martinthomson it might be worth clarifying that this is "packet protection" (see nomenclature note above)

@marten-seemann
Copy link
Contributor

IIUC, this PR breaks key updates, doesn't it? Since the Key Phase bit is encrypted, an endpoint won't be able to notice that a key update happened, and try the wrong key for unprotecting the header.

@kazuho
Copy link
Member

kazuho commented Nov 15, 2018

@marten-seemann

IIUC, this PR breaks key updates, doesn't it? Since the Key Phase bit is encrypted, an endpoint won't be able to notice that a key update happened, and try the wrong key for unprotecting the header.

I am not sure if that is true, because header protection key remains consistent between Key Updates.

({{pn-encrypt}}). The unprotected packet number is part of the associated data
(A). When removing packet protection, an endpoint first removes the protection
from the packet number.
Packets are protected prior to applying header protection ({{header-protect}}).
Copy link
Member

Choose a reason for hiding this comment

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

I think that you might want to call the AEAD process "payload protection", rather than referring to it like "packets are protected". IMO doing so would better clarify that we have two encryption: one that encrypts the payload and one that encrypts the header.

I understand that such change puts burden on the editors, so just saying.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. I think this clarity will help.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this, but the fact is that we also protect the header, so payload is equally misleading, perhaps more so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh. Naming. So how about two nouns: "packet protection" and "header protection". They're both imperfect, but at least accurate. I would then eschew usages like "Packets are protected" in favor of "Packet protection is applied".

@kazuho
Copy link
Member

kazuho commented Nov 15, 2018

I share the view that the design is consistent with what we have discussed in New York. Thank you Martin for making this happen.

({{pn-encrypt}}). The unprotected packet number is part of the associated data
(A). When removing packet protection, an endpoint first removes the protection
from the packet number.
Packets are protected prior to applying header protection ({{header-protect}}).
Copy link
Contributor

Choose a reason for hiding this comment

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

+1. I think this clarity will help.

draft-ietf-quic-tls.md Outdated Show resolved Hide resolved
@@ -3050,6 +3050,7 @@ IP addresses has fallen below 1280 bytes, it MUST immediately cease sending QUIC
packets on the affected path. This could result in termination of the
connection if an alternative path cannot be found.


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Fixed Bit:

: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, I would say that receipt of zero here should be treated as a connection error of type PROTOCOL_VIOLATION.


Third Bit:
: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Contributor

Choose a reason for hiding this comment

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

... same error here.

\[\[Editor's Note: this section should be removed and the bit definitions
changed before this draft goes to the IESG.]]
: The sixth bit (0x20) of byte 0 is the Latency Spin Bit, set as described in
{{!SPIN=I-D.ietf-quic-spin-exp}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave that to the other doc.

{{QUIC-TLS}}). The value included prior to protection MUST be set to 0. An
endpoint MUST treat receipt of a packet that has a non-zero value for these
bits after removing protection as a connection error of type
PROTOCOL_VIOLATION.
Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann : did you mean "after successfully removing header protection"? I don't think you need to wait to remove packet / payload protection before doing this.

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
draft-ietf-quic-tls.md Show resolved Hide resolved
AEAD_CHACHA20_POLY1305 ({{!CHACHA=RFC8439}}).
To ensure that this process does not sample the packet number, header protection
algorithms MUST NOT require sample that is longer than the minimum expansion of
the corresponding AEAD.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, it was earlier statet that the mask has length 5, but here is seems to depend on PN length.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm mixing up the block size input to the encryption with the extracted mask output. The input is always fixed size but depends on AEAD.

AEAD_AES_256_CCM (all AES AEADs are defined in {{!AEAD=RFC5116}}), and
AEAD_CHACHA20_POLY1305 ({{!CHACHA=RFC8439}}).
To ensure that this process does not sample the packet number, header protection
algorithms MUST NOT require sample that is longer than the minimum expansion of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
algorithms MUST NOT require sample that is longer than the minimum expansion of
algorithms MUST NOT require a sample that is longer than the minimum expansion of

ciphertext sample reveals the exclusive OR of the protected fields. Assuming
that the AEAD acts as a PRF, if L bits are sampled, the odds of two ciphertext
samples being identical approach 2^(-L/2), that is, the birthday bound. For the
algorithms described in this document, that probability is one in 2^64.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this probality still true for PN's shorter than 4 bytes? Or even at all given that the mask is at most only 36 or 37 bits effectively?

Copy link
Contributor

Choose a reason for hiding this comment

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

I forget that the sample is a block size input to a encryption step.

Fixed Bit:

: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann I believe there a general section on dropping invalid handshake packets. But the following paragraphs says to raise a protocol violation - which only makes sense if the packet is authenticated. Either way, I think this paragraph needs to be consistent with the other fields.

{{QUIC-TLS}}). The value included prior to protection MUST be set to 0. An
endpoint MUST treat receipt of a packet that has a non-zero value for these
bits after removing protection as a connection error of type
PROTOCOL_VIOLATION.
Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann You mean decrypted/authenticated?

@mikkelfj
Copy link
Contributor

There is some overlap between Jana's and my comments as they were submitted at the same time.

@@ -850,55 +939,45 @@ if packet_type == Initial:
len(token)
~~~

Copy link
Contributor

Choose a reason for hiding this comment

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

Benjamin Saunders pointed out on Slack that the long header pseudo code does not check for out of bounds condition when the packet is small.

Copy link

Choose a reason for hiding this comment

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

Yes; as an implementer (and the reason Benjamin asked about this) I looked at the example code more than the surrounding prose, and incorrectly assumed the offset adjustment only applies for short header packets. It would be nice to clarify this somehow, either by duplicating that bit of code for the long header example code, or by separating it more from the short header example code to clarify that it applies to both.

(pn_length) is determined.

~~~
mask = header_protection(hp_key, sample)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this pseudo-code define how sample is computed first? The spec only defines it further down (in #hp-sample) so we may want to reorder the pseudo-code to somewhere after that

The same number of bytes are always sampled, but an allowance needs to be made
for the endpoint removing protection, which will not know the length of the
Packet Number field. In sampling the packet ciphertext, the Packet Number field
is assumed to be 4 bytes long (its maximum possible encoded length), unless
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have this PR, I'd like to reopen the discussion of PNE simplification I proposed a while back. Given the text in this PR, my proposal would be to require that QUIC packets MUST verify length(packet number) + length(payload) >= 4. Then you can remove this "unless" clause which simplifies both encryption and decryption. (Unlike my previous proposal, we would now only encrypt the packet number (not the start of the payload) since we get the packet number length from the first byte.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current proposal does not encrypt any payload - I think an earlier iteration did.

Copy link
Member

Choose a reason for hiding this comment

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

As pointed out by @huitema elsewhere, we might start seeing cipher-suites that use smaller-sized tags.

For example, the TLS_AES_128_CCM_8_SHA256 cipher-suite defined in TLS 1.3 uses a 64-bit tag, meaning that the shortest possible IV passed to the header protection would be just 72-bits.

Considering that, I think it might be safer to require senders to pad the payload so that there would be at least 128 bits of IV.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be inclined to disallow TLS_AES_128_CCM_8_SHA256 in QUIC but either way padding the payload is a viable solution

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the privacy implications of the header encryption are significant enough to require an IV longer than the AEAD tag, and if they are, choose an algorithm that works. Some applications outside of UDP might want really small fast packets for signalling, like ABS brakes or something.

BTW: I thought all tags were currently 16 bytes?

https://quicwg.org/base-drafts/draft-ietf-quic-tls.html#aead

All ciphersuites currently defined for TLS 1.3 - and therefore QUIC - have a 16-byte authentication tag and produce an output 16 bytes larger than their input.

Copy link
Member

Choose a reason for hiding this comment

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

@DavidSchinazi

I would be inclined to disallow TLS_AES_128_CCM_8_SHA256 in QUIC but either way padding the payload is a viable solution

Yeah my point is that I would rather avoid the possibility of discussing how QUIC should be adjusted to support cipher-suites like AES128-CCM8, and that requiring the encoder to pad is a good approach in that respect. Because people who wants to introduce that type of cipher can simply define a different constant for the right-hand side (see my comment above).

Copy link
Member Author

Choose a reason for hiding this comment

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

Please take this discussion to an issue. This will be lost here.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be #1575

Copy link
Contributor

Choose a reason for hiding this comment

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

This discussion belongs on #1575, and at any rate, I would not make this a reason for blocking this PR. I would argue that we get in what we've agreed on (which is this PR), and then continue discussion on simplification on #1575.

Copy link
Member

Choose a reason for hiding this comment

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

Agrees to @janaiyengar that this is a separate issue. FWIW, the relevant issues are #1575 (simplification) and #2019 (support for AES128-CCM8, or possibly other cipher-suites using shorter tags).

Note that these encodings are similar to those in {{integer-encoding}}, but
use different values.
Packet numbers in long and short packet headers are encoded on 1 to 4 octets.
The number of bits required to represent the packet number reduced by including
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this sentence. Did you mean "The number of bits required to represent the packet number is deduced by observing the two least significant bits of the first byte." or something completely different?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading further down I'm actually understanding the meaning now - perhaps we could rephrase to something like: "Since we have information about which previous packet numbers have been acknowledged, it is possible to only send some of the least significant bits of the packet number when they are sufficient to reconstruct the full packet number unambiguously."

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sentence really means, "compared to the previous draft, the number of bits required to encode the PN is reduced, because we steal 2 bits from the first octet." Which makes sense now as we are arguing about details, but really does not belong in the final spec. In the final spec, the encoding is what it is, and that's it.


Third Bit:
: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to note that forcing bit 0x40 to 1 is critical to Google's demultiplexing. We were using 0x08 (the "Google QUIC Demultiplexing Bit") but since 0x08 is now encrypted we'll be using this one. So please keep this at 1, at least for short headers. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Agreed: keeping 0x40 will mean that servers will be able to support pre-Q044 clients as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DavidSchinazi, we're not doing this for you, but I'm glad you're happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that, but I just wanted to let you know it was important to us (i.e. please let us know if you plan to change this)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @DavidSchinazi. This is important to us (@litespeedtech) as well. Please give a heads up should the plans for bit 0x40 change.

@kazuho
Copy link
Member

kazuho commented Nov 16, 2018

@martinthomson

I'm not certain that these are the only changes needed, but I think that this is essentially complete.

I think you also need to update the diagram of the Stateless Reset packet.

Review suggestions

Co-Authored-By: martinthomson <martin.thomson@gmail.com>
@marten-seemann
Copy link
Contributor

Editorial: Didn't we agree on "byte" instead of "octet"?

confidentiality protection separate from packet protection, as described in
Section 5.3 of {{QUIC-TLS}}. The length of the packet number field is encoded
Section 5.4 of {{QUIC-TLS}}. The length of the packet number field is encoded
in the plaintext packet number. See {{packet-encoding}} for details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that true? Isn't the length of the packet number field encoded in the first byte?

Copy link
Contributor

Choose a reason for hiding this comment

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

@huitema The Packet Number Length field is the last two bits of the first byte (see 2 paragraphs above).

Copy link
Member Author

@martinthomson martinthomson 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 feedback everyone.

({{pn-encrypt}}). The unprotected packet number is part of the associated data
(A). When removing packet protection, an endpoint first removes the protection
from the packet number.
Packets are protected prior to applying header protection ({{header-protect}}).
Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this, but the fact is that we also protect the header, so payload is equally misleading, perhaps more so.

draft-ietf-quic-tls.md Show resolved Hide resolved
draft-ietf-quic-tls.md Show resolved Hide resolved
The same number of bytes are always sampled, but an allowance needs to be made
for the endpoint removing protection, which will not know the length of the
Packet Number field. In sampling the packet ciphertext, the Packet Number field
is assumed to be 4 bytes long (its maximum possible encoded length), unless
Copy link
Member Author

Choose a reason for hiding this comment

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

Please take this discussion to an issue. This will be lost here.

Fixed Bit:

: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
for this bit are not valid packets in this version.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite the same way. This can be detected using public information, so you don't need to run a constant-time decryption on that packet.

{{QUIC-TLS}}). The value included prior to protection MUST be set to 0. An
endpoint MUST treat receipt of a packet that has a non-zero value for these
bits after removing protection as a connection error of type
PROTOCOL_VIOLATION.
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -3677,7 +3666,7 @@ wishes to perform a stateless retry (see {{validate-handshake}}).
0 1 2 3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+
|1| 0x7e |
|1|1| 3 |ODCIL(4|
Copy link
Member Author

Choose a reason for hiding this comment

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

Saving a byte is neither here nor there, but the two blocks of four unused bits was a problem. I agree, it's odd :)


Third Bit:
: The next bit (0x40) of byte 0 is set to 1. Packets containing a zero value
Copy link
Member Author

Choose a reason for hiding this comment

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

@DavidSchinazi, we're not doing this for you, but I'm glad you're happy.

@martinthomson martinthomson changed the title First octet changes First byte changes Nov 19, 2018
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.

As noted on #2006 (comment) (sorry for not making it a review comment), I think you need to change the definition of Stateless Reset.

It currently says:

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+
|0|K|1|1|0|0|0|0|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                      Random Bytes (160..)                   ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                                                               |

IIUC, this needs to be something like below (though, I am not sure if the spin bit needs to be random in this case).

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+
|0|1|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                      Random Bits (166..)                    ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                                                               |

The message consists of a header byte, followed by an arbitrary number of random
bytes, followed by a Stateless Reset Token.
The message consists of the first two fixed bits of the packet header, followed
by an arbitrary number of random bytes, followed by a Stateless Reset Token.
Copy link
Member

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 state like "followed by six (6) bits plus an arbitrary number of bytes in random"?

The fear I have with the current text is that it might imply that Stateless Reset Token starts at the 2 + 8n bit of the packet.

@kazuho kazuho mentioned this pull request Nov 19, 2018
: The fourth bit (0x10) of byte 0 is set to 1.
: The next two bits (those with a mask of 0x18) of byte 0 are reserved. These
bits are protected using header protection (see Section 5.4 of
{{QUIC-TLS}}). The value included prior to protection MUST be set to 0. An
Copy link
Contributor

Choose a reason for hiding this comment

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

What important purpose is served by requiring the value of these bits to be 0? This precludes the use of these bits for experimental purposes, like VEC and LOSS detection.

Before the use of these bits is standardized, the risk of middleboxes ossifying occasional experimental use of the bits seems less significant than the loss of opportunity to experiment.

Copy link
Contributor

Choose a reason for hiding this comment

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

They're set to 0 prior to protection, which means a middlebox can't use them anyway unless you share the header protection key with it.

Copy link
Contributor

@igorlord igorlord Nov 19, 2018

Choose a reason for hiding this comment

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

The header protection is an XOR with a mask. Setting these bits to a desired experimental value after the header protection is applied (after XOR) works ok, if the other endpoint did not require the bits to be 0 after the header protection is removed.

Hence the question -- why a hard requirement to have these bits 0 after removing header protection, if the endpoint does not use these bits for anything? In other words, why require the behavior that @huitema suggested below to be negotiated instead of being a default behavior? That is "reset the bits to 0 after removing header protection before AEAD decryption when receiving".

Copy link
Contributor

Choose a reason for hiding this comment

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

Because otherwise they become scratch space where a middlebox could place a persistent marking on the packets. If peers want to discard integrity checks on some portion of the data they send/receive, they can certainly do so -- but not unilaterally. By default, modification of the packet makes the packet invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely AEAD verification would prevent scratch spaces regardless of whether the value is checked or not?

But I can see that setting the value to 0 simplifies future extensions or experiments.

Copy link
Contributor

Choose a reason for hiding this comment

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

This conversation is getting carried away. @igorlord: If the bits can be anything, and if middleboxes start using those bits as scratch space, they're effectively ossified. Endpoints can't use them anymore, since middleboxes might stomp on them in the network. How do we counter that?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we counter that?

@janaiyengar as I wrote in an earlier comment, the fields are protected as authenticated data in the AEAD tag, so middleboxes cannot change the bits without getting the packet rejected. At best they can read the bits iff they are outside header protection, but they are not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikkelfj : facepalm Right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened issue #2022.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the spec is right. In the absence of standard, the use case for VEC or ERR in V1 cannot be "manage my network". De facto, it has to be "experiment with new algorithms that would let me manage my network better". And experimenters can in fact do that, if clients and servers cooperate.

@huitema
Copy link
Contributor

huitema commented Nov 19, 2018

Suppose that two parties negotiate experimentation with error detection, using either version negotiation or transport parameters. The only requirement would be to set the VEC or ERR bits after packet header protection when sending, and to reset them to zero just before AEAD decryption when receiving. You could specify that as part of the negotiation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport 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.