Skip to content

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

Closed
miguelxpn wants to merge 1 commit intophp:PHP-7.3from
miguelxpn:PHP-7.3
Closed

Fixes #79265: Improper injection of Host header when using fopen for http requests#5201
miguelxpn wants to merge 1 commit intophp:PHP-7.3from
miguelxpn:PHP-7.3

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 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 commented Feb 24, 2020

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

@nikic
Copy link
Member

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@miguelxpn @nikic