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

Fixes #79265: Improper injection of Host header when using fopen for http requests #5201

Closed
wants to merge 1 commit into from

Conversation

miguelxpn
Copy link
Contributor

@miguelxpn miguelxpn commented Feb 24, 2020

fopen currently only looks at the first instance of a string on the request headers when setting the flag if that header is present or not. That causes some problems as malformed requests with two instances of the Host: header. This patch checks every instance of the string to set up the flags correctly.

@nikic
Copy link
Member

@nikic nikic commented Feb 24, 2020

New test is failing on Windows:

========DIFF========
001+ Fatal error: Uncaught Error: Call to undefined function pcntl_alarm() in C:\projects\php-src\ext\standard\tests\http\server.inc:11
002+ Stack trace:
003+ #0 C:\projects\php-src\ext\standard\tests\http\server.inc(43): http_server_init('tcp://127.0.0.1...', NULL)
004+ #1 C:\projects\php-src\ext\standard\tests\http\bug79265.php(8): http_server('tcp://127.0.0.1...', Array, NULL)
005+ #2 {main}
001- GET / HTTP/1.0
002- Connection: close
003- RandomHeader: localhost:8080
004- Cookie: foo=bar
005- Host: userspecifiedvalue
006+   thrown in C:\projects\php-src\ext\standard\tests\http\server.inc on line 11
========DONE========
FAIL Bug #79265 (Improper injection of Host header when using fopen for http requests) [C:\projects\php-src\ext\standard\tests\http\bug79265.phpt] 

@nikic
Copy link
Member

@nikic nikic commented Feb 24, 2020

Okay, looks like that's just the skipif missing.

@nikic
Copy link
Member

@nikic nikic commented Feb 24, 2020

Merged as d0d6050 with some cleanup in cc704f5.

Something I'm wondering about though is why this code is checking for \t or before the header name. Even if the obsolete header continuation syntax is used, the header should still always start after \r\n.

@nikic nikic closed this Feb 24, 2020
@nikic
Copy link
Member

@nikic nikic commented Feb 24, 2020

I went ahead and changed the code to only recognize \n as the start of a header in 56cdbe6.

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.

None yet

2 participants