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

FISH-9690: adding new version of grizzly to validate headers RFC-9110 #7060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

breakponchito
Copy link
Contributor

@breakponchito breakponchito commented Nov 12, 2024

Adding Header validation for RFC-9110

Description

This is a security fix to prevent issues based on CVE reported here: [CVE-2024-45687](https://www.cve.org/CVERecord?id=CVE-2024-45687) and here glassfish issue

Important Info

Blockers

Testing

New tests

Manual testing:

Testing Performed

To test this set the dependency version on the payara server: 4.0.2.payara-p2 build and run
deploy the following reproducer:
ReproducerJDK11.zip

after starting the server set the following properties on the Payara configuration. You can do it by console ui, command line or directly on the domain.xml file:

  • -Dorg.glassfish.grizzly.http.STRICT_HEADER_NAME_VALIDATION_RFC_9110=true
  • -Dorg.glassfish.grizzly.http.STRICT_HEADER_VALUE_VALIDATION_RFC_9110=true

image

image

asadmin create-jvm-options --target=server-config "-Dorg.glassfish.grizzly.http.STRICT_HEADER_NAME_VALIDATION_RFC_9110\=true"

asadmin create-jvm-options --target=server-config "-Dorg.glassfish.grizzly.http.STRICT_HEADER_VALUE_VALIDATION_RFC_9110\=true"

after adding the properties you need to restart the server

by default on the grizzly side those properties are set as false

deploy the reproducer and make curl calls

Header Name Validation tests:

the result of any of those test should be error 400:
image

Header Value Validations tests:

In the case of header validation we can't be very strict on the validation that is why the \n is permitted an is evaluated as a new line on the content for the header value and the \0 is immediately not processed from curl because by default we can't consider that character on the request headers and the \r character can be included as a character on the header values and this will be rejected

Testing Environment

Windows 11, Azul jdk 11, maven 3.9.5

Documentation

working on documentation

Notes for Reviewers

@breakponchito breakponchito added the PR: DO NOT MERGE Don't merge PR until further notice label Nov 12, 2024
@breakponchito
Copy link
Contributor Author

this depend of the following PR: payara/patched-src-grizzly#39

@breakponchito
Copy link
Contributor Author

Jenkins test please

5 similar comments
@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@@ -617,6 +617,7 @@
<exclude>io.opentelemetry.extension</exclude>
<exclude>io.opentelemetry.instrumentation</exclude>
<exclude>fish.payara.shaded</exclude>
<exclude>org.glassfish.grizzly.http</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't ever be adding anything to this.
Please rework the Grizzly change

Suggested change
<exclude>org.glassfish.grizzly.http</exclude>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: DO NOT MERGE Don't merge PR until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants