Skip to content

Fix GH-9316: $http_response_header is wrong for long status line #9319

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

Closed
wants to merge 4 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Aug 12, 2022

While the reason-phrase in a HTTP response status line is usually
short, there is no actual limit specified by the RFCs. As such, we
must not assume that the line fits into the buffer (which is currently
128 bytes large).

Since there is no real need to present the complete status line, we
simply read and discard the rest of a long line.

@cmb69
Copy link
Member Author

cmb69 commented Aug 16, 2022

Any thoughts about this? Should we append the rest of the status line (instead of discarding it)? Should we just increase the buffer size (currently 256 bytes) to able to read longer status lines without further changes? Should there be a test case? Or should we just leave this as is? Oh, and of course, which branch should this target?

@cmb69 cmb69 marked this pull request as ready for review August 16, 2022 12:45
@TimWolla
Copy link
Member

Should we append the rest of the status line (instead of discarding it)?

Not necessary. The reason phrase only exists with HTTP/1.x and even then it is entirely optional and might be missing.

Should we just increase the buffer size (currently 256 bytes) to able to read longer status lines without further changes?

I'd say 256 is plenty for the status line. However the other headers should be able to support longer values. So if they already do, then making a dynamic allocation for the status line, might also make sense.

Should there be a test case?

Yes, I think there should be. It should test both overly long reason phrases and also an entirely empty one (unless there already is).

Or should we just leave this as is?

What exactly does "as is" refer to? The current situation as it is in “master”? Then no, it is a real bug when the status line spills over into another header. In fact this might even have security implications.

Oh, and of course, which branch should this target?

As this is a real bugfix, it should be backported to 8.0.x at the very least. 7.4 might be a stretch.

@cmb69
Copy link
Member Author

cmb69 commented Aug 17, 2022

However the other headers should be able to support longer values.

The other headers are already properly handled by not using a fixed size buffer. The buffer is only used to read the status line. Increasing the buffer would have helped in this case, but we never can be sure how long a status line actually is, so discarding the rest still would be necessary.

It should test both overly long reason phrases and also an entirely empty one (unless there already is).

Make sense. I'll add those tests.

As this is a real bugfix, it should be backported to 8.0.x at the very least. 7.4 might be a stretch.

ACK

cmb69 added 2 commits August 17, 2022 11:58
While the reason-phrase in a HTTP response status line is usually
short, there is no actual limit specified by the RFCs.  As such, we
must not assume that the line fits into the buffer (which is currently
128 bytes large).

Since there is no real need to present the complete status line, we
simply read and discard the rest of a long line.
@cmb69 cmb69 changed the base branch from master to PHP-8.0 August 17, 2022 10:33
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Fix and test look good to me, apart from this technicality, consider this an approval.

@cmb69 cmb69 closed this in 72da418 Aug 18, 2022
@cmb69 cmb69 deleted the cmb/gh9316 branch August 18, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contents of $http_response_header is incorrect with long http message
2 participants