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

Client GET requests with transfer-encoding are wrongly stripped #1925

Closed
seanmonstar opened this issue Sep 4, 2019 · 2 comments
Closed

Client GET requests with transfer-encoding are wrongly stripped #1925

seanmonstar opened this issue Sep 4, 2019 · 2 comments
Assignees
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@seanmonstar
Copy link
Member

The client wrongly strips transfer-encoding: chunked from GET requests, thinking that GET requests shouldn't have payloads. However, that's not explicitly true:

A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

The original implementation was trying to protect from empty Body::wrap_stream(some_empty_stream)s automatically adding transfer-encoding: chunked to a GET request.

The fix should probably still protect against that, but if the transfer-encoding header has been explicitly set on the Request, it should be forwarded as-is.

@seanmonstar seanmonstar added A-http1 Area: HTTP/1 specific. E-easy Effort: easy. A task that would be a great starting point for a new contributor. C-bug Category: bug. Something is wrong. This is bad! labels Sep 4, 2019
@seanmonstar seanmonstar self-assigned this Sep 4, 2019
Demi-Marie added a commit to paritytech/substrate that referenced this issue Sep 26, 2019
without adding a Transfer-Encoding or Content-Length header.

This has always been wrong, but hyperium/hyper#1925 hid the bug until
hyper was upgraded to 0.12.35.
Demi-Marie added a commit to paritytech/substrate that referenced this issue Oct 2, 2019
* Update all dependencies

* Upgrade dependencies whenever “easy”

“easy” means that there are no major changes required.

* Fix build and bump paste dependency to 0.1.6

* Remove dead code

* Re-add = dependency for futures-preview

* Add missing std features for runtime-io

* Remove git dependencies

as updated versions have been published to crates.io

* try to debug bug

* For sr-io, "std" should imply "no_oom" and "no_panic_handler".

Otherwise, rustc complains (correctly) about duplicate lang items.

* Add missing "runtime-io/std" features

* Fix compilation errors

* Prevent duplicate lang items

Rust does not allow duplicate lang items.  When compiled without the
`std` feature, `sr-io` defines two lang items.  Therefore, `sr-io`
compiled without `feature = "std"` must not be linked with `std`.

However, `pwasm-utils` and `wasmi-validation` both bring in `std` unless
compiled with `default-features = "false"`.  This caused a duplicate
lang item error.  Building both with `default-features = "false"`
prevents this error.  When building with `feature = "std"`, they should
both be built with the `std` feature, so this feature needs to be
explicitly depended on.

* Bump `impl_version`

* Make tests pass

Three tests used 1 less gas than they had previously.

* Try to un-break build

* Add a Cargo.lock file

* Revert offchain code

* Revert "Revert offchain code"

This reverts commit d216d08.

* Don’t try to send a body with a GET request

without adding a Transfer-Encoding or Content-Length header.

This has always been wrong, but hyperium/hyper#1925 hid the bug until
hyper was upgraded to 0.12.35.

* Change some more GET requests to POST requests

* Fix excess line width and remove an `extern crate`

* Delete commented-out extern crate

Co-Authored-By: Sergei Pepyakin <[email protected]>

* Fix regression in Cargo.toml files

dev-dependencies need `default-features = false`, too.

* Bump parity-wasm dependency

* Bump `futures-preview`

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <[email protected]>

* Update Cargo.lock files

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <[email protected]>

* Update core/service/src/chain_ops.rs

Co-Authored-By: Sergei Pepyakin <[email protected]>
@Shnatsel
Copy link

FYI there is a PR open against RustSec vulnerability database describing this issue as a potential request smuggling vector: rustsec/advisory-db#255

This article provides a nice explanation of request smuggling vulnerabilities: https://portswigger.net/web-security/request-smuggling

Any additional info from the maintainers would be appreciated.

@flipchan
Copy link

flipchan commented Apr 7, 2020

do you have an example of a curl request that would work for request smuggling? @seanmonstar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Development

No branches or pull requests

3 participants