Skip to content

Harden and Enforce Data Read Limits #1325

@SergioBenitez

Description

@SergioBenitez

Reading from a DataStream should always be restricted to a given limit. While all of Rocket's built-in and contrib data guards enforce a limit, Rocket doesn't require enforcing such a limit, allowing incorrect user code to be written. We should prevent this by requiring, via the type system, data to be read with a limit. A possible re-worked API for Data and DataStream may be:

impl Data {
    fn open(limit: usize) -> DataStream;
    fn peek(&self) -> &[u8];
    fn peek_complete(&self) -> bool;
}

impl DataStream {
    fn stream_to<W: Write>(self, writer: &mut W) -> io::Result<u64>;
    fn stream_to_file<P: AsRef<Path>>(self, path: P) -> io::Result<u64>;
    fn stream_to_string(self) -> io::Result<String>;
    fn stream_to_vec(self) -> io::Result<Vec<u8>>;
}

impl Read for DataStream { /* ... */ }

We might also consider abusing unsafe by adding an open_unsafe() method that sets no limit on the returned DataStream for cases we can't yet foresee.

Another thing to consider is that limit is not a sufficient...limit...to prevent DoS attacks. While it helps mitigates memory-exhaustion based attacks, it does nothing to prevent slow-loris style attacks. On sync, this means completely tying up resources, though there we have a read-timeout, which helps but doesn't solve the problem. On async, this translates to a bunch of idling futures, which in-turn means consuming memory, which takes us right back to memory-exhaustion.

What we'd really like to do is expose an API that requires limits to be set on several properties, with a hard, indelible limit provided by Rocket. In particular:

  • read timeouts - how long we we're willing to wait between byte reads
  • data limits - how many bytes we're willing to read in all, irrespective of time
  • connection timeouts - how long we're willing to keep the connection open, irrespective of whether data is being received or not.

My take is that a bandwidth-minimum approach to the connection timeout might actually make a bit more sense, especially when we consider very purposefully long-lived connections. That is, after a chosen period of time, assuming the other timeouts/limits haven't been exceeded, require that the bandwidth over a sliding window of time exceeds some factor. Otherwise, kill the connection, ideally in a graceful manner.

This approach seems fairly easy to implement in async. Combined with the API I proposed above, this should significantly decrease the opportunity for DoS based attacks on Rocket applications, and in the common case, make them impossible.

This was largely ported over from #990.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementA minor feature request

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions