illegal header name from httparse

[let4be]

Why do we have this defined as a panic?

hyper/src/proto/h1/role.rs

Line 36 in dbea771

Err(_) => panic!(

hyper/src/proto/h1/role.rs

Line 47 in dbea771

Err(_) => panic!("illegal header name from httparse: {:?}", $bytes),

hyper/src/proto/h1/role.rs

Line 60 in dbea771

Err(_) => panic!("illegal header value from httparse: {:?}", __hvb),

Even if under debug_assertions this library has no right to panic as it's completely normal for it to be used on invalid input(I use it in a broad web crawler and web is full of surprises - good and bad). It should always simply return an error back to the user if anything bad happened

as I understand it panics are reserved for unrecoverable errors, and because this is a library it has no right to panic and crash client code... Error "unrecoverability" is completely at library's user discretion

[let4be]

I'm also not quite sure how to disable this, I'm still getting
thread 'tokio-runtime-worker' panicked at 'illegal header name from httparse: [51, 48, 50, 10, 83, 116, 97, 116, 117, 115]', /home/dev/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/hyper-0.14.7/src/proto/h1/role.rs:951:28

after putting

[profile.dev]
panic = "abort"
debug-assertions = false

[profile.release]
panic = "abort"
debug-assertions = false
opt-level = 3

into my Cargo.toml and compiling using cargo build --release

any advice?

edit: it seems the library will panic unconditionally of debug_assertions option on bad input...
I just fork and nuke the check for now

[let4be]

It just seems you cannot simply replace external dependency by cloning it and specifying a local path, because there could be other packages depending on it and they will pull unmodified version => hence the conflict
So it's necessary to use this [patch.crates-io] section in the root...

Let's get back on track on what would be the best way to fix the panic issue in Hyper, right now I did some dirty fix in my fork but it's bad... I simply create "invalid_header" from a static string when it cannot parse it - enough for my use case, but we need a better solution for the library(most likely need to propogate error back to the user)

once again - unconditional panic on bad input in a library is very-very-very bad...

[sfackler]

Some actual code that reproduces the issue would be helpful.

I believe the logic panics because it expects the logic in the http crate to agree with the logic in the httparse crate with respect to valid syntax.

[SergioBenitez]

Some actual code that reproduces the issue would be helpful.

I believe the logic panics because it expects the logic in the http crate to agree with the logic in the httparse crate with respect to valid syntax.

The reproduction is in the panic message, which contains the header name sent:

thread 'tokio-runtime-worker' panicked at 'illegal header name from httparse: [51, 48, 50, 10, 83, 116, 97, 116, 117, 115]'

[sfackler]

Then it seems like this is a bug in httparse somehow producing a "header name" of 302\nStatus.

[let4be]

If this report is correct, this needs a CVE and an immediate fix. This is a trivial DoS vector with extreme ease of execution. I am stunned that this issue remains open and ignored.

I'm not sure this is applicable to hyper http server, I noticed this issue when using hyper's http client on a broad spectrum of inputs

[nox]

as I understand it panics are reserved for unrecoverable errors, and because this is a library it has no right to panic and crash client code... Error "unrecoverability" is completely at library's user discretion

It was not supposed to happen ever, that's why it's a panic.

[nox]

@let4be Could you give us some code to repro the issue? Maybe the HTTP message that was parsed? That would be helpful.

[seanmonstar]

To help understand what is triggering the problem, could you help us with a little more information? What version of rustc are you using? What version of hyper, http, and httparse do you find in your Cargo.lock? Do you know the full HTTP response (not the body) that the server is sending?

The reason I ask for the full HTTP response is that if I try to make some mock responses with that string as a header name, I just see a hyper::Error(Parse(Header)) result. So there must be something more to it than that.

The purpose of this panic is to notice if httparse and http differ in their understand of a valid header name, which could cause different problems if just allowed through. It could be that there's a better way to handle that, but I'd prefer to understand a reproducible cause before that.

[let4be]

rustc 1.51.0 (2fd73fabe 2021-03-23)

name = "httparse"
version = "1.4.0"
name = "http"
version = "0.2.4"

hyper is the latest version available - 0.14.7

I cannot provide an exact dump of http response, this was lost unfortunately and I'm running a locally fixed version right now that does not crash...

If we absolutely need this in order to fix an issue I may try to go back and run my crawler on unfixed version of hyper that panics(and probably it will "crash" eventually in a couple of hours of web browsing stumbling on this defective web site again)

[let4be]

mmm... I'm using hyper in async code, and it does not appear obvious how I can use catch_unwind in async
fast googling fails me...

use futures::FutureExt
+

if future_with_hyper_that_may_panic.catch_unwind().await.is_err() {
   panic!("hyper panic, offending url: {}", task.link.url)
}

does not seem to work, complaining about
the trait UnwindSafe is not implemented for dyn futures::Future<Output = ()> + std::marker::Send

I will try to dig more tomorrow

[MikailBag]

I believe you should be able to wrap future_with_hyper_that_may_panic into std::panic::AssertUnwindSafe

[let4be]

Been running for quite a while and finally it paniced again(after like 6h+ of running)... but my catch_unwind did not work for some reason

let u = task.link.url.clone();
let wtf = async {
    tokio::select! {
        _ = self.process_task(task, &client, &stats) => {}
        _ = timeout => {}
    }
};
if std::panic::AssertUnwindSafe(wtf).catch_unwind().await.is_err() {
    panic!("hyper panics, offending url: {:?}", u)
}

hyper is called a couple of levels deep in self.process_task

thread 'tokio-runtime-worker' panicked at 'illegal header name from httparse: [50, 48, 48, 10, 67, 111, 110, 116, 101, 110, 116, 45, 116, 121, 112, 101]', /home/dev/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/hyper-0.14.7/src/proto/h1/role.rs:951:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aborted (core dumped)

any advice?
I'm not entirely sure how panics/catch_unwind work in rust, I never use them and always handle errors via Result...

edit: nvm, my code was compiled with

[profile.release]
panic = "abort"
debug-assertions = false
opt-level = 3

rerunning with "unwind"

[let4be]

I think I got it...
offending URL: http://watson.addy.com/

this code reproduces the issue:

use anyhow::{Context};
use std::str::FromStr;
type Result<T> = anyhow::Result<T>;

async fn go() -> Result<()> {
    let mut http = hyper::client::HttpConnector::new();
    http.enforce_http(false);
    let https = hyper_tls::HttpsConnector::new_with_connector(http);
    let client = hyper::Client::builder().build::<_, hyper::Body>(https);

    let uri = hyper::Uri::from_str("http://watson.addy.com/").context("cannot create http uri")?;

    let mut req = hyper::Request::builder();
    req = req.uri(uri).method(hyper::Method::GET);

    let resp = client.request(req.body(hyper::Body::default()).unwrap()).await;
    let resp = resp.with_context(|| "cannot make http get")?;
    println!("status code: {}", resp.status());

    Ok(())
}

fn main() -> Result<()> {
    let rt = tokio::runtime::Runtime::new().unwrap();
    let future = go();
    rt.block_on(future)
}

thread 'tokio-runtime-worker' panicked at 'illegal header name from httparse: b"200\nContent-type"', /home/dev/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/hyper-0.14.7/src/proto/h1/role.rs:951:28

[malaire]

Checking http://watson.addy.com/ with curl, it uses 0x0a to separate header lines and 0x0a 0x0a to end headers which is of course incorrect.

Here's start of the response:

00000000  48 54 54 50 2f 31 2e 30  20 32 30 30 0a 43 6f 6e  |HTTP/1.0 200.Con|
00000010  74 65 6e 74 2d 74 79 70  65 3a 20 74 65 78 74 2f  |tent-type: text/|
00000020  68 74 6d 6c 0a 0a 3c 68  74 6d 6c 3e 0a 3c 68 65  |html..<html>.<he|
00000030  61 64 3e 0a 3c 73 74 79  6c 65 3e 0a 68 31 7b 0a  |ad>.<style>.h1{.|

[let4be]

Thing is we may as well feed complete binary garbage to hyper at any point and it still should handle this gracefully without panicing by returning Result back to the user

So I'm cautious there are panics throughout hyper's code...

[sfackler]

seanmonstar/httparse#96

[malaire]

seanmonstar/httparse#96

Should that be parsed into headers as it's not correct HTTP? I think that should return an error.

[let4be]

Probably there should be an option somewhere about how strict we want to follow http standard, and if we still can parse something malformed and this option is on(non_strict_http: true) we should make our best to parse

[seanmonstar]

Thing is we may as well feed complete binary garbage to hyper at any point and it still should handle this gracefully without panicing by returning Result back to the user. So I'm cautious there are panics throughout hyper's code...

Yes, hyper doesn't generally try to panic on garbage data from the network. As explained in an earlier comment, the panic is not that the bytes are bad, the panic is that two parts of hyper disagreed on the validity. You can see this panic as an assertion test of the internals.

[SergioBenitez]

Thing is we may as well feed complete binary garbage to hyper at any point and it still should handle this gracefully without panicing by returning Result back to the user. So I'm cautious there are panics throughout hyper's code...

Yes, hyper doesn't generally try to panic on garbage data from the network. As explained in an earlier comment, the panic is not that the bytes are bad, the panic is that two parts of hyper disagreed on the validity. You can see this panic as an assertion test of the internals.

Hyper shouldn't panic regardless. Any non-local assertion should be handled gracefully. That is, if you can determine from the immediate surrounding code that an assertion will never fail, then you might be okay, otherwise, handle it gracefully. There is no excuse for doing otherwise, especially in networked software.

[seanmonstar]

You can see this panic as an assertion test of the internals.

That said, I think we can change this to something better here. We still want to notice when the two parts disagree, and when under unit tests it probably should still panic, so we notice and fix it. And when not under test, since it's still a bug, it should log an error! message, and return a regular parse error.

[nox]

That said, I think we can change this to something better here.

I think it's best to keep the panic, personally. There are other cases where we found panics in h2 or hyper or whatever and we just fixed those.

Saying that it shouldn't panic ever is in my opinion a little bit grandstanding, as tokio will catch that at task boundary anyway.

[nox]

Checking http://watson.addy.com/ with curl, it uses 0x0a to separate header lines and 0x0a 0x0a to end headers which is of course incorrect.

Should that be parsed into headers as it's not correct HTTP? I think that should return an error.

Probably there should be an option somewhere about how strict we want to follow http standard, and if we still can parse something malformed and this option is on(non_strict_http: true) we should make our best to parse

All browsers and many HTTP libs support that, AFAIK most of them do so by default and don't provide a setting.

Note that httparse is already more lax than the RFCs, too.

[nox]

If we ever replace the panic by a returned error, please at least make that its own error kind and not a normal parse error, make it an error that literally states that this is a bug inside Hyper and/or its dependencies, that an invariant is broken and that this should be investigated ASAP.

[nox]

FWIW the bug is fixed in httparse 1.4.1.

[nox]

All browsers and many HTTP libs support that, AFAIK most of them do so by default and don't provide a setting.

I completely forgot about this but chatting with @Geal this weekend I was reminded that RFC 7230 actually says that LF may also be accepted in addition to CRLF:

Although the line terminator for the start-line and header fields is the sequence CRLF, a recipient MAY recognize a single LF as a line terminator and ignore any preceding CR.

So that's why httparse supports LF too (just writing this in case someone is curious about this stuff).

[SergioBenitez]

Saying that it shouldn't panic ever is in my opinion a little bit grandstanding, as tokio will catch that at task boundary anyway.

My saying that hyper should never panic isn't an attempt to gain favor, it's an assertion about what reliable networking software should do. A panic in hyper has disastrous effects, even in the client.

Consider the following request handler in some framework.

#[get("/")]
fn handler(db: Db, client: &Client) -> Result<()> {
    db.start_expensive_computation().await?;
    let value = client.get(Uri::from_static("http://some-url.org")).await.unwrap_or_default();
    db.end_expensive_computation(value).await?;

    Ok(())
}

What happens if some-url.org returns a (valid, if odd) response resembling the one highlighted in this issue? As we now know, the client.get() line will panic. This has the following implications:

1. The server will respond with an invalid, empty HTTP response, and abruptly terminate the connection.
2. The "expensive" computation is now logically invalid because it is never "ended", assuming the framework would have otherwise guaranteed execution beyond the third await.
3. The user now has to guard against an undocumented panic.
4. Depending on destructors, this could introduce a DoS attack vector.

And this isn't just the application author misplacing trust in some-url.org: in many settings (setting profile images, OAuth, etc.), the client can also choose the URL, and thus the response, triggering the panic. If the server stores the URL and subsequently makes requests to it later, this becomes yet another DoS attack vector.

This is bad, and treating it as anything but is unequivocally harmful.

I think it's best to keep the panic, personally. There are other cases where we found panics in h2 or hyper or whatever and we just fixed those.

If you continue to disagree and consider panics in hyper okay, then this fact should be documented so that users can make their own decisions about which libraries to use.

I am saddened by the handling of this issue. This demonstrates to me an unwillingness to take responsibility for security-sensitive bug reports. My comment indicating the sensitive nature of this bug report is now hidden and marked as "disruptive content," when I would consider it anything but. I restate it now, in hopes that it is not again silenced: "If this report is correct, this needs a CVE and an immediate fix. This is a trivial DoS vector with extreme ease of execution. I am stunned that this issue remains open and ignored."

[sfackler]

Just to make sure I understand, is your position that any bug that could result in a panic in any library that could conceivably be used in a webserver mandates the assignment of a CVE with a high severity score because it "introduces a trivial DoS vulnerability"?

[SergioBenitez]

Just to make sure I understand, is your position that any bug that could result in a panic in any library that could conceivably be used in a webserver mandates the assignment of a CVE with a high severity score because it "introduces a trivial DoS vulnerability"?

No, of course not. You've taken my words out of context. I've never stated that all panics "introduce a trivial DoS vulnerability" nor that they merit a higher severity score. There's nothing wrong, security-wise, with hyper panicking before it starts accepting requests, for instance, or in some recoverable book-keeping thread with no importance to serving or handling requests. Debatably, there might be nothing wrong with it panicking ever as long as it is clearly documented. However, if an undocumented panic can occur in the main serving loop and directly impact request serving, then yes, a CVE is merited for the reasons I've outlined above. The severity depends on if, and if so, how easily, the panic can be triggered accidentally or purposefully by the user of the library, external actors, or both. In this case, the panic is likely to occur in a main serving loop, was triggered accidentally, and can be triggered trivially by an external actor, making it particularly bad.

To clarify further, when I said "a panic in hyper" in my previous comments, I was implicitly qualifying it as "a panic in hyper [in a component likely to appear in a serving loop]."

[seanmonstar]

To wrap up, this specific issue (the parse error) is fixed in a new httparse release. You can get by running cargo update -p httparse.

The problem was that httparse introduced the ability to parse HTTP responses without a reason-phrase, but had a bug in how it handled those when only followed by LF. The status code, the newline, and the next header name would parsed as the first header. However, http::HeaderName would return an error that a newline is not valid. The LF-only case of a reason-phrase-less response is now handled the same way it would a response that did have a reason-phrase.

Because the original issue is fixed, I'm going to close this issue.

The other part that has been discussed in this issue is around the internal assertion panicking. I've opened #2546 to track changing that with something more appropriate.

[SergioBenitez]

@seanmonstar What is your stance on issuing a CVE? Do you understand the security implications of this bug?