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.

Sign up for GitHub

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

Jump to bottom

Fix possible HTTP Response Splitting #377

Merged
merged 2 commits into from
May 20, 2021
Merged

Fix possible HTTP Response Splitting #377

merged 2 commits into from
May 20, 2021

Conversation

macournoyer
Copy link
Owner

Ignore any response header with \r or \n in their value.

See GHSA-84j7-475p-hp8v

@ioquatix
Copy link
Collaborator

Should this raise an error?

@macournoyer
Copy link
Owner Author

I based my approach on Puma's https://github.com/puma/puma/blob/3846a4e0ae9cdfe80b4dfb7fc477f03ec4cabb9d/lib/puma/server.rb#L775

@macournoyer
Fix possible HTTP Response Splitting
dd24bf5 
Ignore any response header with \r or \n in their value

See GHSA-84j7-475p-hp8v
@ioquatix
Copy link
Collaborator

I personally think this should raise an exception as it represents a faulty request and I don't think we should try to interpret the remainder of the request as valid.

@ockeghem
Copy link

How about the following modifications of webrick?
https://github.com/ruby/webrick/blob/8658b1bd1fac9641806dc9efb4855c1c2e19eb72/lib/webrick/httpresponse.rb#L398

@ioquatix
Copy link
Collaborator

This looks good to me. Does this result in a 400 Bad Request?

@ockeghem
Copy link

ockeghem commented May 18, 2021 •
edited

This looks good to me. Does this result in a 400 Bad Request?

No. 500 Internal Server Error

curl -s -i http://localhost:3000/header/index | sed 's/\r/[CR]/'
HTTP/1.1 500 Internal Server Error[CR]
Content-Type: text/html; charset=ISO-8859-1[CR]
[CR]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">
<HTML>
  <HEAD><TITLE>Internal Server Error</TITLE></HEAD>
  <BODY>
    <H1>Internal Server Error</H1>
    WEBrick::HTTPResponse::InvalidHeader
    <HR>
    <ADDRESS>
     WEBrick/1.7.0 (Ruby/3.0.1/2021-04-05) at
     localhost:3000
    </ADDRESS>
  </BODY>
</HTML>

But, I also think 400 is a valid response code.

@macournoyer macournoyer merged commit 0c6d8e2 into master May 20, 2021
@macournoyer macournoyer deleted the header-injection branch May 20, 2021 00:48
@ioquatix ioquatix added this to the v1.8.1 milestone May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ioquatix ioquatix ioquatix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.8.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@macournoyer @ioquatix @ockeghem