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

fix: prevent HEAD requests from writing body in streamhandler #1113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pankgeorg
Copy link

@pankgeorg pankgeorg commented Sep 20, 2023

(Note: if you're using your own streamhandler, you're on your own)
fixes: #1112

with this fix, 3 connections fly over the same connection with cURL

curl -Ivvvvvvvvvvvvvvvv http://localhost:1234 http://localhost:1234 http://localhost:1234
*   Trying 127.0.0.1:1234...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact
* Found bundle for host localhost: 0x563e0dc11eb0 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact
* Found bundle for host localhost: 0x563e0dc11eb0 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (127.0.0.1) port 1234 (#0)
> HEAD / HTTP/1.1
> Host: localhost:1234
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK

<
* Connection #0 to host localhost left intact

@pankgeorg pankgeorg changed the title fix: prevent HEAD requests from writing body fix: prevent HEAD requests from writing body in streamhandler Sep 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5cd586d) 82.70% compared to head (64d36e2) 82.71%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
+ Coverage   82.70%   82.71%   +0.01%     
==========================================
  Files          32       32              
  Lines        3053     3055       +2     
==========================================
+ Hits         2525     2527       +2     
  Misses        528      528              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm, yeah, this seems fine. I tried seeing if there was somewhere else we could enforce this (like startwrite or unsafe_write for Stream), but I think this fine. Mind adding a quick test and then we can merge?

@fredrikekre
Copy link
Member

I tried seeing if there was somewhere else we could enforce this (like startwrite or unsafe_write for Stream)

@quinnj why did you dismiss this idea? Seems like we could bail early in

function Base.unsafe_write(http::Stream, p::Ptr{UInt8}, n::UInt)
just like if n = 0? This would take care of both request handler and stream handler, I think?

Comment on lines +316 to +333
@testset "HEAD request without body" begin
sometext = "This is a big body that we don't want returned during a head"
handler = req -> begin
return HTTP.Response(200, [], sometext)
end
server = HTTP.serve!(handler; listenany=true)
port = HTTP.port(server)

response = HTTP.head("http://localhost:$port")
@test response.status == 200
@test String(response.body) == ""

response = HTTP.get("http://localhost:$port")
@test response.status == 200
@test String(response.body) == sometext

close(server)
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The very interesting thing here is that this fails with

HEAD request without body: Error During Test at /home/pgeorgakopoulos/pluto/HTTP.jl/test/server.jl:316
  Got exception outside of a @test
  HTTP.RequestError:
  HTTP.Request:
  HTTP.Messages.Request:
  """
  GET / HTTP/1.1
  Host: localhost:8081
  Accept: */*
  User-Agent: HTTP.jl/1.10.0-rc1
  Content-Length: 0
  Accept-Encoding: gzip
  
  """Underlying error:
  HTTP.Parsers.ParseError(:INVALID_STATUS_LINE, "2e\r")
...
caused by: TaskFailedException
  
      nested task error: HTTP.Parsers.ParseError(:INVALID_STATUS_LINE, "2e\r")
...

caused by: HTTP.Parsers.ParseError(:INVALID_STATUS_LINE, "This is a big body that we don't want returnedHTTP/1.1 200 OK\r")
      Stacktrace:
       [1] parse_status_line!(bytes::String, response::HTTP.Messages.Response)
         @ HTTP.Parsers ~/pluto/HTTP.jl/src/Parsers.jl:206

without this PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is very similar to a request smuggling attack.

@pankgeorg
Copy link
Author

I tried seeing if there was somewhere else we could enforce this (like startwrite or unsafe_write for Stream)

@quinnj why did you dismiss this idea? Seems like we could bail early in

function Base.unsafe_write(http::Stream, p::Ptr{UInt8}, n::UInt)

just like if n = 0? This would take care of both request handler and stream handler, I think?

n = 0 is totally stateless and always correct to be a no-op, while the check for HEAD also needs to know that the headers are already written and the current bytes to write are part of the body (and not the headers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HEAD requests should never write a body
4 participants