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

Fix http splitting vulnerabilities #24004

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

auden-woolfson
Copy link
Contributor

@auden-woolfson auden-woolfson commented Nov 11, 2024

Description

Dealing with vulnerabilities found in static scan labeled http splitting vulnerability

== RELEASE NOTES ==

General Changes
* Improve HTTP responses from async page transport servlet, by sanitizing the URI before use. :pr:`24004`

@auden-woolfson auden-woolfson requested a review from a team as a code owner November 11, 2024 22:25
@auden-woolfson auden-woolfson changed the title fix AsyncPageTransportServlet issues fix http splitting vulnerabilities Nov 11, 2024
@steveburnett
Copy link
Contributor

Thanks for the release note entry! Suggest revising to follow the Order of changes in the Release Notes Guidelines. Maybe something like?

== RELEASE NOTES ==

General Changes
* Improve HTTP responses from async page transport servlet, by sanitizing the URI before use. :pr:`24004`

Please revise my suggestion if my phrasing doesn't correctly describe the work you've done.

@auden-woolfson auden-woolfson force-pushed the fix_http_splitting_vulnerabilities branch from a2f47a6 to d6b4b9e Compare November 19, 2024 19:34
@auden-woolfson auden-woolfson changed the title fix http splitting vulnerabilities Fix http splitting vulnerabilities Nov 19, 2024
@auden-woolfson auden-woolfson force-pushed the fix_http_splitting_vulnerabilities branch from d6b4b9e to 8090138 Compare November 21, 2024 21:04
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Nov 22, 2024
@prestodb-ci prestodb-ci requested review from a team, pdabre12 and czentgr and removed request for a team November 22, 2024 16:00
@@ -128,12 +128,14 @@ protected void parseURI(String requestURI, HttpServletRequest request, HttpServl
OutputBufferId bufferId = null;
long token = 0;

String sanitizedRequestURI = requestURI.replaceAll("[\\r\\n]", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

So this removes \r and \n from the URI. But having these chars would not be a valid URI in the first place. The server listener doesn't filter this out already?
The vulnerability talks about having these values in the header - because these values are used to separate header and body in HTTP.

Validating the input is fine. Just surprised this isn't already taken care of.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1.
We need to sanitize the request headers in order to handle this vulnerability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just to clarify, you are suggesting I get all headers from the request variable and sanitize each one using the same method here?

Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

Is it possible to write a test which can verify the new behavior?

@auden-woolfson auden-woolfson force-pushed the fix_http_splitting_vulnerabilities branch from bbcb63e to a329169 Compare December 3, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants