rustls / rustls

A modern TLS library in Rust
Other
6.22k stars 648 forks source link

QUIC transport parameters should not be relied upon before handshake is complete #655

Open djc opened 3 years ago

djc commented 3 years ago

See https://tools.ietf.org/html/draft-ietf-quic-tls-32#section-8.2:

While the transport parameters are technically available prior to the completion of the handshake, they cannot be fully trusted until the handshake completes, and reliance on them should be minimized. However, any tampering with the parameters will cause the handshake to fail.

djc commented 3 years ago

One change I contemplated was to just return None from get_quic_transport_parameters() while Connection::is_handshaking(), but I think we should validate the impact on Quinn code if we don't get early access to the parameters.

Ralith commented 3 years ago

Having these available early is required to support 0.5-RTT communication from the server, which is currently implemented in Quinn.

Ralith commented 3 years ago

That's for the server, to be clear. For the client, we require immediate access to the transport parameters saved from the previous connection in the session cache for 0-RTT to work.

djc commented 3 years ago

If the transport parameters come from the cache, then supposedly they have been authenticated at some point, right? So that would probably still be okay.

Ralith commented 3 years ago

Yeah, they shouldn't be put into the cache by rustls unless the handshake succeeded. I don't think they can even be delivered before then, though I'm less of an expert on the TLS state machine.

briansmith commented 2 years ago

@djc @Ralith Any chance you are going to fix this soon? It seems to me like it would be ideal for the transport parameters to be passed through the handshake state machine, and then saved in ExpectQuicTraffic. Then the public accessor from the would only be able to access them if the current state is ExpectQuicTraffic.

Ralith commented 2 years ago

I don't have plans to work on this in the near future.

Then the public accessor from the would only be able to access them if the current state is ExpectQuicTraffic.

Note that it must be possible to access previously saved parameters from 0/0.5-RTT prior to this state.

briansmith commented 2 years ago

Note that it must be possible to access previously saved parameters from 0/0.5-RTT prior to this state.

Maybe there should be a separate API for this that makes it clear that these are the previous-saved transport parameters that are only to be used for the purpose of doing 0/0.5-RTT, probably by exposing a WriteEarlyData::early_data_transport_params() function, or similar?

Ralith commented 2 years ago

That sounds reasonable for the client. For the server (0.5-RTT) I think we actually need the just-transmitted parameters, though.

djc commented 2 years ago

I also don't have plans to work on this.

briansmith commented 2 years ago

For the server (0.5-RTT) I think we actually need the just-transmitted parameters, though.

I think we should verify that that is really the case. It doesn't seem safe to use the just-transmitted parameters until we've processed the Finished message.

briansmith commented 2 years ago

For the server (0.5-RTT) I think we actually need the just-transmitted parameters, though.

I think we should verify that that is really the case. It doesn't seem safe to use the just-transmitted parameters until we've processed the Finished message.

If I'm understanding you correctly, and if I'm understanding this part of the spec correctly, using the just-transmitted values is the wrong thing to do. Instead we must use our saved/cached values until the handshake is finished. See This is covered in https://datatracker.ietf.org/doc/html/rfc9000#section-7.4.1.

Ralith commented 2 years ago

§7.4.1 discusses the transport parameters sent by the server to the client:

To enable 0-RTT, endpoints store the values of the server transport parameters with any session tickets it receives on the connection.

However, for 0.5-RTT, we also require the transport parameters sent by the client to the server. I don't believe these are cached. It's also not clear to me that this is any less safe than using 0.5-RTT in general.

I'll pursue this in the QUIC slack for further clarity.

briansmith commented 2 years ago

Here's what the QUIC spec says about the client side:

To enable 0-RTT, endpoints store the values of the server transport parameters with any session tickets it receives on the connection. The values of stored transport parameters are used when attempting 0-RTT using the session tickets. Remembered transport parameters apply to the new connection until the handshake completes and the client starts sending 1-RTT packets. Not all transport parameters are remembered, as some do not apply to future connections or they have no effect on the use of 0-RTT.

The following is how I would translate this into code, so that Rustls can enforce the security invariants mentioned:

When saving the transport parameters in the session, Rustls should:

When restoring the transport parameters from the session for a client, Rustls would expose the transport parameters to the QUIC implementation through a new method in WriteEarlyData:

/// The QUIC transport parameters to use for sending early data.
///
/// The sequence of transport parameters has been filtered so that it will only contain the
/// parameters known to be relevant and safe for 0-RTT.
/// These transport parameters must only be used for sending 0-RTT data.
fn quic_transport_parameters(&'a self) -> impl Iterator<Item=TransportParameter<'a>> {
     // 1. Parse the serialized transport parameters.
     // 2. Filter out any that are not known to be safe for 0-RTT. This is a security requirement.
     // Filtering here ensures that any old cache values saved using a different (wrong) allowlist are
     // still safe to consume.
     // Returning an interator ensures we don't have to do allocations when parsing and filtering.
}

/// A QUIC transport parameter.
struct TransportParameter<'a> {
    id: <some integer type that makes sense>,
    value: &'a [u8],
};
briansmith commented 2 years ago

