Skip to content

Fix possible HTTP Response Splitting#377

Merged
macournoyer merged 2 commits intomasterfrom
header-injection
May 20, 2021
Merged

Fix possible HTTP Response Splitting#377
macournoyer merged 2 commits intomasterfrom
header-injection

Conversation

@macournoyer
Copy link
Copy Markdown
Owner

@macournoyer macournoyer commented Nov 29, 2020

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

See GHSA-84j7-475p-hp8v

@macournoyer macournoyer requested a review from ioquatix November 29, 2020 16:06
@ioquatix
Copy link
Copy Markdown
Collaborator

ioquatix commented Nov 29, 2020

Should this raise an error?

@macournoyer
Copy link
Copy Markdown
Owner Author

macournoyer commented Nov 30, 2020

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
Copy Markdown
Collaborator

ioquatix commented May 17, 2021

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
Copy Markdown

ockeghem commented May 18, 2021

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

@ioquatix
Copy link
Copy Markdown
Collaborator

ioquatix commented May 18, 2021

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

@ockeghem
Copy link
Copy Markdown

ockeghem commented May 18, 2021 •
edited
Loading

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.

3 participants

@macournoyer @ioquatix @ockeghem