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 CIO headers and chunked parsers #1547

Closed
wants to merge 3 commits into from
Closed

Fix CIO headers and chunked parsers #1547

wants to merge 3 commits into from

Conversation

cy6erGn0m
Copy link
Contributor

@cy6erGn0m cy6erGn0m commented Jan 5, 2020 •
edited
Loading

Subsystem
ktor-server-cio, ktor-client-cio

Motivation

  1. If a header is too long, then CIO parser simply cut it into two that is not correct.
  2. If both content length and chunked encoding specified, then we discard content length. This is correct according to RFC. However, this may lead to undesired consequences.
    Both may cause potential security issues, especially with a proxy server.

Solution

  1. Terminate request processing when a header body is too long
  2. Terminate request processing AND connection when the specified content length and decoded chunked content length do not match. Note that this violates RFC however it looks like we have no choice.

@cy6erGn0m cy6erGn0m added this to the 1.3.0 milestone Jan 5, 2020
@cy6erGn0m cy6erGn0m requested a review from e5l January 5, 2020 19:03
@cy6erGn0m cy6erGn0m added the Priority High priority or critical label Jan 5, 2020
@e5l
Copy link
Member

e5l commented Jan 9, 2020

Commit names

@cy6erGn0m
Copy link
Contributor Author

Merged

@cy6erGn0m cy6erGn0m closed this Jan 9, 2020
@cy6erGn0m cy6erGn0m deleted the cy/fix-chunked-with-length branch January 9, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e5l e5l e5l approved these changes

Assignees
No one assigned
Labels
Priority High priority or critical
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cy6erGn0m @e5l