Regarding 0.5-RTT data, I filed a separate issue because I think we're perhaps not doing it correctly/safely in general: https://github.com/rustls/rustls/issues/944. AFAICT, it shouldn't be treated any differently than 0-RTT data, from the sender's perspective.

Ralith commented 2 years ago

Discard any with an ID that isn't that aren't explicitly known to be relevant and safe for 0-RTT (allowlist approach; new 0-RTT transport parameters would require updating Rustls to expand the allowlist). This is important for minimizing memory usage.

I'm concerned that this would make it much more difficult for downstream QUIC implementations to implement new or experimental QUIC extensions, requiring toilsome rustls updates and/or forks. What are the memory usage benefits you see here?

Regarding 0.5-RTT data

Are you convinced that it is correct for a server to use the client's current transport parameters for sending 0.5-RTT data? Discussion on slack suggests this is at least the consensus of existing implementations. Totally agree that 0.5-RTT should be properly signposted in application-facing APIs.

briansmith commented 2 years ago

Discard any with an ID that isn't that aren't explicitly known to be relevant and safe for 0-RTT (allowlist approach; new 0-RTT transport parameters would require updating Rustls to expand the allowlist). This is important for minimizing memory usage.

I'm concerned that this would make it much more difficult for downstream QUIC implementations to implement new or experimental QUIC extensions, requiring toilsome rustls updates and/or forks.

Perhaps we could change the API so that it takes an dangerous_additional_experimental_pre_auth_parameters: Option<&[TransportParameterId]> parameter to facilitate that, or add a separate function dangerous_quic_transport_parameters that takes that extra parameter.

What are the memory usage benefits you see here?

I guess I was being pessimistic about how the transport parameter mechanism might be extended and abused. It seems like all current QUIC transport parameters are tiny and I think we don't need to be so pessimistic. OTOH, it should be easy to enumerate which QUIC transport parameters are relevant to 0-RTT data and compare that to the size of the full set, and then make a decision based on actual information.

Regarding 0.5-RTT data

Are you convinced that it is correct for a server to use the client's current transport parameters for sending 0.5-RTT data? Discussion on slack suggests this is at least the consensus of existing implementations. Totally agree that 0.5-RTT should be properly signposted in application-facing APIs.

My understanding from reading the specs is that "0.5-RTT data" is just a special case of 1-RTT data and so all the requirements for 1-RTT data apply. In particular, the same QUIC transport parameters that would be used for 1-RTT data need to be used, which seems to be the as-yet-unauthenticated parameters from the ClientHello. However, I also think that maybe the QUIC spec is underspecifying what to do here. Compare how explicit and clear the requirements are for what clients are supposed to do, compared to what the server is supposed to do. Perhaps an erratum is in order.

Ralith commented 2 years ago

Perhaps we could change the API so that [...]

This seems like excessive complexity to me, considering the small population of QUIC implementers, and the assorted other verifications that must be performed regardless.

I guess I was being pessimistic about how the transport parameter mechanism might be extended and abused.

Endpoints have considerable leeway to abort connections that request unreasonable amounts of buffering for any handshake data (see Cryptographic Message Buffering), so I'm not sure how large a threat this is.

OTOH, it should be easy to enumerate which QUIC transport parameters are relevant to 0-RTT data and compare that to the size of the full set, and then make a decision based on actual information.

Right, except that this set might be freely extended in the future.

In particular, the same QUIC transport parameters that would be used for 1-RTT data need to be used, which seems to be the as-yet-unauthenticated parameters from the ClientHello

Right, that's consistent with my understanding.

However, I also think that maybe the QUIC spec is underspecifying what to do here.

It's certainly not stated directly anywhere. I don't think any implementer has found it confusing in practice, but the wg is very receptive to discussion in github issues if you're motivated to pursue an editorial clarification.

briansmith commented 2 years ago

Perhaps we could change the API so that [...]

This seems like excessive complexity to me, considering the small population of QUIC implementers, and the assorted other verifications that must be performed regardless.

I agree that it might be overkill. OTOH, look at it from the perspective of the implementer of the TLS library: our goal (AFAICT) is to provide the safest, most correct public API (and implementation of that API) that we can. This complexity falls out of that. Otherwise, we could just return a *mut c_void to the QUIC implementation :)

OTOH, it should be easy to enumerate which QUIC transport parameters are relevant to 0-RTT data and compare that to the size of the full set, and then make a decision based on actual information.

Right, except that this set might be freely extended in the future.

I think we're agreeing we don't need to try to do the filtering during the storage. If we find a way to make things more efficient in the future, then we could change the format of the data we store in some future version without too much trouble.